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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5529
+def warn_call_function_without_prototype : Warning<
+  "calling function %0 with arguments when function has no prototype">, InGroup<
+  DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore;
----------------
This diagnostic doesn't tell me what's wrong with the code (and in fact, there's very possibly nothing wrong with the code whatsoever). Further, why does calling a function *with no arguments* matter when it has no prototype? I would imagine this should flag any call to a function without a prototype given that the function without a prototype may still expect arguments. e.g.,
```
// Header.h
int f();

// Source.c
int f(a) int a; { ... }

// Caller.c
#include "Header.h"

int main() {
  return f();
}
```
I think the diagnostic text should be updated to something more like `cannot verify %0 is being called with the correct number or %plural{1:type|:types}1 of arguments because it was declared without a prototype` (or something along those lines that explains what's wrong with the code).


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6391
 
+  // Diagnose calls that pass arguments to functions without a prototype
+  if (!LangOpts.CPlusPlus) {
----------------



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




================
Comment at: clang/test/Sema/warn-calls-without-prototype.c:152
+}
\ No newline at end of file

----------------
Please add a newline to the end of this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116635