In D120244#3540746, @aaron.ballman wrote:

I'll roll back the #warning use and report back when that's done.

In D120244#3540703, @ldionne wrote:

In D120244#3540598, @aaron.ballman wrote:

The system header including a header that's explicitly deprecated is dubious practice, but that does still require some amount of timing coordination.

I agree, and I'll be filing bug reports against system headers within my control that use <stdbool.h>. That is unfortunately not the majority of headers, though.

In D120244#3540598, @aaron.ballman wrote:

We could maybe extend #pragma clang deprecate to deprecate inclusion of the current file so that it acts sort of like a [[deprecated]] attribute that triggers on inclusion using typical diagnostics instead of #warning.

In D120244#3539852, @ldionne wrote:

In D120244#3538660, @aaron.ballman wrote:

In D120244#3538380, @ldionne wrote:

We've started having several internal user complaints because their system headers are suddenly triggering a warning for using <stdbool.h>. This seems to be caused by the fact that #warning is surfaced to users even when the #warning is present in a system header (which makes sense, because otherwise there would be no way to issue a #warning from a system header). This ends up causing problems because users have no way to suppress the warning in the system headers they use without also disabling deprecation warnings in their own code. Is this intended? Instead, it seems to me like what we'd want is some way to mark the header as deprecated such that Clang will not flag uses of <stdbool.h> from within system headers, but will flag them from user code. This would be consistent with how the deprecated attributes work for classes and functions.

Thoughts?

This is what _CLANG_DISABLE_CRT_DEPRECATION_WARNINGS is for. We documented it here: https://clang.llvm.org/docs/UsersManual.html#controlling-deprecation-diagnostics-in-clang-provided-c-runtime-headers but the basic idea is, if that's defined before including the header, we don't issue those warnings. Does that suffice?

Well, the problem is that a user that is using some system header like Foundation.h (which includes <stdbool.h>) will be forced to define _CLANG_DISABLE_CRT_DEPRECATION_WARNINGS, which has two drawbacks:

1. It's annoying to them because they never used <stdbool.h> in their own code
2. It turns off deprecation warnings in their own code as well

In D102122#3538668, @dblaikie wrote:

Sorry, with all the layers in the previous messages I'm not sure what the summary is. Sounds like the summary is "this is OK to continue work in the direction of supporting [[clang::warn_unused_result]] (only that spelling) for typedefs in C and C++ (for compatibility with C/so that C headers, like LLVM C's APIs can still be parsed as C++), while considering some of the complications raised in the WG21 discussion"?

In D120244#3538380, @ldionne wrote:

We've started having several internal user complaints because their system headers are suddenly triggering a warning for using <stdbool.h>. This seems to be caused by the fact that #warning is surfaced to users even when the #warning is present in a system header (which makes sense, because otherwise there would be no way to issue a #warning from a system header). This ends up causing problems because users have no way to suppress the warning in the system headers they use without also disabling deprecation warnings in their own code. Is this intended? Instead, it seems to me like what we'd want is some way to mark the header as deprecated such that Clang will not flag uses of <stdbool.h> from within system headers, but will flag them from user code. This would be consistent with how the deprecated attributes work for classes and functions.

Thoughts?

In D123235#3537820, @aaron.ballman wrote:

In D123235#3537636, @aaron.ballman wrote:

Please revert the changes while investigating how to resolve the crashes though.

I just noticed that you needed someone to commit on your behalf for this, so I went ahead and did the revert myself in 69da3b6aead2e7a18a2578aad661d6d36b8d30cf.

I think this has sat for long enough and my LGTM will have to suffice. :-)

In D123235#3537636, @aaron.ballman wrote:

Please revert the changes while investigating how to resolve the crashes though.

Please add more details to the summary and remove the rdar link (nobody outside of Apple can access that anyway). Also, this should have a release note for the bug fix.

LGTM (minor nit with release notes, take it or leave it at your discretion).

Adding the language WG as a reviewer in case others have opinions.

In D123235#3537580, @koops wrote:

I am not sure why it is indicating an uninitialized variable at that point. The code at " llvm-project/clang/lib/Sema/SemaOpenMP.cpp:12060 " is as below:

In D125814#3535452, @jyknight wrote:

Good improvement, but an additional change to wording for just the zero-arg non-prototype function declaration case could help a lot. The fact that zero-arg it's only being warned about because of the "...because of" note isn't particularly clear -- especially when the "because of" isn't emitted.

So, suggestion for zero-arg-specific warning text:

Update based on review feedback.

In D126324#3537373, @wanders wrote:

In D126324#3536785, @aaron.ballman wrote:

The changes so far look sensible, but I think we should add some more tests for a few situations. 1) Using a const weak symbol as a valid initializer should be diagnosed (with a warning? with an error?) so users are alerted to the behavioral quirks. 2) Using a const weak symbol in a constant expression context should probably be an error, right? e.g.,

I do think we have already diagnose/fail on all the relevant cases here. And have tests for them, like

