This is an archive of the discontinued LLVM Phabricator instance.

[C89/C2x] Improve diagnostics around strict prototypes in C
ClosedPublic

Authored by aaron.ballman on Apr 1 2022, 5:33 AM.

Details

Summary

Functions without prototypes in C (also known as K&R C functions) were introduced into C89 as a deprecated feature and C2x is now reclaiming that syntax space with different semantics. However, Clang's -Wstrict-prototypes diagnostic is off-by-default (even in pedantic mode) and does not suffice to warn users about issues in their code.

This patch changes the behavior of -Wstrict-prototypes to only diagnose declarations and definitions which are not going to change behavior in C2x mode, and enables the diagnostic in -pedantic mode. The diagnostic is now specifically about the fact that the feature is deprecated.

It also adds -Wdeprecated-non-prototype, which is grouped under -Wstrict-prototypes and diagnoses declarations or definitions which will change behavior in C2x mode. This diagnostic is enabled by default because the risk is higher for the user to continue to use the deprecated feature.

A few things to note:

  • The RFC for this can be found at: https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c
  • There is some awkward overlap between the diagnostics, but the best time to diagnose a function type without a prototype is when forming the function type, but we don't know whether the behavior will change in C2x at that point. So we alter the behavior sometimes depending on whether -Wstrict-prototypes is enabled in an effort to keep the diagnostics understandable and not too chatty.
  • There will be another patch for handling call site behavior as a follow-up.

Diff Detail

Event Timeline

aaron.ballman created this revision.Apr 1 2022, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 5:33 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
aaron.ballman requested review of this revision.Apr 1 2022, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 5:33 AM

Rebased, no changes.

lenary added a subscriber: lenary.Apr 1 2022, 6:15 AM
cor3ntin added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
5566–5567

"declaration of a function without a prototype is deprecated", may be slightly better?

clang/lib/Sema/SemaDecl.cpp
3881

I wonder if

  • This shouldn't be placed in a separate function for clarity
  • We could bail out early for built ins ?
clang/lib/Sema/SemaType.cpp
5560–5562

if (!LangOpts.CPlusPlus) is probably enough here

aaron.ballman marked an inline comment as done.Apr 1 2022, 7:48 AM
aaron.ballman added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
5566–5567

Sure, I can reword the rest so they're consistent.

clang/lib/Sema/SemaDecl.cpp
3881

The change looks more complicated than it is because of the indentation changes -- I'm not certain putting this in its own function gains a whole lot (it's not reusable below; I already looked into doing that and wasn't happy with the results).

We can't bail out early for builtins because we still issue diagnostics in cases where only one side is a builtin. e.g.,

void exit(); // We still want to diagnose this as changing behavior specifically

See test/Sema/knr-variadic-def.c for this test case.

clang/lib/Sema/SemaType.cpp
5560–5562

Hmm, yeah, you're right. Good catch!

Starting by looking at the test cases, I've got some suggestions on making the diagnostics a bit less confusing.

clang/test/CodeGen/2009-06-01-addrofknr.c
8

This message seems vaguer than necessary, since we know _for sure_ this is invalid.

Can we say something like: "K&R-style function definitions are deprecated in all versions of C and not supported in C2x"?

clang/test/Parser/declarators.c
5

Perhaps we should add an explanation to the message like
(specify '(void)' if you intended to accept no arguments)
to make it clearer to folks who aren't familiar with this weirdness yet?

clang/test/Sema/implicit-decl.c
25 ↗(On Diff #419734)

It's not "preceded by a function without a prototype", though, it's preceeded by a implicit declaration due to a call. Which we would've already diagnosed with the default-on -Wimplicit-function-declaration diagnostic.

Although...what _is_ the desired behavior of an implicit function declaration in C2x mode? If we permit it _at all_, it must still create a non-prototype function declaration, I expect. In that case this decl with types would still be valid, so the warning is just wrong.

clang/test/Sema/knr-def-call.c
16

I think we don't want to emit a warning here. If a declaration with params doesn't match a K&R definition, we already emit a conflicting-types error.

[[Well...except for one case, I've noticed -- we don't current emit any warning when a definition with no parameters fails to match a preceding declaration with params, e.g. void f(int); void f() { }. I'm quite surprised -- I'm willing to say that such code is 100% just a bug, not intentional. I note that GCC unconditionally rejects it. I think we should also be emitting an unconditional error in that case.]]]

Anyhow, when you have a K&R style definition with parameters, that -- all by itself -- is definitely invalid in C2x, so we don't need to emit any warning on the declaration.

clang/test/Sema/warn-deprecated-non-prototype.c
47

Maybe something like "this function declaration will conflict with the preceding declaration in C2x, because the preceding declaration does not specify arguments."

aaron.ballman marked 3 inline comments as done.Apr 1 2022, 10:48 AM
aaron.ballman added inline comments.
clang/test/CodeGen/2009-06-01-addrofknr.c
8

I'd like to avoid using K&R in the diagnostic text (we're inconsistent about K&R vs prototype, etc already, but I'd like to see us excise K&R from diagnostics because it's not particularly descriptive to anyone younger than about 40. :-D

How about: function declarations without a prototype are deprecated in all versions of C and not supported in C2x ?

