[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
aaron.ballman added a comment. FWIW, now that https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c has reached a conclusion, I think this patch may be worth resurrecting, if you're interested. https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/38 has most of the conclusion which includes a diagnostic option roughly similar to what you have in this patch, though there was a request for the diagnostic in this patch to be named `-Wstrict-uses-without-prototype` so that it can encompass other cases like function pointer assignment. 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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher abandoned this revision.
delcypher added inline comments.
Herald added a project: All.
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;
aaron.ballman wrote:
> delcypher wrote:
> > aaron.ballman wrote:
> > > delcypher wrote:
> > > > aaron.ballman wrote:
> > > > > delcypher wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 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).
> > > > > > @aaron.ballman Thanks for the helpful feedback.
> > > > > >
> > > > > > > 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).
> > > > > >
> > > > > > That's a fair criticism. I think the diagnostic message you
> > > > > > suggest is a lot more helpful so I'll go for something like that.
> > > > > >
> > > > > > > Further, why does calling a function *with no arguments* matter
> > > > > > > when it has no prototype?
> > > > > >
> > > > > > The reason was to avoid the warning being noisy. E.g. I didn't the
> > > > > > warning to fire in this situation.
> > > > > >
> > > > > > ```
> > > > > > // Header.h
> > > > > > int f(); // The user forgot to put `void` between parentheses
> > > > > >
> > > > > > // Source.c
> > > > > > int f(void) { ... }
> > > > > >
> > > > > > // Caller.c
> > > > > > #include "Header.h"
> > > > > >
> > > > > > int main() {
> > > > > > return f();
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > > Forgetting to put `void` in the declaration of `f()` is a pretty
> > > > > > common thing to do because a lot of people read `int f()` as
> > > > > > declaring a function that takes no arguments (it does in C++ but
> > > > > > not in C).
> > > > > >
> > > > > > I don't want the warning to be noisy because I was planning on
> > > > > > switching it on by default in open source and in a downstream
> > > > > > use-case make it an error.
> > > > > >
> > > > > > How about this as a compromise? Split the warning into two separate
> > > > > > warnings
> > > > > >
> > > > > > * `strict-call-without-prototype` - Warns on calls to functions
> > > > > > without a prototype when no arguments are passed. Not enabled by
> > > > > > default
> > > > > > * `call-without-prototype` -Warns on calls to functions without a
> > > > > > prototype when arguments are passed. Enable this by default.
> > > > > >
> > > > > > Alternatively we could enable both by default. That would still
> > > > > > allow me to make `call-without-prototype` an error and
> > > > > > `strict-call-without-prototype` not be an error for my downstream
> > > > > > use-case.
> > > > > >
> > > > > > Thoughts?
> > > > > > Forgetting to put void in the declaration of f() is a pretty common
> > > > > > thing to do because a lot of people read int f() as declaring a
> > > > > > function that takes no arguments (it does in C++ but not in C).
> > > > >
> > > > > Yup, and this is exactly why I think we *should* be warning. That is
> > > > > a function without a prototype, so the code is very brittle and
> > > > > dangerous at the call site. The fact that the call site *currently*
> > > > > is correct doesn't mean it's *intentionally* correct. e.g.,
> > > > > ```
> > > > > // Header.h
> > > > > int f(); // No prototype
> > > > >
> > > > > // Source.c
> > > > > int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
> > > > >
> > > > > // OtherSource.c
> > > > > #include "Header.h"
> > > > >
> > > > > int main() {
> > > > > return f(); // No diagnostic with this patch, but still have the UB.
> > > > > }
> > > > > ```
> > > > >
> > > > > > I don't want the warning to be
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
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;
delcypher wrote:
> aaron.ballman wrote:
> > delcypher wrote:
> > > aaron.ballman wrote:
> > > > delcypher wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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).
> > > > > @aaron.ballman Thanks for the helpful feedback.
> > > > >
> > > > > > 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).
> > > > >
> > > > > That's a fair criticism. I think the diagnostic message you suggest
> > > > > is a lot more helpful so I'll go for something like that.
> > > > >
> > > > > > Further, why does calling a function *with no arguments* matter
> > > > > > when it has no prototype?
> > > > >
> > > > > The reason was to avoid the warning being noisy. E.g. I didn't the
> > > > > warning to fire in this situation.
> > > > >
> > > > > ```
> > > > > // Header.h
> > > > > int f(); // The user forgot to put `void` between parentheses
> > > > >
> > > > > // Source.c
> > > > > int f(void) { ... }
> > > > >
> > > > > // Caller.c
> > > > > #include "Header.h"
> > > > >
> > > > > int main() {
> > > > > return f();
> > > > > }
> > > > > ```
> > > > >
> > > > > Forgetting to put `void` in the declaration of `f()` is a pretty
> > > > > common thing to do because a lot of people read `int f()` as
> > > > > declaring a function that takes no arguments (it does in C++ but not
> > > > > in C).
> > > > >
> > > > > I don't want the warning to be noisy because I was planning on
> > > > > switching it on by default in open source and in a downstream
> > > > > use-case make it an error.
> > > > >
> > > > > How about this as a compromise? Split the warning into two separate
> > > > > warnings
> > > > >
> > > > > * `strict-call-without-prototype` - Warns on calls to functions
> > > > > without a prototype when no arguments are passed. Not enabled by
> > > > > default
> > > > > * `call-without-prototype` -Warns on calls to functions without a
> > > > > prototype when arguments are passed. Enable this by default.
> > > > >
> > > > > Alternatively we could enable both by default. That would still allow
> > > > > me to make `call-without-prototype` an error and
> > > > > `strict-call-without-prototype` not be an error for my downstream
> > > > > use-case.
> > > > >
> > > > > Thoughts?
> > > > > Forgetting to put void in the declaration of f() is a pretty common
> > > > > thing to do because a lot of people read int f() as declaring a
> > > > > function that takes no arguments (it does in C++ but not in C).
> > > >
> > > > Yup, and this is exactly why I think we *should* be warning. That is a
> > > > function without a prototype, so the code is very brittle and dangerous
> > > > at the call site. The fact that the call site *currently* is correct
> > > > doesn't mean it's *intentionally* correct. e.g.,
> > > > ```
> > > > // Header.h
> > > > int f(); // No prototype
> > > >
> > > > // Source.c
> > > > int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
> > > >
> > > > // OtherSource.c
> > > > #include "Header.h"
> > > >
> > > > int main() {
> > > > return f(); // No diagnostic with this patch, but still have the UB.
> > > > }
> > > > ```
> > > >
> > > > > I don't want the warning to be noisy because I was planning on
> > > > > switching it on by default in open source and in a downstream
> > > > > use-case make it an error.
> > > >
> > > > Hmmm. Thinking out loud here.
> > > >
> > > > Functions without prototypes were standardized in C89 as a deprecated
> > > > feature (C89 3.9.4, 3.9.5).
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher 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;
aaron.ballman wrote:
> delcypher wrote:
> > aaron.ballman wrote:
> > > delcypher wrote:
> > > > aaron.ballman wrote:
> > > > > 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).
> > > > @aaron.ballman Thanks for the helpful feedback.
> > > >
> > > > > 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).
> > > >
> > > > That's a fair criticism. I think the diagnostic message you suggest is
> > > > a lot more helpful so I'll go for something like that.
> > > >
> > > > > Further, why does calling a function *with no arguments* matter when
> > > > > it has no prototype?
> > > >
> > > > The reason was to avoid the warning being noisy. E.g. I didn't the
> > > > warning to fire in this situation.
> > > >
> > > > ```
> > > > // Header.h
> > > > int f(); // The user forgot to put `void` between parentheses
> > > >
> > > > // Source.c
> > > > int f(void) { ... }
> > > >
> > > > // Caller.c
> > > > #include "Header.h"
> > > >
> > > > int main() {
> > > > return f();
> > > > }
> > > > ```
> > > >
> > > > Forgetting to put `void` in the declaration of `f()` is a pretty common
> > > > thing to do because a lot of people read `int f()` as declaring a
> > > > function that takes no arguments (it does in C++ but not in C).
> > > >
> > > > I don't want the warning to be noisy because I was planning on
> > > > switching it on by default in open source and in a downstream use-case
> > > > make it an error.
> > > >
> > > > How about this as a compromise? Split the warning into two separate
> > > > warnings
> > > >
> > > > * `strict-call-without-prototype` - Warns on calls to functions
> > > > without a prototype when no arguments are passed. Not enabled by default
> > > > * `call-without-prototype` -Warns on calls to functions without a
> > > > prototype when arguments are passed. Enable this by default.
> > > >
> > > > Alternatively we could enable both by default. That would still allow
> > > > me to make `call-without-prototype` an error and
> > > > `strict-call-without-prototype` not be an error for my downstream
> > > > use-case.
> > > >
> > > > Thoughts?
> > > > Forgetting to put void in the declaration of f() is a pretty common
> > > > thing to do because a lot of people read int f() as declaring a
> > > > function that takes no arguments (it does in C++ but not in C).
> > >
> > > Yup, and this is exactly why I think we *should* be warning. That is a
> > > function without a prototype, so the code is very brittle and dangerous
> > > at the call site. The fact that the call site *currently* is correct
> > > doesn't mean it's *intentionally* correct. e.g.,
> > > ```
> > > // Header.h
> > > int f(); // No prototype
> > >
> > > // Source.c
> > > int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
> > >
> > > // OtherSource.c
> > > #include "Header.h"
> > >
> > > int main() {
> > > return f(); // No diagnostic with this patch, but still have the UB.
> > > }
> > > ```
> > >
> > > > I don't want the warning to be noisy because I was planning on
> > > > switching it on by default in open source and in a downstream use-case
> > > > make it an error.
> > >
> > > Hmmm. Thinking out loud here.
> > >
> > > Functions without prototypes were standardized in C89 as a deprecated
> > > feature (C89 3.9.4, 3.9.5). I'd like to get to the point where any code
> > > that doesn't pass `-ansi` is given a diagnostic (at least in pedantic
> > > mode outside of system headers) about this deprecation, though I could
> > > probably be persuaded to keep not diagnose in c89 mode if
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
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;
delcypher wrote:
> aaron.ballman wrote:
> > delcypher wrote:
> > > aaron.ballman wrote:
> > > > 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).
> > > @aaron.ballman Thanks for the helpful feedback.
> > >
> > > > 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).
> > >
> > > That's a fair criticism. I think the diagnostic message you suggest is a
> > > lot more helpful so I'll go for something like that.
> > >
> > > > Further, why does calling a function *with no arguments* matter when it
> > > > has no prototype?
> > >
> > > The reason was to avoid the warning being noisy. E.g. I didn't the
> > > warning to fire in this situation.
> > >
> > > ```
> > > // Header.h
> > > int f(); // The user forgot to put `void` between parentheses
> > >
> > > // Source.c
> > > int f(void) { ... }
> > >
> > > // Caller.c
> > > #include "Header.h"
> > >
> > > int main() {
> > > return f();
> > > }
> > > ```
> > >
> > > Forgetting to put `void` in the declaration of `f()` is a pretty common
> > > thing to do because a lot of people read `int f()` as declaring a
> > > function that takes no arguments (it does in C++ but not in C).
> > >
> > > I don't want the warning to be noisy because I was planning on switching
> > > it on by default in open source and in a downstream use-case make it an
> > > error.
> > >
> > > How about this as a compromise? Split the warning into two separate
> > > warnings
> > >
> > > * `strict-call-without-prototype` - Warns on calls to functions without
> > > a prototype when no arguments are passed. Not enabled by default
> > > * `call-without-prototype` -Warns on calls to functions without a
> > > prototype when arguments are passed. Enable this by default.
> > >
> > > Alternatively we could enable both by default. That would still allow me
> > > to make `call-without-prototype` an error and
> > > `strict-call-without-prototype` not be an error for my downstream
> > > use-case.
> > >
> > > Thoughts?
> > > Forgetting to put void in the declaration of f() is a pretty common thing
> > > to do because a lot of people read int f() as declaring a function that
> > > takes no arguments (it does in C++ but not in C).
> >
> > Yup, and this is exactly why I think we *should* be warning. That is a
> > function without a prototype, so the code is very brittle and dangerous at
> > the call site. The fact that the call site *currently* is correct doesn't
> > mean it's *intentionally* correct. e.g.,
> > ```
> > // Header.h
> > int f(); // No prototype
> >
> > // Source.c
> > int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
> >
> > // OtherSource.c
> > #include "Header.h"
> >
> > int main() {
> > return f(); // No diagnostic with this patch, but still have the UB.
> > }
> > ```
> >
> > > I don't want the warning to be noisy because I was planning on switching
> > > it on by default in open source and in a downstream use-case make it an
> > > error.
> >
> > Hmmm. Thinking out loud here.
> >
> > Functions without prototypes were standardized in C89 as a deprecated
> > feature (C89 3.9.4, 3.9.5). I'd like to get to the point where any code
> > that doesn't pass `-ansi` is given a diagnostic (at least in pedantic mode
> > outside of system headers) about this deprecation, though I could probably
> > be persuaded to keep not diagnose in c89 mode if that's a massive pain
> > point. But if in C99 or later, I definitely see no reason not to diagnose
> > the declarations as deprecated by default.
> >
> > However, calling a function without a prototype declaration is not itself
> > indicative
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
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 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 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();
> > > > > const FunctionType *NewFuncType = NewQType->getAs();
> > > > > const FunctionProtoType *OldProto = nullptr;
> > > > > if (MergeTypeWithOld && isa(NewFuncType) &&
> > > > > (OldProto = dyn_cast(OldFuncType))) {
> > > > > // The old declaration provided a function prototype, but the
> > > > > // new declaration does not. Merge in the prototype.
> > > > > ```
> > > > >
> > > > > ` isa(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(NewFuncType)` with
> > > > >
> > > > > ```
> > > > > (isa(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 function definition it will be marked
> > > > // as not having a prototype. If `Old` has a prototype
> > > > // then to "merge" we should mark the K 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(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();
> > >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(T)) {
> > >3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
> > >...
> > >3107 }
> > >3108
> > >3109 return cast(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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher 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;
aaron.ballman wrote:
> delcypher wrote:
> > aaron.ballman wrote:
> > > 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).
> > @aaron.ballman Thanks for the helpful feedback.
> >
> > > 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).
> >
> > That's a fair criticism. I think the diagnostic message you suggest is a
> > lot more helpful so I'll go for something like that.
> >
> > > Further, why does calling a function *with no arguments* matter when it
> > > has no prototype?
> >
> > The reason was to avoid the warning being noisy. E.g. I didn't the warning
> > to fire in this situation.
> >
> > ```
> > // Header.h
> > int f(); // The user forgot to put `void` between parentheses
> >
> > // Source.c
> > int f(void) { ... }
> >
> > // Caller.c
> > #include "Header.h"
> >
> > int main() {
> > return f();
> > }
> > ```
> >
> > Forgetting to put `void` in the declaration of `f()` is a pretty common
> > thing to do because a lot of people read `int f()` as declaring a function
> > that takes no arguments (it does in C++ but not in C).
> >
> > I don't want the warning to be noisy because I was planning on switching it
> > on by default in open source and in a downstream use-case make it an error.
> >
> > How about this as a compromise? Split the warning into two separate warnings
> >
> > * `strict-call-without-prototype` - Warns on calls to functions without a
> > prototype when no arguments are passed. Not enabled by default
> > * `call-without-prototype` -Warns on calls to functions without a prototype
> > when arguments are passed. Enable this by default.
> >
> > Alternatively we could enable both by default. That would still allow me to
> > make `call-without-prototype` an error and `strict-call-without-prototype`
> > not be an error for my downstream use-case.
> >
> > Thoughts?
> > Forgetting to put void in the declaration of f() is a pretty common thing
> > to do because a lot of people read int f() as declaring a function that
> > takes no arguments (it does in C++ but not in C).
>
> Yup, and this is exactly why I think we *should* be warning. That is a
> function without a prototype, so the code is very brittle and dangerous at
> the call site. The fact that the call site *currently* is correct doesn't
> mean it's *intentionally* correct. e.g.,
> ```
> // Header.h
> int f(); // No prototype
>
> // Source.c
> int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
>
> // OtherSource.c
> #include "Header.h"
>
> int main() {
> return f(); // No diagnostic with this patch, but still have the UB.
> }
> ```
>
> > I don't want the warning to be noisy because I was planning on switching it
> > on by default in open source and in a downstream use-case make it an error.
>
> Hmmm. Thinking out loud here.
>
> Functions without prototypes were standardized in C89 as a deprecated feature
> (C89 3.9.4, 3.9.5). I'd like to get to the point where any code that doesn't
> pass `-ansi` is given a diagnostic (at least in pedantic mode outside of
> system headers) about this deprecation, though I could probably be persuaded
> to keep not diagnose in c89 mode if that's a massive pain point. But if in
> C99 or later, I definitely see no reason not to diagnose the declarations as
> deprecated by default.
>
> However, calling a function without a prototype declaration is not itself
> indicative of a programming mistake and is also not deprecated (it just stops
> being a problem once all functions are required to have a prototype), so I'm
> not certain it's well-motivated to enable the new diagnostic by default. This
> is a bit more like use of VLAs, in that it's a
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
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;
delcypher wrote:
> aaron.ballman wrote:
> > 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).
> @aaron.ballman Thanks for the helpful feedback.
>
> > 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).
>
> That's a fair criticism. I think the diagnostic message you suggest is a lot
> more helpful so I'll go for something like that.
>
> > Further, why does calling a function *with no arguments* matter when it has
> > no prototype?
>
> The reason was to avoid the warning being noisy. E.g. I didn't the warning to
> fire in this situation.
>
> ```
> // Header.h
> int f(); // The user forgot to put `void` between parentheses
>
> // Source.c
> int f(void) { ... }
>
> // Caller.c
> #include "Header.h"
>
> int main() {
> return f();
> }
> ```
>
> Forgetting to put `void` in the declaration of `f()` is a pretty common thing
> to do because a lot of people read `int f()` as declaring a function that
> takes no arguments (it does in C++ but not in C).
>
> I don't want the warning to be noisy because I was planning on switching it
> on by default in open source and in a downstream use-case make it an error.
>
> How about this as a compromise? Split the warning into two separate warnings
>
> * `strict-call-without-prototype` - Warns on calls to functions without a
> prototype when no arguments are passed. Not enabled by default
> * `call-without-prototype` -Warns on calls to functions without a prototype
> when arguments are passed. Enable this by default.
>
> Alternatively we could enable both by default. That would still allow me to
> make `call-without-prototype` an error and `strict-call-without-prototype`
> not be an error for my downstream use-case.
>
> Thoughts?
> Forgetting to put void in the declaration of f() is a pretty common thing to
> do because a lot of people read int f() as declaring a function that takes no
> arguments (it does in C++ but not in C).
Yup, and this is exactly why I think we *should* be warning. That is a function
without a prototype, so the code is very brittle and dangerous at the call
site. The fact that the call site *currently* is correct doesn't mean it's
*intentionally* correct. e.g.,
```
// Header.h
int f(); // No prototype
// Source.c
int f(int a, int b) { return 0; } // Has a prototype, no diagnostic
// OtherSource.c
#include "Header.h"
int main() {
return f(); // No diagnostic with this patch, but still have the UB.
}
```
> I don't want the warning to be noisy because I was planning on switching it
> on by default in open source and in a downstream use-case make it an error.
Hmmm. Thinking out loud here.
Functions without prototypes were standardized in C89 as a deprecated feature
(C89 3.9.4, 3.9.5). I'd like to get to the point where any code that doesn't
pass `-ansi` is given a diagnostic (at least in pedantic mode outside of system
headers) about this deprecation, though I could probably be persuaded to keep
not diagnose in c89 mode if that's a massive pain point. But if in C99 or
later, I definitely see no reason not to diagnose the declarations as
deprecated by default.
However, calling a function without a prototype declaration is not itself
indicative of a programming mistake and is also not deprecated (it just stops
being a problem once all functions are required to have a prototype), so I'm
not certain it's well-motivated to enable the new diagnostic by default. This
is a bit more like use of VLAs, in that it's a common situation to accidentally
declare a function without a prototype. So having a "congrats, you're using
this feature" warning (like we did for `-Wvla`) for people who don't want to
accidentally use it seems reasonable. But "use" is a bit
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher 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;
aaron.ballman wrote:
> 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).
@aaron.ballman Thanks for the helpful feedback.
> 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).
That's a fair criticism. I think the diagnostic message you suggest is a lot
more helpful so I'll go for something like that.
> Further, why does calling a function *with no arguments* matter when it has
> no prototype?
The reason was to avoid the warning being noisy. E.g. I didn't the warning to
fire in this situation.
```
// Header.h
int f(); // The user forgot to put `void` between parentheses
// Source.c
int f(void) { ... }
// Caller.c
#include "Header.h"
int main() {
return f();
}
```
Forgetting to put `void` in the declaration of `f()` is a pretty common thing
to do because a lot of people read `int f()` as declaring a function that takes
no arguments (it does in C++ but not in C).
I don't want the warning to be noisy because I was planning on switching it on
by default in open source and in a downstream use-case make it an error.
How about this as a compromise? Split the warning into two separate warnings
* `strict-call-without-prototype` - Warns on calls to functions without a
prototype when no arguments are passed. Not enabled by default
* `call-without-prototype` -Warns on calls to functions without a prototype
when arguments are passed. Enable this by default.
Alternatively we could enable both by default. That would still allow me to
make `call-without-prototype` an error and `strict-call-without-prototype` not
be an error for my downstream use-case.
Thoughts?
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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher added inline comments.
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 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 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();
> > > const FunctionType *NewFuncType = NewQType->getAs();
> > > const FunctionProtoType *OldProto = nullptr;
> > > if (MergeTypeWithOld && isa(NewFuncType) &&
> > > (OldProto = dyn_cast(OldFuncType))) {
> > > // The old declaration provided a function prototype, but the
> > > // new declaration does not. Merge in the prototype.
> > > ```
> > >
> > > ` isa(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(NewFuncType)` with
> > >
> > > ```
> > > (isa(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 function definition it will be marked
> > // as not having a prototype. If `Old` has a prototype
> > // then to "merge" we should mark the K 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(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();
>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(T)) {
>3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
>...
>3107 }
>3108
>3109 return cast(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(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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
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 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 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();
> > > const FunctionType *NewFuncType = NewQType->getAs();
> > > const FunctionProtoType *OldProto = nullptr;
> > > if (MergeTypeWithOld && isa(NewFuncType) &&
> > > (OldProto = dyn_cast(OldFuncType))) {
> > > // The old declaration provided a function prototype, but the
> > > // new declaration does not. Merge in the prototype.
> > > ```
> > >
> > > ` isa(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(NewFuncType)` with
> > >
> > > ```
> > > (isa(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 function definition it will be marked
> > // as not having a prototype. If `Old` has a prototype
> > // then to "merge" we should mark the K 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(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();
>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:
>
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
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 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 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();
> > const FunctionType *NewFuncType = NewQType->getAs();
> > const FunctionProtoType *OldProto = nullptr;
> > if (MergeTypeWithOld && isa(NewFuncType) &&
> > (OldProto = dyn_cast(OldFuncType))) {
> > // The old declaration provided a function prototype, but the
> > // new declaration does not. Merge in the prototype.
> > ```
> >
> > ` isa(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(NewFuncType)`
> > with
> >
> > ```
> > (isa(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 function definition it will be marked
> // as not having a prototype. If `Old` has a prototype
> // then to "merge" we should mark the K 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(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();
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(T)) {
3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
...
3107 }
3108
3109 return cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher added inline comments.
Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+ return a + b +c;
+}
+
delcypher wrote:
> delcypher wrote:
> > @NoQ Any ideas about this? It seems kind of weird that when merging
> > `not_a_prototype3` prototype with the K 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 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();
> const FunctionType *NewFuncType = NewQType->getAs();
> const FunctionProtoType *OldProto = nullptr;
> if (MergeTypeWithOld && isa(NewFuncType) &&
> (OldProto = dyn_cast(OldFuncType))) {
> // The old declaration provided a function prototype, but the
> // new declaration does not. Merge in the prototype.
> ```
>
> ` isa(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(NewFuncType)` with
>
> ```
> (isa(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 function definition it will be marked
// as not having a prototype. If `Old` has a prototype
// then to "merge" we should mark the K 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?
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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5530 + "calling function %0 with arguments when function has no prototype">, InGroup< + DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore; def warn_missing_variable_declarations : Warning< fcloutier wrote: > sp: I think this should be called "strict-call-without-prototype" (singular > call) as most warning flag names seem to prefer singulars. @fcloutier I'm open to renaming the warning. I originally made the warning plural because there were warnings near by that used it. E.g. * `missing-variable-declarations` * `strict-prototypes` * `missing-prototypes` If no one else chimes in I'll make it singular. Also, do you think we should drop the "strict" prefix? I.e. `call-without-prototype` instead of `strict-call-without-prototype`. The reason I'm thinking that using `strict` might be a bad idea is: 1. The warning isn't actually strict. A strict version of the warning would warn on **all calls to functions without prototypes**. We've decided to only warn on calls with arguments to avoid noisy warnings that result from people frequently forgetting to explicitly mark functions as taking no arguments. 2. Calling this warning `strict-call-without-prototype` precludes from adding a stricter version of warning in the future (i.e. we'd be force to name it something like `strict-strict-call-without-prototype` which is super confusing). 3. The intention in a future patch is to enable the warning by default. It seems odd to have a "strict" warning on by default given that there is precedence for doing the opposite (e.g. `-Wstrict-prototypes` is off by default). 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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
fcloutier accepted this revision. fcloutier added a comment. This revision is now accepted and ready to land. I think that this is a good warning and I'll defer to the experts for what has to happen when prototypes merge with K definitions :) Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5530 + "calling function %0 with arguments when function has no prototype">, InGroup< + DiagGroup<"strict-calls-without-prototype">>, DefaultIgnore; def warn_missing_variable_declarations : Warning< sp: I think this should be called "strict-call-without-prototype" (singular call) as most warning flag names seem to prefer singulars. 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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher added inline comments.
Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+ return a + b +c;
+}
+
delcypher wrote:
> @NoQ Any ideas about this? It seems kind of weird that when merging
> `not_a_prototype3` prototype with the K 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 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();
const FunctionType *NewFuncType = NewQType->getAs();
const FunctionProtoType *OldProto = nullptr;
if (MergeTypeWithOld && isa(NewFuncType) &&
(OldProto = dyn_cast(OldFuncType))) {
// The old declaration provided a function prototype, but the
// new declaration does not. Merge in the prototype.
```
` isa(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(NewFuncType)` with
```
(isa(NewFuncType) || !New->hasPrototype())
```
However, I don't really know this code well enough to know if that's the right
fix.
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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher added a comment. Patch https://reviews.llvm.org/D116636 demonstrates the changes needed if the warning was enabled by default: 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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher added inline comments. Comment at: clang/test/Sema/warn-calls-without-prototype.c:39 + return a + b +c; +} + @NoQ Any ideas about this? It seems kind of weird that when merging `not_a_prototype3` prototype with the K 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 style function definitions. 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
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher updated this revision to Diff 397431.
delcypher added a comment.
Add another test case for function definitions without prototypes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116635/new/
https://reviews.llvm.org/D116635
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-calls-without-prototype.c
Index: clang/test/Sema/warn-calls-without-prototype.c
===
--- /dev/null
+++ clang/test/Sema/warn-calls-without-prototype.c
@@ -0,0 +1,151 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-calls-without-prototype -std=c99 -verify %s
+
+//--
+// Expected -Wstrict-calls-without-prototype warnings
+//--
+
+unsigned not_a_prototype();
+
+unsigned call_to_function_without_prototype(void) {
+ // expected-warning@+2{{calling function 'not_a_prototype' with arguments when function has no prototype}}
+ // expected-note@-4{{'not_a_prototype' declared here}}
+ return not_a_prototype(1);
+}
+
+// K style function declaration has no proto-type
+unsigned not_a_prototype2(a, b)
+int a;
+int b;
+{
+ return a + b;
+}
+
+unsigned call_to_function_without_prototype2(void) {
+ // expected-warning@+2{{calling function 'not_a_prototype2' with arguments when function has no prototype}}
+ // expected-note@-9{{'not_a_prototype2' declared here}}
+ return not_a_prototype2(1, 2);
+}
+
+// K style function seems to "hide" the prototype because the merged
+// FunctionDecl seems to not have a prototype.
+// FIXME(dliew): Is this a bug in `Sema::MergeFunctionDecl`?
+unsigned not_a_prototype3(int a, int b, int c); // Start with a prototype
+unsigned not_a_prototype3(a, b, c )
+ int a;
+ int b;
+ int c;
+{
+ return a + b +c;
+}
+
+unsigned call_to_function_without_prototype3(void) {
+ // FIXME(dliew): Should we actually warn about this? There is a prototype it
+ // just gets hidden when the FunctionDecls get merged.
+ // expected-warning@+2{{calling function 'not_a_prototype3' with arguments when function has no prototype}}
+ // expected-note@-12{{'not_a_prototype3' declared here}}
+ return not_a_prototype3(1, 2, 3);
+}
+
+// Merging two FunctionDecls without prototypes should result in a FunctionDecl without a prototype.
+unsigned not_a_prototype4(); // Start with something that has no prototype.
+unsigned not_a_prototype4(a, b, c)
+ int a;
+ int b;
+ int c;
+{
+ return a + b +c;
+}
+
+unsigned call_to_function_without_prototype4(void) {
+ // expected-warning@+2{{calling function 'not_a_prototype4' with arguments when function has no prototype}}
+ // expected-note@-10{{'not_a_prototype4' declared here}}
+ return not_a_prototype4(1, 2, 3);
+}
+
+// Definition does not have a prototype
+unsigned not_a_prototype5() {
+ return 0;
+}
+
+unsigned call_to_function_without_prototype5(void) {
+ // expected-warning@+3{{too many arguments in call to 'not_a_prototype5'}}
+ // expected-warning@+2{{calling function 'not_a_prototype5' with arguments when function has no prototype}}
+ // expected-note@-7{{'not_a_prototype5' declared here}}
+ return not_a_prototype5(1, 2, 3);
+}
+
+//--
+// No expected warnings from -Wstrict-calls-without-prototype
+//--
+
+unsigned call_to_function_without_declartion(void) {
+ // `-Wstrict-calls-without-prototype` doesn't emit a warning here because
+ // `-Wimplicit-function-declaration` cover this.
+ // expected-warning@+1{{implicit declaration of function 'function_without_declaration' is invalid in C99}}
+ return function_without_declaration(5);
+}
+
+unsigned call_builtins(void) {
+ // Builtin with a signature
+ int *alloc = (int *)__builtin_alloca(sizeof(int)); // no warning
+ *alloc = 5;
+
+ // Builtin without a meaningful signature
+ int gt = __builtin_isgreater(0.0, 0.1); // no warning
+
+ return *alloc + gt;
+}
+
+unsigned provides_prototype(int *a);
+unsigned provides_prototype_from_definition(int *a) {
+ return a[1];
+}
+unsigned provides_prototype2(void) {
+ return 0;
+}
+unsigned call_prototypes(void) {
+ int a[] = {0, 1, 2};
+ int result = provides_prototype_from_definition(a); // no warning
+ int result2 = provides_prototype(a); // no warning
+ // Deliberately pass arguments. Even in this case
+ //`-Wstrict-calls-without-prototype` should not fire.
+ // expected-error@+2{{too many arguments to function call, expected 0, have 1}}
+ // expected-note@-10{{'provides_prototype2' declared here}}
+ int result3 = provides_prototype2(5);
+ return result + result2 + result3;
+}
+
+unsigned will_be_refined();
+unsigned will_be_refined(void *); // refine the decl to be a
[PATCH] D116635: Add warning to detect when calls passing arguments are made to functions without prototypes.
delcypher created this revision.
delcypher added reviewers: arphaman, rapidsna, fcloutier, NoQ.
delcypher requested review of this revision.
Herald added a project: clang.
The warning is currently disabled by default but can be enabled with
`-Wstrict-calls-without-prototype`. Enabling it by default will be
handled by future patches.
The warning is intended to catch C code like this:
int f(); // No prototype, accepts any number of arguments of any type.
int call(void) {
return f(1, 2, 3);
}
While code like this is legal it can very easily invoke undefined behavior
by passing the wrong number of arguments or wrong types.
The warning only fires when arguments are passed to make it less noisy, i.e.
so that the warning does not fire on code like this:
int g();
int call(void) {
return g();
}
While the above code could invoke undefined behavior, if the definition of
`g()` takes no
arguments then the lack of a prototype is benign.
This warning while similar to `-Wstrict-prototypes` is distinct because
it warns at call sites rather at declarations.
This patch currently warns on calls to K style function declarations where a
previous declaration has a prototype. It is currently not clear if this is the
correct behavior
as noted in the included test case.
rdar://87118271
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116635
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/warn-calls-without-prototype.c
Index: clang/test/Sema/warn-calls-without-prototype.c
===
--- /dev/null
+++ clang/test/Sema/warn-calls-without-prototype.c
@@ -0,0 +1,131 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -Wstrict-calls-without-prototype -std=c99 -verify %s
+
+//--
+// Expected -Wstrict-calls-without-prototype warnings
+//--
+
+unsigned not_a_prototype();
+
+unsigned call_to_function_without_prototype(void) {
+ // expected-warning@+2{{calling function 'not_a_prototype' with arguments when function has no prototype}}
+ // expected-note@-4{{'not_a_prototype' declared here}}
+ return not_a_prototype(1);
+}
+
+// K style function declaration has no proto-type
+unsigned not_a_prototype2(a, b)
+int a;
+int b;
+{
+ return a + b;
+}
+
+unsigned call_to_function_without_prototype2(void) {
+ // expected-warning@+2{{calling function 'not_a_prototype2' with arguments when function has no prototype}}
+ // expected-note@-9{{'not_a_prototype2' declared here}}
+ return not_a_prototype2(1, 2);
+}
+
+// K style function seems to "hide" the prototype because the merged
+// FunctionDecl seems to not have a prototype.
+// FIXME(dliew): Is this a bug in `Sema::MergeFunctionDecl`?
+unsigned not_a_prototype3(int a, int b, int c); // Start with a prototype
+unsigned not_a_prototype3(a, b, c )
+ int a;
+ int b;
+ int c;
+{
+ return a + b +c;
+}
+
+unsigned call_to_function_without_prototype3(void) {
+ // FIXME(dliew): Should we actually warn about this? There is a prototype it
+ // just gets hidden when the FunctionDecls get merged.
+ // expected-warning@+2{{calling function 'not_a_prototype3' with arguments when function has no prototype}}
+ // expected-note@-12{{'not_a_prototype3' declared here}}
+ return not_a_prototype3(1, 2, 3);
+}
+
+// Merging two FunctionDecls without prototypes should result in a FunctionDecl without a prototype.
+unsigned not_a_prototype4(); // Start with something that has no prototype.
+unsigned not_a_prototype4(a, b, c)
+ int a;
+ int b;
+ int c;
+{
+ return a + b +c;
+}
+
+unsigned call_to_function_without_prototype4(void) {
+ // expected-warning@+2{{calling function 'not_a_prototype4' with arguments when function has no prototype}}
+ // expected-note@-10{{'not_a_prototype4' declared here}}
+ return not_a_prototype4(1, 2, 3);
+}
+
+//--
+// No expected warnings from -Wstrict-calls-without-prototype
+//--
+
+unsigned call_to_function_without_declartion(void) {
+ // `-Wstrict-calls-without-prototype` doesn't emit a warning here because
+ // `-Wimplicit-function-declaration` cover this.
+ // expected-warning@+1{{implicit declaration of function 'function_without_declaration' is invalid in C99}}
+ return function_without_declaration(5);
+}
+
+unsigned call_builtins(void) {
+ // Builtin with a signature
+ int *alloc = (int *)__builtin_alloca(sizeof(int)); // no warning
+ *alloc = 5;
+
+ // Builtin without a meaningful signature
+ int gt = __builtin_isgreater(0.0, 0.1); // no warning
+
+ return *alloc + gt;
+}
+
+unsigned provides_prototype(int *a);
+unsigned provides_prototype_from_definition(int *a) {
+ return