extern const int weak_int __attribute__((weak));
const int weak_int = 42;
int weak_int_test = weak_int; // expected-error {{not a compile-time constant}}

in clang/test/Sema/const-eval.c

But I'll have another look to make sure we have proper coverage.

In D126324#3537282, @jyknight wrote:

In D126324#3536785, @aaron.ballman wrote:

The changes so far look sensible, but I think we should add some more tests for a few situations. 1) Using a const weak symbol as a valid initializer should be diagnosed (with a warning? with an error?) so users are alerted to the behavioral quirks.

I don't feel this is really necessary.

Thank you for the ping, this fell entirely off my radar!

The changes so far look sensible, but I think we should add some more tests for a few situations. 1) Using a const weak symbol as a valid initializer should be diagnosed (with a warning? with an error?) so users are alerted to the behavioral quirks. 2) Using a const weak symbol in a constant expression context should probably be an error, right? e.g.,

const int hmm __attribute__((weak)) = 12;
int erp = hmm; // Error in C, dynamic init in C++?

Thank you for your work on this, I generally like this new approach and think we're heading in the right direction.

LGTM -- I checked as well and these changes are NFC. Thanks!

LGTM, though this should have a release note for the fix.

LGTM, thank you for catching this!

Ping

LGTM aside from a testing request and a nit. Thanks for the fix!

In D125970#3531673, @JonChesterfield wrote:

In D125970#3531645, @aaron.ballman wrote:

In D125970#3527685, @JonChesterfield wrote:

If it was adding a calling convention, sure - caution warranted. There's no llvm change here though, an existing CC is exposed to C++. No change to the type system either.

This is adding a user-facing calling convention to Clang and it changes the type system as a result. For example, lambda function pointer conversion operators sometimes are generated for each calling convention so that you can form a function pointer of the correct type (this might not be impacted by your change here); there's a specific number of bits for representing the enumeration of calling conventions and this uses one of those bits, etc.

It slightly changes the type system of C++ code in that the calling convention was previously only available in opencl / openmp etc. I was under the impression that the compiler data representation cost of calling conventions was in LLVM and thus pre-paid for the calling convention this gives access to. There's the enum CallingConv which has gained a field, I didn't realise that was input into something of limited bitwidth.

In D125970#3527685, @JonChesterfield wrote:

If it was adding a calling convention, sure - caution warranted. There's no llvm change here though, an existing CC is exposed to C++. No change to the type system either.

LGTM, thank you!

Thanks for this! It generally LGTM (though my Python skills are not particularly awesome, so take it with a grain of salt). Just a few questions at this point.

In D126186#3530938, @njames93 wrote:

As a side point I'm not sure this change really follows what the rule is trying to enforce. The rule is about not using std::memset to reinitialise objects that aren't trivial. Having said that limiting it to only 0 does seem a little restrictive,

In rGdf46fb40557a14807dd508af32251ceb1cab8b86#1088254, @abrachet wrote:

We are seeing test failures see for example https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8813529725139260241/overview https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-mac-x64/b8813529727054945089/overview https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8813529725592813073/overview

In D125970#3527624, @aaron.ballman wrote:

The primary application is for more rapid debugging of the amdgpu backend by permuting a C or C++ test file instead of manually updating an IR file.

Given that this is adding a calling convention, which has significant impacts on our type system: is this use case important enough to steal a bit for this CC? This sounds *super* special case to me, but maybe it's a common need?

The primary application is for more rapid debugging of the amdgpu backend by permuting a C or C++ test file instead of manually updating an IR file.

In D123667#3524711, @aaron.ballman wrote:

In D123667#3524614, @tcanabrava wrote:

This affects all released versions of llvm14, and potentially llvm13 too (I got a crash today on llvm13 hitting this exact code).
yet, this is not backported to llvm14 on the current 14.x github branch. Please backport so this is released on the next tarball.

I filed https://github.com/llvm/llvm-project/issues/55585 to see if we can get this into the 14.x branch, thanks for the request!

LGTM, thank you!

In D125919#3523441, @rjmccall wrote:

We definitely don't want ObjC to differ here; thanks for the CC.

Fixes failed AST matching unit test and speculatively fixes a CodeGen test, all found via precommit CI testing.

Thank you for this! Is there a way to add test coverage for this change? Also, this should come with a release note as it's fixing a bug someone reported.

In D123667#3524614, @tcanabrava wrote:

This affects all released versions of llvm14, and potentially llvm13 too (I got a crash today on llvm13 hitting this exact code).
yet, this is not backported to llvm14 on the current 14.x github branch. Please backport so this is released on the next tarball.

Adding some Apple reviewers because this touches ObjC behavior (I would be very surprised if ObjC wanted different rules here than C though).

In D125259#3522198, @aaron.ballman wrote:

In D125259#3520118, @aaron.ballman wrote:

In D125259#3519947, @aaron.ballman wrote:

