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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
