[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
Dan Liew via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 6 14:07:23 PST 2022
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
More information about the cfe-commits
mailing list