Oh wow, good catch! I'll correct this.

Oof, the plot thickens... the diagnostic is correct for some types, but is incorrect for others: https://godbolt.org/z/fahzx53W6. I also discovered a parsing issue in C++ where we get confused by elaborated type specifiers: https://godbolt.org/z/1ejxqd9ss.

https://reviews.llvm.org/D125882 addresses the diagnostic behavior for the unreachable warning.

In D125259#3520118, @aaron.ballman wrote:

In D125259#3519947, @aaron.ballman wrote:

Oh wow, good catch! I'll correct this.

Oof, the plot thickens... the diagnostic is correct for some types, but is incorrect for others: https://godbolt.org/z/fahzx53W6. I also discovered a parsing issue in C++ where we get confused by elaborated type specifiers: https://godbolt.org/z/1ejxqd9ss.

In D125259#3520544, @tahonermann wrote:

I'm questioning the benefit to supporting _Generic in C++ and starting to wonder if perhaps we should pull this extension back.

I imagine it may be used in shared system headers. E.g., implementations of tgmath.h that don't use the builtins.

Went with a more simple implementation approach and the diagnostic results seem to be a mild improvement in terms of wording and behavior.

In D125814#3520137, @jyknight wrote:

This confuses me.

Looking at behavior with default flags:

We won't emit a -Wdeprecated-non-prototype warning for int foo();, until we subsequently find int foo(int arg) { return 5; }.

In D125259#3519947, @aaron.ballman wrote:

Oh wow, good catch! I'll correct this.

In D125259#3519822, @aeubanks wrote:

$ cat /tmp/a.cc
typedef struct Test {
} Test;

void f() {
  const Test constVal;
  _Generic(constVal, const Test : 0);
}
$ ./build/rel/bin/clang -fsyntax-only  -x c++ /tmp/a.cc -Wall
/tmp/a.cc:6:28: warning: due to lvalue conversion of the controlling expression, association of type 'const Test' will never be selected because it is qualified [-Wunreachable-code-generic-assoc]
  _Generic(constVal, const Test : 0);
                           ^
/tmp/a.cc:6:35: warning: expression result unused [-Wunused-value]
  _Generic(constVal, const Test : 0);
                                  ^
2 warnings generated.
$ ./build/rel/bin/clang -fsyntax-only  -x c /tmp/a.cc -Wall
/tmp/a.cc:6:28: warning: due to lvalue conversion of the controlling expression, association of type 'const Test' (aka 'const struct Test') will never be selected because it is qualified [-Wunreachable-code-generic-assoc]
  _Generic(constVal, const Test : 0);
                           ^
/tmp/a.cc:6:12: error: controlling expression type 'Test' (aka 'struct Test') not compatible with any generic association type
  _Generic(constVal, const Test : 0);
           ^~~~~~~~
1 warning and 1 error generated.

the C++ case looks wrong, it's warning that const Test can't be selected then selects it

In D122895#3512314, @aaron.ballman wrote:

In D122895#3511682, @aaron.ballman wrote:

In D122895#3511632, @jyknight wrote:

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.

Btw, I'm in C standards meetings all week this week, so my reviews are going to be delayed by a bit, just as an FYI.

LGTM!

I know it's retroactive, but this also LGTM. Thank you for the fix!

In D124726#3516346, @nathanchance wrote:

I see an instance of this warning in the Linux kernel due to the "Now, for unknown directives inside a skipped conditional block, we diagnose the unknown directive as a warning if it is sufficiently similar to a directive specific to preprocessor conditional blocks" part of this change:

arch/x86/crypto/aesni-intel_asm.S:532:8: warning: invalid preprocessing directive, did you mean '#if'? [-Wunknown-directives]
                                        # in in order to perform
                                          ^
arch/x86/crypto/aesni-intel_asm.S:547:8: warning: invalid preprocessing directive, did you mean '#if'? [-Wunknown-directives]
                                        # in in order to perform
                                          ^
2 warnings generated.

It's not clear to me why existing facilities shouldn't be extended to cover this case rather than coming up with another feature testing macro. There's already plenty of confusion for users to decide between __has_feature and __has_extension, and now we're talking about adding a third choice to the mix.

LGTM, thank you for this!

Note, I'm in C standards committee meetings all week this week, so I expect I won't be doing many reviews this week and I'll be catching up as best I can starting next week.

In D122895#3512341, @aaron.ballman wrote:

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
}

In D122895#3511855, @aaron.ballman wrote:

In D122895#3511646, @dexonsmith wrote:

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.

In D122895#3511682, @aaron.ballman wrote:

In D122895#3511632, @jyknight wrote:

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.

In D122895#3511646, @dexonsmith wrote:

In D122895#3511611, @dexonsmith wrote:

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.)

In D122895#3511632, @jyknight wrote:

The warnings for this case aren't great:

int foo();

int
foo(int arg) {
  return 5;
}

In D122895#3511611, @dexonsmith wrote:

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.