clang/test/Parser/declarators.c
5

They're already offered a fixit for this situation, so I don't know that we need the extra explanation? The user currently gets something like this:

C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:16: warning: declaration of a function without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
void other_func();
               ^
                void

Do you still think the diagnostic needs to be made longer?

clang/test/Sema/implicit-decl.c
25 ↗(On Diff #419734)

It's not "preceded by a function without a prototype", though, it's preceeded by a implicit declaration due to a call. Which we would've already diagnosed with the default-on -Wimplicit-function-declaration diagnostic.

In order for that to work, Clang gins up an implicit declaration of extern int func(); which is the signature C89 requires, so I think the diagnostic here is fine (it's also a very weird edge case I don't expect people to hit all that often, since the only way to trigger this diagnostic is to define with a prototype the implicitly-declared function).

There are no implicit function declarations after C89 (warned by default, but should be an error). There's also no implicit int after C89 (not even warned on by default, but should also be an error). Personally, I would like to see us enable both diagnostics as warning-defaults-to-error in C99 through C17 (if possible) and error only in C2x and forward, but I've not proposed it or worked on it yet (fixing one horror show at a time, basically).

clang/test/Sema/knr-def-call.c
16

I'm quite surprised -- I'm willing to say that such code is 100% just a bug, not intentional. I note that GCC unconditionally rejects it. I think we should also be emitting an unconditional error in that case.

I'd rather we be consistent here -- either every mixture of prototype/unprototyped is an error, or they're all a warning. I've added your example as a test case and we warn on it as being a change of behavior in C2x, which I think is defensible.

Anyhow, when you have a K&R style definition with parameters, that -- all by itself -- is definitely invalid in C2x, so we don't need to emit any warning on the declaration.

I tend to agree, let me see what I can do.

clang/test/Sema/warn-deprecated-non-prototype.c
47

I can see the appeal, but that makes the diagnostic rather long and I'd still like to be consistent in the terminology we use. Given that the old diagnostic was -Wstrict-prototypes and the new one is -Wdeprecated-non-prototype, I think we don't need to tie ourselves into knots to avoid using the terminology. Do you feel strongly about rewording this?

aaron.ballman marked 5 inline comments as done.

Updating based on review feedback.

clang/test/CodeGen/2009-06-01-addrofknr.c
8

I went with a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x, let me know if you think it needs further adjusting. I also reworded the other diagnostics to sound similar.

clang/test/Parser/declarators.c
5

I elected to leave this alone unless you feel strongly.

clang/test/Sema/knr-def-call.c
16

I addressed this so we no longer diagnose the function with a prototype in the case where it precedes the function without a prototype.

clang/test/Sema/warn-deprecated-non-prototype.c
47

I've elected to leave this one alone unless you feel strongly.

Fix failing test case caught by precommit CI.

erichkeane accepted this revision.Apr 7 2022, 5:55 AM
This revision is now accepted and ready to land.Apr 7 2022, 5:55 AM

Thanks @erichkeane!

@jyknight, @hubert.reinterpretcast, @eli.friedman -- I'll likely land this sometime tomorrow unless I hear from you. But if I land it and you still have comments, I'll be happy to address them post commit.

jyknight added inline comments.Apr 12 2022, 7:03 AM
clang/test/CodeGen/2009-06-01-addrofknr.c
8

I do agree with you that "K&R" is perhaps not the best name either...

But I think this warning message may still be confusing to users. A typical C user I think will be surprised to see a warning about a "declaration" pointing to a definition -- even though that is technically accurate. Especially when we're complaining concretely about the weird old definition syntax, it would be better to say "definition".

It looks like the most common name for this is "old-style function definition" -- both GCC and MSVC use that name, and clang also did, before. (Hmmm. And GCC actually seems to place this particular warning under the off-by-default -Wold-style-definition flag -- which clang doesn't implement.) I don't know that "old-style" is a great description either, but "old-style function definition" definitely seems more understandable than "function declaration without a prototype" when talking about a K&R-style separated-arguments-types definition.

clang/test/Parser/declarators.c
5

I guess I sort of wondered if users might consider "without a prototype" to generally indicate "implicit declaration". So, the wording "declaration without a prototype" might be like..."wait you're complaining that my declaration doesn't have a declaration? Huh? It's right there!"

But, with the fixit hint suggesting the addition of "void", that's probably sufficiently explanatory.

clang/test/Sema/knr-def-call.c
16

I'd rather we be consistent here -- either every mixture of prototype/unprototyped is an error, or they're all a warning. I've added your example as a test case and we warn on it as being a change of behavior in C2x, which I think is defensible.

Even before, we are NOT consistent. We emit an error on void f(int); void f(x) float x; {}, but not for void f(int); void f() {}. In both cases, we have a prototyped declaration, followed by an old-style "prototypeless" definition. I think it would be sensible to diagnose with an unconditional error in both cases, not only the former.

aaron.ballman added inline comments.Apr 12 2022, 7:25 AM
clang/test/CodeGen/2009-06-01-addrofknr.c
8

But I think this warning message may still be confusing to users. A typical C user I think will be surprised to see a warning about a "declaration" pointing to a definition -- even though that is technically accurate. Especially when we're complaining concretely about the weird old definition syntax, it would be better to say "definition".

I can look into making it say "definition" when written on a definition, but this feels like it's starting to split awfully fine hairs to me, so if it turns out to add more code than I think it's worth, I may stick with "declaration" because it's still correct and I don't think the confusion should impede a user's ability to fix the bug *too much*.

clang/test/Parser/declarators.c
5

I guess I sort of wondered if users might consider "without a prototype" to generally indicate "implicit declaration". So, the wording "declaration without a prototype" might be like..."wait you're complaining that my declaration doesn't have a declaration? Huh? It's right there!"

That's a good point, we should watch to see if that sort of confusion does happen in practice (I'll keep my eyes peeled), but I also think the fix-it should be sufficiently clarifying for most folks.

clang/test/Sema/knr-def-call.c
16

Even before, we are NOT consistent. We emit an error on void f(int); void f(x) float x; {}, but not for void f(int); void f() {}. In both cases, we have a prototyped declaration, followed by an old-style "prototypeless" definition. I think it would be sensible to diagnose with an unconditional error in both cases, not only the former.

I don't think void f(int); void f() {} should be diagnosed solely as an issue with strict prototypes; I think it should be diagnosed the same as void f(int); void f(x) float x; {}, which is *not* about strict prototypes but *is* about the fact that the function types cannot merge because they conflict. Once there's a declaration with a prototype, the function always has a prototype (see C17 6.2.7p3 ) and redeclarations (including for definition) need to have a compatible signature (see C17 6.7.6.3p15). So I'd expect a conflicting types error diagnostic. I think it's defensible to *additionally* issue the strict prototypes warning (though if we can silence the warning because we're already issuing an error without introducing significant extra complexity, that would be ideal). I'll look into adding the error diagnostic.

jyknight added inline comments.Apr 12 2022, 8:22 AM
clang/test/Sema/knr-def-call.c
16

Sorry about being unclear -- what you propose is precisely what I meant to say.

aaron.ballman added inline comments.Apr 12 2022, 8:31 AM
clang/test/Sema/knr-def-call.c
16

Thank you for confirming (and poking me about this). As always, I really appreciate your input!

aaron.ballman added inline comments.Apr 12 2022, 9:10 AM
clang/test/CodeGen/2009-06-01-addrofknr.c
8

Yup, this turned out to be hard enough to not be worth it IMO. The issue is that when merging function declarations, we don't know whether the function we want to diagnose is a definition or not (we've not yet started parsing its body, we've just finished with its declarator).

Some of our users are not very happy with the churn probably caused by this change where the declaration has the "void" argument but the later definition does not have explicit "void".

void foo(void);

void foo() 
{
}

GCC does not warn about this usage: https://godbolt.org/z/zPP8qjc98

Any opinions?

Some of our users are not very happy with the churn probably caused by this change where the declaration has the "void" argument but the later definition does not have explicit "void".

void foo(void);

void foo() 
{
}

GCC does not warn about this usage: https://godbolt.org/z/zPP8qjc98

Any opinions?

Thank you for the feedback! I think it's debatable whether this is a bug or not, but it's an interesting case that I may think about a bit further before making any final decisions. My current thinking is...

We turned -Wstrict-prototypes into a pedantic warning specifically telling the user about K&R C function usage everywhere it occurs, and from that perspective void f() {} is correct to diagnose pedantically as the signature is specifically a K&R C one. However, the composite type between void f(void) (with a parameter list) and void f() {} (with an empty identifier list) is a function with a prototype of void(void) (per C99 6.2.7p3). Thus, a call to the function through the declaration can pass no arguments, and a call to the function through the definition can pass no arguments, and so it also seems reasonable to not diagnose.

(We added -Wdeprecated-non-prototype to cover the case where there's a behavioral change to the code in C2x, which diagnoses void f(int); and void f(i) int i; {} (this code will break in C2x), so despite the similarity with your case, it's a different diagnostic entirely.)

FWIW, GCC's behavior isn't particularly interesting because we're knowingly diverging from their strategy with the warning, so it's not too surprising that our behavior is different.

I think what sells it for me not being a bug is that we issue that warning when determining the function *type* when forming the declaration which happens before doing function merging. At the point where we issue this warning, we don't have enough information to know whether the function will eventually have a composite type with a prototype or not (we don't even have enough information to know whether it's for a function declaration or not; it could be for a typedef, etc), so suppressing the diagnostic would be rather difficult. And because it's a pedantic warning specifically, I think it's defensible that we're issuing the diagnostic.

Is disabling the pedantic warning an option for your users?

I think it's debatable whether this is a bug or not

For C99 through C17, I kind of agree, but for C2x (where the warning is still issued with -Wstrict-prototypes), my understanding is that void foo(void) and void foo() are equivalent; there is no unprototyped declaration. I think the diagnostic should at least be suppressed for C2x since we don't want to encourage programmers to explicitly add void when targeting that standard.

I think it's debatable whether this is a bug or not

For C99 through C17, I kind of agree, but for C2x (where the warning is still issued with -Wstrict-prototypes), my understanding is that void foo(void) and void foo() are equivalent; there is no unprototyped declaration. I think the diagnostic should at least be suppressed for C2x since we don't want to encourage programmers to explicitly add void when targeting that standard.

Good catch... -Wstrict-prototypes should be a noop in C2x mode! I'll work on fixing that.

Is disabling the pedantic warning an option for your users?

Disabling it wholesale is not an option since they actually want this warning (the older version). But we agreed to disable it specifically for the code where the warning was getting fired.
One instance is https://review.coreboot.org/c/coreboot/+/63936 .

I have been fixing our codebase to clean this and clean the instances. But it takes a lot of time and effort. Plus it takes a long time to clean one failure before I can find others (public CLs) https://chromium-review.googlesource.com/q/Wstrict-prototypes+owner:manojgupta

Is disabling the pedantic warning an option for your users?

Disabling it wholesale is not an option since they actually want this warning (the older version). But we agreed to disable it specifically for the code where the warning was getting fired.
One instance is https://review.coreboot.org/c/coreboot/+/63936 .

I have been fixing our codebase to clean this and clean the instances. But it takes a lot of time and effort. Plus it takes a long time to clean one failure before I can find others (public CLs) https://chromium-review.googlesource.com/q/Wstrict-prototypes+owner:manojgupta

Out of curiosity, are you using K&R C functions in your code base (definitions with an identifier list or a declaration with empty parens)? Basically, I'm wondering if you'd be able to enable -fno-knr-function?

I think it's debatable whether this is a bug or not

For C99 through C17, I kind of agree, but for C2x (where the warning is still issued with -Wstrict-prototypes), my understanding is that void foo(void) and void foo() are equivalent; there is no unprototyped declaration. I think the diagnostic should at least be suppressed for C2x since we don't want to encourage programmers to explicitly add void when targeting that standard.

Good catch... -Wstrict-prototypes should be a noop in C2x mode! I'll work on fixing that.

I fixed that issue in ef87865b98fa25af1d2c045bab1268b2a1503374, thanks for catching it.

Basically, I'm wondering if you'd be able to enable -fno-knr-function?

Thanks. this looks promising. Any ideas when -fno-knr-function support was added?

Unless I probably mis-interpreted something, -fno-knr-functions does not suppress the warning: https://godbolt.org/z/rbEfbbb33

Basically, I'm wondering if you'd be able to enable -fno-knr-function?

Thanks. this looks promising. Any ideas when -fno-knr-function support was added?

Oops, I had a slight typo, it's -fno-knr-functions plural. But it was added 9 days ago in https://github.com/llvm/llvm-project/commit/9955f14aaf9995f6f0f7bf058ac740463003c470, and as of today, using that flag will disable the -Wstrict-prototypes warnings entirely (as will specifying -std=c2x).

Unless I probably mis-interpreted something, -fno-knr-functions does not suppress the warning: https://godbolt.org/z/rbEfbbb33

Godbolt hasn't had the chance to catch up to https://github.com/llvm/llvm-project/commit/ef87865b98fa25af1d2c045bab1268b2a1503374 yet.

Tried locally but I still see the warning with -fno-knr-functions. It also says that the argument is unused.

bin/clang --version
clang version 15.0.0 (https://github.com/llvm/llvm-project.git a9d68a5524dea113cace5983697786599cbdce9a)
Target: x86_64-unknown-linux-gnu

$ cat pr.c
void foo(void);

void foo()
{
}
$ bin/clang -c pr.c -Wstrict-prototypes -fno-knr-functions
clang-14: warning: argument unused during compilation: '-fno-knr-functions' [-Wunused-command-line-argument]
pr.c:3:9: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
void foo()

^
 void

1 warning generated.

It works if -fno-knr-functions is passed with Xclang . Is it intentional that -fno-knr-functions is only a cc1 option? That makes it very hard for us to enable it.

$ bin/clang -c pr.c -Wstrict-prototypes -Xclang -fno-knr-functions (no warnings)

manojgupta added a comment.EditedApr 30 2022, 9:30 AM

Following behavior is also surprising:

-Werror -Wimplicit-function-declaration does not rep-promote it to an error either if I suppress it globally with -Wno-error=implicit-function-declaration.

$ bin/clang -c tent.c -Wno-error=implicit-function-declaration -Werror -Wimplicit-function-declaration
tent.c:3:2: warning: call to undeclared function 'tgetent'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
tgetent(0,0);
^
1 warning generated.

This is breaking a lot of upstream configure scripts. I have had problems with less/sed/zip etc. They fail so early that I do not even know how many more packages are broken which is going to be very hard and cumbersome to fix :(..

Tried locally but I still see the warning with -fno-knr-functions. It also says that the argument is unused.

bin/clang --version
clang version 15.0.0 (https://github.com/llvm/llvm-project.git a9d68a5524dea113cace5983697786599cbdce9a)
Target: x86_64-unknown-linux-gnu

$ cat pr.c
void foo(void);

void foo()
{
}
$ bin/clang -c pr.c -Wstrict-prototypes -fno-knr-functions
clang-14: warning: argument unused during compilation: '-fno-knr-functions' [-Wunused-command-line-argument]
pr.c:3:9: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
void foo()

^
 void

1 warning generated.

It works if -fno-knr-functions is passed with Xclang . Is it intentional that -fno-knr-functions is only a cc1 option? That makes it very hard for us to enable it.

$ bin/clang -c pr.c -Wstrict-prototypes -Xclang -fno-knr-functions (no warnings)

No, that's not at all intentional -- it should be exposed as a driver flag. I can reproduce the issue locally and will fix this today (it's very strange because the option is listed as a CoreOption should it should be exposed through the driver). I'm very sorry for the trouble, but thank you for catching this!

Following behavior is also surprising:

-Werror -Wimplicit-function-declaration does not rep-promote it to an error either if I suppress it globally with -Wno-error=implicit-function-declaration.

On its face, I agree that it's surprising, but that's the general behavior of -Werror when warning flags default to an error, and is not specific to the changes here: https://godbolt.org/z/ronq687cj

Tried locally but I still see the warning with -fno-knr-functions. It also says that the argument is unused.

bin/clang --version
clang version 15.0.0 (https://github.com/llvm/llvm-project.git a9d68a5524dea113cace5983697786599cbdce9a)
Target: x86_64-unknown-linux-gnu

$ cat pr.c
void foo(void);

void foo()
{
}
$ bin/clang -c pr.c -Wstrict-prototypes -fno-knr-functions
clang-14: warning: argument unused during compilation: '-fno-knr-functions' [-Wunused-command-line-argument]
pr.c:3:9: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
void foo()

^
 void

1 warning generated.

It works if -fno-knr-functions is passed with Xclang . Is it intentional that -fno-knr-functions is only a cc1 option? That makes it very hard for us to enable it.

$ bin/clang -c pr.c -Wstrict-prototypes -Xclang -fno-knr-functions (no warnings)

No, that's not at all intentional -- it should be exposed as a driver flag. I can reproduce the issue locally and will fix this today (it's very strange because the option is listed as a CoreOption should it should be exposed through the driver). I'm very sorry for the trouble, but thank you for catching this!

Sheesh, I added the driver tests to make sure we don't accept a negated version of the flag, but I didn't add the test to validate that the driver accepted the flag, which is why I didn't catch this before. I've corrected the problem and added test coverage in 786954db06ab253dbd62d059036e06f6bbd9223c. Thanks for letting me know about the issue!

lenary removed a subscriber: lenary.May 3 2022, 12:29 AM
dexonsmith added inline comments.
clang/test/Sema/warn-strict-prototypes.m
20

This is a definition, so the compiler knows that there are no parameters. Why would we warn here? Reading https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18 it looks to me like an example of what @rnk was referring to, about churning code to add (void) and then return back to () later.

(cc: @steven_wu and @rjmccall as well)

dexonsmith added inline comments.May 12 2022, 2:31 PM
clang/test/Sema/warn-strict-prototypes.m
20

Specifically, ^void() { /* anything */} is the definition of a block with zero parameters. Maybe pedantically it's lacking a prototype, but the compiler knows (since this is the definition) how many parameters there are.

(Same goes for void() { /* anything */ } at global scope; is that triggering -Wstrict-prototypes now too?)

For additional context to my questions above, even though open source clang hasn't been using -Wstrict-prototypes, Xcode has had it on-by-default in new projects since sometime in 2017, with project modernizations to turn it on for old projects.

Warning on block and function definitions such as ^(){} and void f1() {}, which are pedantically lacking a prototype but the compiler knows there are exactly zero parameters, would be super noisy for users.

Unless I read the RFC too quickly, it doesn't look like it's adding any value, since these aren't going to change meaning. Is it possible to revert this part of the change?

efriedma added inline comments.
clang/test/Sema/warn-strict-prototypes.m
20

To be clear, you're asking specifically about the following two cases where the behavior with -Wstrict-prototypes changed, right? I don't think this was really discussed in the RFC.

// Block
void (^f1)(void) = ^(){};

// Function definition
void f(void);
void f(){}

Note that the behavior of the following did not change:

void (^f1)(void) = ^{}; // no warning
void f2(){} // warning since clang 11.
dexonsmith added inline comments.May 12 2022, 4:07 PM
clang/test/Sema/warn-strict-prototypes.m
20

Right, specifically those two cases that changed. Thanks for clarifying.

For additional context to my questions above, even though open source clang hasn't been using -Wstrict-prototypes, Xcode has had it on-by-default in new projects since sometime in 2017, with project modernizations to turn it on for old projects.

Thanks, that's very good to know! And also, thank you for raising the questions here, I appreciate the discussion.

Warning on block and function definitions such as ^(){} and void f1() {}, which are pedantically lacking a prototype but the compiler knows there are exactly zero parameters, would be super noisy for users.

Unless I read the RFC too quickly, it doesn't look like it's adding any value, since these aren't going to change meaning. Is it possible to revert this part of the change?

-Wstrict-prototypes is now a pedantic deprecation warning that fires any time you form a function type which has no prototype, which was discussed in the RFC (https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/38?u=aaronballman):

Change -Wstrict-prototypes to diagnose functions without a prototype that don’t change behavior in C2x, it remains off-by-default but is automatically enabled by -pedantic as it’s still warning the user about a deprecation.

However, I think the blocks behavior is a bug based on this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728 and I'd be more than happy to fix that, as I'm not checking that condition here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579 which seems like a pretty obvious thing to be checking for before warning about not having a strict prototype.

But I'm pretty insistent on warning about the other case as it does use functions without a prototype and we need *some* blanket warning for use of a deprecated feature for folks who want to be strictly conforming. It sounds like Apple may want to no longer enable -Wstrict-prototypes by default as it's shifted to be a more pedantic warning than it was before -- would that be a viable option for you (can you use project modernizations to turn it back off for old projects)?

However, I think the blocks behavior is a bug based on this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728 and I'd be more than happy to fix that, as I'm not checking that condition here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579 which seems like a pretty obvious thing to be checking for before warning about not having a strict prototype.

I fixed this false positive bug in 4be105c98a9c7e083cd878ee1751e11160b97b4a, so blocks (and OpenCL) behavior should now be improved.

dexonsmith added a subscriber: arphaman.EditedMay 13 2022, 7:22 AM

However, I think the blocks behavior is a bug based on this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728 and I'd be more than happy to fix that, as I'm not checking that condition here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579 which seems like a pretty obvious thing to be checking for before warning about not having a strict prototype.

I fixed this false positive bug in 4be105c98a9c7e083cd878ee1751e11160b97b4a, so blocks (and OpenCL) behavior should now be improved.

Thanks! I think that's the bigger issue of the two. @steven_wu or @arphaman will have more context though.

But I'm pretty insistent on warning about the other case as it does use functions without a prototype and we need *some* blanket warning for use of a deprecated feature for folks who want to be strictly conforming. It sounds like Apple may want to no longer enable -Wstrict-prototypes by default as it's shifted to be a more pedantic warning than it was before -- would that be a viable option for you (can you use project modernizations to turn it back off for old projects)?

[EDIT: trimmed the quote to what I was intending to reply to!]

Sure, I'm all for adding a new warning for users that want a pedantic warning. Can it be put behind a separate flag, such as -Wstrict-prototypes-pedantic, which isn't triggered by -Wstrict-prototypes?

Previously, -Wstrict-prototypes was useful for preventing actual bugs in code.

The warnings for this case aren't great:

int foo();

int
foo(int arg) {
  return 5;
}

results in:

/tmp/test.c:1:5: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
int foo();
    ^
        void
/tmp/test.c:4:1: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
foo(int arg) {
^

Two warnings for the problem, instead of a warning and a note, plus, the fix-it hint is incorrect, which the compiler should know, since we see that there are arguments expected.

I'd expect more like:

/tmp/test.c:1:5: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
int foo();
/tmp/test.c:4:1: note: subsequent declaration specifies arguments.
foo(int arg) {
    ^

Previously, -Wstrict-prototypes was useful for preventing actual bugs in code.

For example, it's important to have a warning that catches code like this:

void f1(void (^block)());

void f2(void) {
  f1(^(int x) { /* do something with x */ });
}

without triggering in cases that are pedantic.

(It also seems unfortunate to regress the false positive rate of this diagnostic before -fstrict-prototypes is available.)

Sure, I'm all for adding a new warning for users that want a pedantic warning. Can it be put behind a separate flag, such as -Wstrict-prototypes-pedantic, which isn't triggered by -Wstrict-prototypes?

Previously, -Wstrict-prototypes was useful for preventing actual bugs in code.

Doing that would then make -Wstrict-prototypes effectively a no-op (it would still control -Wdeprecated-non-prototype I suppose?), but there were also people who enabled -Wstrict-prototypes because they wanted the pedantic aspects of the warning in cases where it was firing, and those folks would then be losing warning coverage without knowing it. For example:

void f(){}

In prior versions of Clang with -Wstrict-prototypes this would issue a this old-style function definition is not preceded by a prototype diagnostic, but would now be silenced entirely unless the user knew to turn on a different flag.

The warnings for this case aren't great:

int foo();

int
foo(int arg) {
  return 5;
}

Yeah, that's not ideal, I'm looking into it to see if I can improve that scenario or not. Thankfully, IMO, such code is likely to be extremely rare. Most people writing that declaration either expect an arbitrary number of args (they really don't want the prototype) and so the definition is extremely suspect that it only accepts one, or they wrote the declaration assuming it would accept *no* args (the C++ behavior) and then the definition becomes flat-out a bug. Do you have evidence this code pattern appears with some frequency?

Sure, I'm all for adding a new warning for users that want a pedantic warning. Can it be put behind a separate flag, such as -Wstrict-prototypes-pedantic, which isn't triggered by -Wstrict-prototypes?

Previously, -Wstrict-prototypes was useful for preventing actual bugs in code.

Doing that would then make -Wstrict-prototypes effectively a no-op (it would still control -Wdeprecated-non-prototype I suppose?),

Is it necessary to make -Wstrict-prototypes weaker in order to move the newer more pedantic cases to a different name?

but there were also people who enabled -Wstrict-prototypes because they wanted the pedantic aspects of the warning in cases where it was firing, and those folks would then be losing warning coverage without knowing it. For example:

void f(){}

In prior versions of Clang with -Wstrict-prototypes this would issue a this old-style function definition is not preceded by a prototype diagnostic, but would now be silenced entirely unless the user knew to turn on a different flag.

Oh, I thought that would catch bugs like this:

void longfunctionname(){}
void longfunctionnames(int x){ /* do something with x */ }

void g(void) {
  longfunctionname(7); // oops, meant to call longfunctionnames().
}

but if it's entirely pedantic (if the call to longfunctionname(7) would fail to compile) then I agree it'd be better to silence it unless someone asks for -pedantic.

Previously, -Wstrict-prototypes was useful for preventing actual bugs in code.

For example, it's important to have a warning that catches code like this:

void f1(void (^block)());

void f2(void) {
  f1(^(int x) { /* do something with x */ });
}

without triggering in cases that are pedantic.

(It also seems unfortunate to regress the false positive rate of this diagnostic before -fstrict-prototypes is available.)

-fno-knr-functions is available already today in trunk, so I'm not certain I understand your concern there (or were you unaware we changed the flag name perhaps?).

Your code example is interesting though as I would expect that to trigger -Wdeprecated-non-prototype as well as -Wstrict-prototypes. I'd expect the pedantic warning to complain about the block declaration because it specifies no prototype, and I'd expect the changes behavior warning because that code will break in C2x.

Is it necessary to make -Wstrict-prototypes weaker in order to move the newer more pedantic cases to a different name?

This diagnostic is *extremely* difficult for a whole host of reasons. 1) we need to diagnose function *types* without a prototype, but you form a function type when declaring or defining a function in addition to just using the type in a typedef or a variable declaration. 2) we need to handle function definitions differently; that's the only point at which we know whether the user is defining a function with an identifier list which will change behavior in C2x, 3) we need to handle function declaration merging (which happens for definitions as well as redeclarations). So there's this very convoluted dance where some cases are handled in Sema::GetFullTypeForDeclarator() (this is the only place we trigger the pedantic warning from), some are handled in Sema::ActOnFinishFunctionBody(), and some are handled in Sema::MergeFunctionDecl(). Trying to weaken -Wstrict-prototypes specifically means having to make the decision at the time when the only information we have is what we can glean from the declarator.

Oh, I thought that would catch bugs like this:

void longfunctionname(){}
void longfunctionnames(int x){ /* do something with x */ }

void g(void) {
  longfunctionname(7); // oops, meant to call longfunctionnames().
}

but if it's entirely pedantic (if the call to longfunctionname(7) would fail to compile) then I agree it'd be better to silence it unless someone asks for -pedantic.

The call to longfunctionname(7) would be rejected in C2x and accepted in earlier modes, which is why it gets flagged warning: passing arguments to 'longfunctionname' without a prototype is deprecated in all versions of C and is not supported in C2x. It's worth noting that -Wstrict-prototypes is now in -pedantic explicitly (it shifted from being a Warning to an Extension because it is now a pedantic warning).

The warnings for this case aren't great:

int foo();

int
foo(int arg) {
  return 5;
}

Yeah, that's not ideal, I'm looking into it to see if I can improve that scenario or not.

There's not much to be done about the strict prototype warning for the declaration; the issue is that we need to give the strict prototypes warning when forming the function *type* for the declaration, which is long before we have any idea there's a subsequent definition (also, because this is for forming the type, we don't know what declarations, if any, may be related, so there's no real way to improve the fix-it behavior). However, I think the second warning is just bad wording -- we know we have a function definition with a prototype for this particular diagnostic. However, there's the redeclaration case to consider at the same time. So here's the full test and the best behavior that I can come up with so far:

void foo();
void foo(int arg);

void bar();
void bar(int arg) {}

With -Wstrict-prototypes enabled:

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only -Wstrict-prototypes "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c"
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:9: warning: a function declaration without a prototype
      is deprecated in all versions of C [-Wstrict-prototypes]
void foo();
        ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: this function declaration with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void foo(int arg);
     ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:9: warning: a function declaration without a prototype
      is deprecated in all versions of C [-Wstrict-prototypes]
void bar();
        ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: this function definition with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void bar(int arg) {}
     ^
4 warnings generated.

With -Wstrict-prototypes disabled:

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c"
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:6: warning: a function declaration without a prototype
      is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
void foo();
     ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: this function declaration with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void foo(int arg);
     ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:6: warning: a function declaration without a prototype
      is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
void bar();
     ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: this function definition with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void bar(int arg) {}
     ^
4 warnings generated.

It's not ideal, but it's about the most workable solution I can come up with that doesn't regress far more important cases. It'd be nice if we had a note instead of leaning on the previous warning like we're doing, but I still claim this is defensible given how rare the situation is that you'd declare without a prototype and later redeclare (perhaps through a definition) with a non-void prototype. Note (it's easy to miss with the wall of text), the difference between the two runs is that when strict prototypes are enabled, we issue the strict prototype warning on the first declaration and when strict prototypes are disabled, we issue the "is not supported in C2x" diagnostic on the first declaration -- but in either case the intention is to alert the user to which previous declaration has no prototype for the subsequent declaration/definition that caused the issue.

this function declaration|definition with a prototype will change behavior in C2x because of a previous declaration with no prototype is the new diagnostic wording I've got so far, and I'm not strongly tied to it, if you have suggestions to improve it. I would also be happy with this function declaration|definition with a prototype is not supported in C2x because of a previous declaration with no prototype or something along those lines.

void f1(void (^block)());

void f2(void) {
  f1(^(int x) { /* do something with x */ });
}

Your code example is interesting though as I would expect that to trigger -Wdeprecated-non-prototype as well as -Wstrict-prototypes. I'd expect the pedantic warning to complain about the block declaration because it specifies no prototype, and I'd expect the changes behavior warning because that code will break in C2x.

I think the current behavior today makes sense but we should see if we can improve it to make *more* sense. With -Wstrict-prototypes, we should complain about the block without a prototype, but the use at the call site is not declaring a conflicting declaration nor is it a call to a function without a prototype but passes arguments (both of those are -Wdeprecated-non-prototype warnings). Instead, it's a new kind of situation that we may want to consider adding additional coverage for under -Wdeprecated-non-prototype based on the same reasoning we used for diagnosing calls to a function without a prototype but pass arguments. This is not specific to blocks, consider:

void func(void (*fp)());
void do_it(int i);

int main(void) {
  func(do_it); // It'd be nice to diagnose this too, but we don't today
}

I think the current behavior today makes sense but we should see if we can improve it to make *more* sense. With -Wstrict-prototypes, we should complain about the block without a prototype, but the use at the call site is not declaring a conflicting declaration nor is it a call to a function without a prototype but passes arguments (both of those are -Wdeprecated-non-prototype warnings). Instead, it's a new kind of situation that we may want to consider adding additional coverage for under -Wdeprecated-non-prototype based on the same reasoning we used for diagnosing calls to a function without a prototype but pass arguments. This is not specific to blocks, consider:

void func(void (*fp)());
void do_it(int i);

int main(void) {
  func(do_it); // It'd be nice to diagnose this too, but we don't today
}

Mostly as a note to myself (but others can definitely chime in if they think I'm wrong here), I *think* the correct way to handle both blocks and function pointers is to add new AssignConvertType enumerators for IncompatibleC2xFunctionPointer and IncompatibleC2xBlockPointer to represent an assignment conversion that is compatible today but will be incompatible in C2x. Then functions like checkPointerTypesForAssignment() and checkBlockPointerTypesForAssignment() can return the new value, which DiagnoseAssignmentResult() can use to determine whether to diagnose a potential behavior change or not.

The warnings for this case aren't great:

int foo();

int
foo(int arg) {
  return 5;
}

Yeah, that's not ideal, I'm looking into it to see if I can improve that scenario or not.

There's not much to be done about the strict prototype warning for the declaration; the issue is that we need to give the strict prototypes warning when forming the function *type* for the declaration, which is long before we have any idea there's a subsequent definition (also, because this is for forming the type, we don't know what declarations, if any, may be related, so there's no real way to improve the fix-it behavior). However, I think the second warning is just bad wording -- we know we have a function definition with a prototype for this particular diagnostic. However, there's the redeclaration case to consider at the same time. So here's the full test and the best behavior that I can come up with so far:

void foo();
void foo(int arg);

void bar();
void bar(int arg) {}

With -Wstrict-prototypes enabled:

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only -Wstrict-prototypes "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c"
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:9: warning: a function declaration without a prototype
      is deprecated in all versions of C [-Wstrict-prototypes]
void foo();
        ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: this function declaration with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void foo(int arg);
     ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:9: warning: a function declaration without a prototype
      is deprecated in all versions of C [-Wstrict-prototypes]
void bar();
        ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: this function definition with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void bar(int arg) {}
     ^
4 warnings generated.

With -Wstrict-prototypes disabled:

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c"
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:6: warning: a function declaration without a prototype
      is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
void foo();
     ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: this function declaration with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void foo(int arg);
     ^
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:6: warning: a function declaration without a prototype
      is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
void bar();
     ^
         void
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: this function definition with a prototype
      will change behavior in C2x because of a previous declaration with no prototype [-Wdeprecated-non-prototype]
void bar(int arg) {}
     ^
4 warnings generated.

It's not ideal, but it's about the most workable solution I can come up with that doesn't regress far more important cases. It'd be nice if we had a note instead of leaning on the previous warning like we're doing, but I still claim this is defensible given how rare the situation is that you'd declare without a prototype and later redeclare (perhaps through a definition) with a non-void prototype. Note (it's easy to miss with the wall of text), the difference between the two runs is that when strict prototypes are enabled, we issue the strict prototype warning on the first declaration and when strict prototypes are disabled, we issue the "is not supported in C2x" diagnostic on the first declaration -- but in either case the intention is to alert the user to which previous declaration has no prototype for the subsequent declaration/definition that caused the issue.

this function declaration|definition with a prototype will change behavior in C2x because of a previous declaration with no prototype is the new diagnostic wording I've got so far, and I'm not strongly tied to it, if you have suggestions to improve it. I would also be happy with this function declaration|definition with a prototype is not supported in C2x because of a previous declaration with no prototype or something along those lines.

I put up a review at https://reviews.llvm.org/D125814 going in this direction and added you both as reviewers.

(It also seems unfortunate to regress the false positive rate of this diagnostic before -fstrict-prototypes is available.)

-fno-knr-functions is available already today in trunk, so I'm not certain I understand your concern there (or were you unaware we changed the flag name perhaps?).

That's right, I missed it. Thanks for pointing it out!