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 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 weird here -- this flags call
> > > sites but the issue is with the declaration of the function, not with its
> > > callers.
> > >
> > > So I'm more of the opinion that we should be strengthening the
> > > diagnostics here rather than weakening them, and I sort of think we
> > > should be focusing on the declarations and not the call expressions. As a
> > > WG14 member for the community, I'm definitely motivated to see us be more
> > > aggressive here because of proposals like:
> > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2841.htm. The committee
> > > is trying to remove support for function declarations without prototypes,
> > > and the empty paren case is basically the final sticking point.
> > > Diagnosing it more appropriately to our users would help avoid nasty
> > > surprises.
> > >
> > > Have you considered whether you could stomach strengthening
> > > `-Wstrict-prototypes` by enabling it by default outside of `-ansi` (or
> > > perhaps `-std=c89`)? I know this does not match GCC's behavior, but IIRC,
> > > GCC's behavior came about because they implemented `-Wstrict-prototypes`
> > > in around 1990, aka, just as prototypes were being deprecated in C (so it
> > > would have been incredibly disruptive to enable it at that point).
> > >
> > > > 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.
> > >
> > > We could definitely split the diagnostic into two if we're convinced that
> > > diagnosing call sites is the appropriate action to take.
> > > 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
> >
> > True. It's not necessarily a mistake but calling functions without a
> > prototype is very error prone due the lack of argument count and type
> > checking. This is why I think it might be worth flagging the potential
> > problem by default. I'm happy to not have it on by default if that is the
> > general consensus.
> >
> > > But "use" is a bit weird here -- this flags call sites but the issue is
> > > with the declaration of the function, not with its callers.
> >
> > You're right that the underlying issue is at the declaration and not at the
> > call sites. However, if I wanted to warn about all the declarations I would
> > just use `-Wstrict-prototypes` which is already implemented in Clang. I
> > don't consider that warning very pragmatic because it'll warn about
> > functions that I'm not calling which could make it extremely noisy. Instead
> > I wanted a more pragmatic (less noisy) warning. I consider what I'm
> > proposing to be more pragmatic because if a function is missing a
> > prototype, it **only matters when it is called** (i.e. this is where the
> > lack of a prototype will cause problems if the arguments/types aren't "just
> > right"). If I'm not calling a function I don't want to be told its missing
> > a prototype because it does not matter for the current compilation unit.
> >
> > > Have you considered whether you could stomach strengthening
> > > -Wstrict-prototypes by enabling it by default outside of -ansi (or
> > > perhaps -std=c89)?
> >
> > As I said above I don't think ` -Wstrict-prototypes` is very pragmatic.
> > It's probably good if you're trying to audit a header file, but noisy if
> > you're actually trying to compile code.
> >
> > Thinking about it I guess what I've proposed is a complement to
> > `-Wstrict-prototypes`. I don't see why clang couldn't have both warnings.
> > Thinking about it I guess what I've proposed is a complement to
> > -Wstrict-prototypes. I don't see why clang couldn't have both warnings.
>
> To me, it's about what action the user can take to respond to the diagnostic.
> I don't see the complement adding value in that regard. The user is calling a
> function that's declared somewhere without a prototype, but unless the user
> has control over the declaration, there is *nothing* they can do at the call
> site about it. Well, almost. They could add their own redeclaration with a
> prototype, but that's actually *worse* because now the redeclaration might
> get out of sync with the actual definition, but the users won't get any
> warnings about it.
>
> Basically, I don't think it's a problem worth diagnosing to *call* a function
> without a prototype, but I definitely agree it's more dangerous to *have*
> functions without prototypes which are called. So I'm absolutely sold on
> warning users about the dangers here, but I don't think a new warning is the
> right approach when we could strengthen the existing warning. The current
> diagnostic is effectively `-Wstrict-prototypes-but-only-when-called`.
@aaron.ballman Sorry for the slow response here, I was out sick.
> To me, it's about what action the user can take to respond to the diagnostic.
> I don't see the complement adding value in that regard. The user is calling a
> function that's declared somewhere without a prototype, but unless the user
> has control over the declaration, there is *nothing* they can do at the call
> site about it. Well, almost. They could add their own redeclaration with a
> prototype, but that's actually *worse* because now the redeclaration might
> get out of sync with the actual definition, but the users won't get any
> warnings about it.
While this is all true I think:
* The existing `-Wstrict-prototypes` also has this exact same problem (if you
can't change the declaration then it's hard to work around). This problem did
not stop us creating the `-Wstrict-prototypes` warning in clang and I don't
think that problem should stop us creating a new warning either.
* If the user can't adjust the header with the bad declaration they also have
the option of filing a bug against the vendor of the header file. If the
warning really causes them problems they can switch it off. If they really want
to switch it off only at the call site they could use pragmas.
>Basically, I don't think it's a problem worth diagnosing to *call* a function
>without a prototype, but I definitely agree it's more dangerous to *have*
>functions without prototypes which are called.
I'm a bit confused. If you agree it's more dangerous then why do you think it's
not worth diagnosing?
The point of **only looking at calls** to functions without prototypes is that
it is effectively a quieter version of `-Wstrict-prototypes`. That "quieter"
nature means it could conceivably be switched on by default in the future,
whereas I would never want to switch `-Wstrict-prototypes` on by default.
We can also do a better job of warning when we know what the call sites look
like.
For example, when warning about calls to functions without a prototype and the
call site doesn't pass args (not implemented in the patch yet) we can deduce
the function declaration is very likely missing a `void` between parentheses.
So we can warn about that specially.
> So I'm absolutely sold on warning users about the dangers here, but I don't
> think a new warning is the right approach when we could strengthen the
> existing warning.
What do you mean by "strengthen" here? The existing warning isn't very
sophisticated and I don't really see how we can make it better.
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
