[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+  return a + b +c;
+}
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116635/new/

https://reviews.llvm.org/D116635