delcypher added inline comments.
================
Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+ return a + b +c;
+}
+
----------------
aaron.ballman wrote:
> delcypher wrote:
> > NoQ wrote:
> > > delcypher wrote:
> > > > delcypher wrote:
> > > > > delcypher wrote:
> > > > > > @NoQ Any ideas about this? It seems kind of weird that when
> > > > > > merging `not_a_prototype3` prototype with the K&R style definition
> > > > > > of `not_a_prototype3` that the resulting FunctionDecl we see at the
> > > > > > call site in `call_to_function_without_prototype3` is marked as not
> > > > > > having a prototype.
> > > > > >
> > > > > > If I flip the order (see `not_a_prototype6`) then the merged
> > > > > > declaration is marked as having a prototype.
> > > > > >
> > > > > > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if
> > > > > > this just a peculiarity of K&R style function definitions.
> > > > > I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> > > > >
> > > > > ```lang=c++
> > > > > // C: Function types need to be compatible, not identical. This
> > > > > handles
> > > > > // duplicate function decls like "void f(int); void f(enum X);"
> > > > > properly.
> > > > > if (!getLangOpts().CPlusPlus &&
> > > > > Context.typesAreCompatible(OldQType, NewQType)) {
> > > > > const FunctionType *OldFuncType = OldQType->getAs<FunctionType>();
> > > > > const FunctionType *NewFuncType = NewQType->getAs<FunctionType>();
> > > > > const FunctionProtoType *OldProto = nullptr;
> > > > > if (MergeTypeWithOld && isa<FunctionNoProtoType>(NewFuncType) &&
> > > > > (OldProto = dyn_cast<FunctionProtoType>(OldFuncType))) {
> > > > > // The old declaration provided a function prototype, but the
> > > > > // new declaration does not. Merge in the prototype.
> > > > > ```
> > > > >
> > > > > ` isa<FunctionNoProtoType>(NewFuncType)` is false in this particular
> > > > > case, however `New` doesn't have a prototype (i.e.
> > > > > `New->hasPrototype()` is false). One fix might be to replace
> > > > > `isa<FunctionNoProtoType>(NewFuncType)` with
> > > > >
> > > > > ```
> > > > > (isa<FunctionNoProtoType>(NewFuncType) || !New->hasPrototype())
> > > > > ```
> > > > >
> > > > > However, I don't really know this code well enough to know if that's
> > > > > the right fix.
> > > > Okay. I think the above would actually be the wrong location for a fix
> > > > because in this case we don't need to go down the path that synthesizes
> > > > the parameters because we already know them for both `old` and `new` in
> > > > this situation.
> > > >
> > > > Instead I think the change would have to be in
> > > > `Sema::MergeCompatibleFunctionDecls` to do something like.
> > > >
> > > > ```lang=c++
> > > > // If New is a K&R function definition it will be marked
> > > > // as not having a prototype. If `Old` has a prototype
> > > > // then to "merge" we should mark the K&R function as having a
> > > > prototype.
> > > > if (!getLangOpts().CPlusPlus && Old->hasPrototype() &&
> > > > !New->hasPrototype())
> > > > New->setHasInheritedPrototype();
> > > > ```
> > > >
> > > > What I'm not sure about is if this is semantically the right thing to
> > > > do. Thoughts?
> > > Ok dunno but I definitely find this whole thing surprising. I'd expect
> > > this example to be the entirely normal situation for this code, where it
> > > sees that the new declaration has no prototype so it "inherits" it from
> > > the old declaration. But you're saying that
> > >
> > > > `isa<FunctionNoProtoType>(NewFuncType)` is false in this particular case
> > >
> > > Where does that proto-type come from then? I only see this code affecting
> > > the type
> > > ```lang=c++
> > > 3523 if (RequiresAdjustment) {
> > > 3524 const FunctionType *AdjustedType =
> > > New->getType()->getAs<FunctionType>();
> > > 3525 AdjustedType = Context.adjustFunctionType(AdjustedType,
> > > NewTypeInfo);
> > > 3526 New->setType(QualType(AdjustedType, 0));
> > > 3527 NewQType = Context.getCanonicalType(New->getType());
> > > 3528 }
> > > ```
> > > which doesn't seem to touch no-proto-types:
> > > ```lang=c++
> > > 3094 const FunctionType *ASTContext::adjustFunctionType(const
> > > FunctionType *T,
> > > 3095
> > > FunctionType::ExtInfo Info) {
> > > 3096 if (T->getExtInfo() == Info)
> > > 3097 return T;
> > > 3098
> > > 3099 QualType Result;
> > > 3100 if (const auto *FNPT = dyn_cast<FunctionNoProtoType>(T)) {
> > > 3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
> > > ...
> > > 3107 }
> > > 3108
> > > 3109 return cast<FunctionType>(Result.getTypePtr());
> > > 3110 }
> > > ```
> > > So it sounds like `New->getType()` was already with prototype from the
> > > start? Maybe whatever code has set that type should also have set the
> > > `HasInheritedPrototype` flag?
> > > Ok dunno but I definitely find this whole thing surprising. I'd expect
> > > this example to be the entirely normal situation for this code, where it
> > > sees that the new declaration has no prototype so it "inherits" it from
> > > the old declaration. But you're saying that
> > >
> > > `isa<FunctionNoProtoType>(NewFuncType) `is false in this particular case
> >
> > Yep.
> >
> > ```
> > (lldb) p NewFuncType->dump()
> > FunctionProtoType 0x128829430 'int (int, int)' cdecl
> > |-BuiltinType 0x12782bf10 'int'
> > |-BuiltinType 0x12782bf10 'int'
> > `-BuiltinType 0x12782bf10 'int'
> > (lldb) p OldFuncType->dump()
> > FunctionProtoType 0x128829430 'int (int, int)' cdecl
> > |-BuiltinType 0x12782bf10 'int'
> > |-BuiltinType 0x12782bf10 'int'
> > `-BuiltinType 0x12782bf10 'int'
> > ```
> >
> > I think we might be confusing the `FunctionNoProtoType` type and what the
> > `FunctionDecl::hasPrototype()` method does.
> >
> > ## `FunctionNoProtoType`
> >
> > I'm not 100% sure about this but I suspect `FunctionNoProtoType` exists
> > only for functions without any prototype that are written like this.
> >
> > ```
> > int function();
> > ```
> >
> > which would make some sense given what this code tries to do.
> >
> > ```lang=c++
> > if (MergeTypeWithOld && isa<FunctionNoProtoType>(NewFuncType) &&
> > (OldProto = dyn_cast<FunctionProtoType>(OldFuncType))) {
> > // The old declaration provided a function prototype, but the
> > // new declaration does not. Merge in the prototype.
> > ...
> > ```
> >
> > i.e. I think it's supposed to handle code like this
> >
> >
> > ```
> > int foo(int, int, int); // prototype
> > int foo(); // has no prototype, but when we merge decls it inherits the
> > prototype from the previous decl.
> > ```
> >
> > ## `FunctionDecl::hasPrototype()`
> >
> > This method is implemented as
> >
> > ```
> > bool hasPrototype() const {
> > return hasWrittenPrototype() || hasInheritedPrototype();
> > }
> > ```
> >
> > and those implementations get the data from data in the class without
> > examining its `FunctionType`. So it seems a `FunctionDecl`'s property of
> > having a prototype is independent of that FunctionDecl's `FunctionType`.
> >
> > ## K&R function declarations
> >
> > > So it sounds like New->getType() was already with prototype from the
> > > start? Maybe whatever code has set that type should also have set the
> > > HasInheritedPrototype flag?
> >
> > The nasty thing about K&R function declarations is they are treated as not
> > having a prototype according to the C11 standard (6.9.1.13) .
> >
> > ```lang=c
> > extern int max(int a, int b)
> > {
> > return a > b ? a : b;
> > }
> > ```
> >
> > ```lang=c
> > extern int max(a, b)
> > int a, b;
> > {
> > return a > b ? a : b;
> > }
> > ```
> >
> > //Here int a, b; is the declaration list for the parameters. The difference
> > between these two definitions is that the first form acts as a prototype
> > declaration that forces conversion of the arguments of subsequent calls to
> > the function, whereas the second form does not.//
> >
> > As much as I hate this, given the above it makes to make that in
> > `Sema::MergeFunctionDecl` `New->hasPrototype()` returns false. So in my
> > example
> >
> > ```lang=c
> > unsigned not_a_prototype3(int a, int b, int c); // "Old" has a prototype
> > unsigned not_a_prototype3(a, b, c ) // "New" has no prototype
> > int a;
> > int b;
> > int c;
> > {
> > return a + b +c;
> > }
> > ```
> >
> > The question is, what should we do when trying to merge these two
> > FunctionDecls?
> > @NoQ Any ideas about this? It seems kind of weird that when merging
> > not_a_prototype3 prototype with the K&R style definition of
> > not_a_prototype3 that the resulting FunctionDecl we see at the call site in
> > call_to_function_without_prototype3 is marked as not having a prototype.
>
> I am reasonably certain this is not a bug, but C being weird:
> https://godbolt.org/z/sGj5aejrY.
>
> I'd have to double-check what C89 says, but IIRC, the only thing that was
> specified is how to tell whether the two types are compatible, and it's left
> implicit that the definition is the final arbiter of the function type.
>
>
@aaron.ballman Thanks for chiming in. I'll update the test cases to assume this
is the intended behavior.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116635/new/
https://reviews.llvm.org/D116635
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
