In D130510#3698415, @rtrieu wrote:

Can you add my earlier test case or something like it to SemaCXX/warn-bitwise-compare.cpp ?

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;  // only warn here
}

void run(int x) {
    foo<4, int>(8);
}

Adopted another approach working for the error caught. Kindly review this.

In D130510#3684740, @aaron.ballman wrote:

LGTM! I'll make some editorial corrections for grammar to the release note when I land.

updated with the small fixes and addition of release note

In D130510#3682902, @aaron.ballman wrote:

Thank you for this, I think this is good incremental progress and is almost ready to go. Just a few small nits, but also, can you also add a release note for the fix (be sure to mention which issue is being closed too).

Note, the precommit CI failures are unrelated to the changes in this patch.

updating the code suggestions and test cases.

also updating for the Xor bitwise operator

updating to more efficient

In D129048#3674582, @Mordante wrote:

In D129048#3674500, @Codesbyusman wrote:

updated the libcxx all files

FYI for libcxx you can ignore the red format CI step. So the libcxx build is green :-)

I had a look at your libc++ changes and they look good, thanks.
So LGTM for the libc++ part.

updated the libcxx all files

updated libcxx ready to check again

updating of the libcxx with the regex in the diagnostic messages, haven't test them yet locally

In D129048#3671657, @ldionne wrote:

In D129048#3671568, @aaron.ballman wrote:

FWIW, I've convinced myself that I agree with you here that the burden probably should have been on libc++ maintainers in this case. libc++ almost feels more like a downstream consumer of Clang in terms of testing needs, so I think when tests break because Clang diagnostic wording or behavior changes in a standards conforming way, libc++ folks should address it, but if Clang changes in a way that's not standards conforming, then Clang folks should address it. (Roughly speaking.)

Yes, exactly. It's always possible to write tests that depend on implementation details of another component in subtle ways, and the fact that such brittle tests exist doesn't create a responsibility on that component to avoid breaking those downstream users. Here, it's about Clang making a valid change and libc++ containing brittle tests, but it happens quite often where libc++ changes something valid and a downstream consumer is broken in some way. The LLVM revert culture has some benefits to keep things working in a post-commit CI world, however I've noticed that it also creates a lot of friction between projects and sometimes makes important work much more difficult to land than it should. For example, we're currently in the middle of D128146 where LLDB reverted an important patch for LLVM 15 because one of their tests broke. This is not something that comes up super often, but it's extremely disruptive and frustrating when it does, and I think it would be worth trying to address. Anyway, I'm digressing now, but I'll try to talk to a few people at the LLVM Dev Meeting to see if this is a shared concern and to think about potential solutions to address this.

That said, I absolutely think we all need to continue to collaborate closely with one another as best we can when issues arise, and I really appreciate the discussion on how we can do that!

Agreed.

For this particular issue, I'd like @Codesbyusman to continue to try to fix the libc++ testing issues (it's good experience), but if that takes significantly longer (say, more than 8 hours of his effort), perhaps @ldionne or someone else from libc++ will have a moment to step in to help?

Replacing static_assert by (static_assert|static assertion) should do the trick. See the patch attached to this comment, I think it should satisfy the CI @Codesbyusman.

static_assert.diff70 KBDownload

In D129048#3671665, @erichkeane wrote:

In D129048#3671657, @ldionne wrote:

In D129048#3671568, @aaron.ballman wrote:

FWIW, I've convinced myself that I agree with you here that the burden probably should have been on libc++ maintainers in this case. libc++ almost feels more like a downstream consumer of Clang in terms of testing needs, so I think when tests break because Clang diagnostic wording or behavior changes in a standards conforming way, libc++ folks should address it, but if Clang changes in a way that's not standards conforming, then Clang folks should address it. (Roughly speaking.)

Yes, exactly. It's always possible to write tests that depend on implementation details of another component in subtle ways, and the fact that such brittle tests exist doesn't create a responsibility on that component to avoid breaking those downstream users. Here, it's about Clang making a valid change and libc++ containing brittle tests, but it happens quite often where libc++ changes something valid and a downstream consumer is broken in some way. The LLVM revert culture has some benefits to keep things working in a post-commit CI world, however I've noticed that it also creates a lot of friction between projects and sometimes makes important work much more difficult to land than it should. For example, we're currently in the middle of D128146 where LLDB reverted an important patch for LLVM 15 because one of their tests broke. This is not something that comes up super often, but it's extremely disruptive and frustrating when it does, and I think it would be worth trying to address. Anyway, I'm digressing now, but I'll try to talk to a few people at the LLVM Dev Meeting to see if this is a shared concern and to think about potential solutions to address this.

That said, I absolutely think we all need to continue to collaborate closely with one another as best we can when issues arise, and I really appreciate the discussion on how we can do that!

Agreed.

For this particular issue, I'd like @Codesbyusman to continue to try to fix the libc++ testing issues (it's good experience), but if that takes significantly longer (say, more than 8 hours of his effort), perhaps @ldionne or someone else from libc++ will have a moment to step in to help?

Replacing static_assert by (static_assert|static assertion) should do the trick. See the patch attached to this comment, I think it should satisfy the CI @Codesbyusman.

static_assert.diff70 KBDownload

I just want to say: I really appreciate the insight you gave into the libc++ testing in this thread. From the CFE's perspective, our view of precommit is "it is basically always misconfigured or broken in some way", so knowing that you guys keep yours running well is nice to hear! We'll be more cognizant in the future.

I DO think there IS a sizable difference between patches like this one, and patches like my deferred-concepts (which, btw, the libc++ tests have been great for coming up with new tests... though it was a pain to figure out how to run them in the first place!). My patches DID deserve the reverts it received so far, so don't see Aaron's mention as an issue with that.

I appreciate your suggestion/patch file, that IS a much better regex than we'd come up with, and looks like it should work. So thank you! We'll commit that once @Codesbyusman can get it integrated into his patch (and we'll make sure CI passes too!).

In D129048#3671568, @aaron.ballman wrote:

In D129048#3670212, @ldionne wrote:

The libc++ CI runs pre-commit only, and it runs on all commits that touch something under libcxx/, libcxxabi/, runtimes/ and libunwind/. In particular, it won't trigger if you make changes to clang/ only -- that would create too much traffic and concretely it wouldn't help much, as we're rarely broken by Clang changes (this is an exception). Once the tests under libcxx/ were touched, the libc++ CI triggered, I found this build for instance: https://buildkite.com/llvm-project/libcxx-ci/builds/12210. I do think it boils down to familiarity -- while the Clang pre-commit CI is sometimes overlooked because it fails spuriously, the libc++ CI usually doesn't, so when it fails, there's usually a good reason. I do understand why someone mostly familiar with the Clang workflow could think they needed to be overlooked, though.

Thanks for the explanation!

FWIW, the specific reason why I overloooked it was because of running against old versions of Clang -- I saw the diagnostic was correct in the test and presumed that the old clang was a bot configuration issue. It definitely doesn't help that Clang's precommit CI tends to be spuriously broken quite often; it's trained some of us reviewers to be more dismissive of failing precommit CI than we should.

Concretely, libc++ supports the two latest releases of Clang, and so the CI tests against those. In fact, we run most of the tests using pre-installed Clangs since it allows us to bypass building Clang itself, which is a necessary optimization in the current setup. Regarding the tests themselves, I am able to find the following kind of output in the CI job that failed:

Yeah, which makes a lot of sense for libc++!

FAIL: llvm-libc++-shared.cfg.in :: std/numerics/numbers/illformed.verify.cpp (4388 of 7532)
[...]
error: 'error' diagnostics expected but not seen:
  File /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/numbers Line * (directive at /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/libcxx/test/std/numerics/numbers/illformed.verify.cpp:15): static assertion failed{{.*}}A program that instantiates a primary template of a mathematical constant variable template is ill-formed.
error: 'error' diagnostics seen but not expected:
  File /home/libcxx-builder/.buildkite-agent/builds/2b1c77dd9e90-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/numbers Line 83: static_assert failed due to requirement '__false<int>' "A program that instantiates a primary template of a mathematical constant variable template is ill-formed."
2 errors generated.

IMO this is reasonable output, in fact it's pretty much as good as we could have had. So I'm not taking any action item regarding improving the output of our CI, but I'm entirely open to suggestions if somebody has some.

I don't have a concrete suggestion for the CI output itself -- the failure was clear in retrospect, this was more a matter of not being familiar with the testing practices used.

In D129048#3669568, @aaron.ballman wrote:

At a high-level, I think it boils down to familiarity. If we can get the libc++ CI to run as part of precommit CI (assuming precommit CI can be made to run with reliable results, which is a bit of a question mark),

It is pretty reliable. However, I don't think it'll ever become reasonable to run the full libc++ CI on every Clang patch due to resource limitations.

The resource limitations are certainly a pragmatic thing for us to be concerned with, but I hope we're able to find some happy middle ground.

It would have also helped to catch the cause for the initial revert where everyone thought the patch was ready to land.

100% agreed, however this is blocked on the resource limitations mentioned above. I guess one thing we could do here is trigger a simple bootstrapping build of Clang and run the libc++ tests in a single configuration even on changes to clang/. That might catch most issues and perhaps that would be small enough to be scalable. I'll experiment with that after the release.

Thanks, that would be really beneficial. It's not just diagnostic wording changes that cause Clang developers problems with libc++ reverts; I've also seen C++20 work reverted because it broke libc++ without anyone in Clang knowing about it until after it landed (most recently, it's with the deferred concept checking work done by @erichkeane).

Another thing that would help would be to get the libc++ test bots into the LLVM lab (https://lab.llvm.org/buildbot/#/builders)

Libc++ CI is pre-commit, the lab is post-commit only AFAICT.

Correct, but right now we have no community testing for libc++ whatsoever from the perspective of Clang folks. We only find out about failures when libc++ folks start reverting or pointing them out.

I also don't know whether there's a way to bridge between BuildKite and BuildBot, and what that would look like. So while I share the desire to have a single place to look for all LLVM wide CI, I'm not sure it is possible given the variety of technologies used.

I don't know enough about the technology to know if it's easy or hard, unfortunately.

and ensure that they're sending failure emails to everyone on the blame list including people who land a patch on behalf of others.

100% agreed here -- this is one problem with our current setup.

It looks like we have one builder for libc++, but it doesn't appear to be working recently: https://lab.llvm.org/buildbot/#/builders/156.

Yeah, this one needs to be removed, it's a remnant of when we used to have libc++ CI on BuildBot.

Okay, so to summarize:

1. I'll experiment with a simple job that runs the libc++ pre-commit CI on every patch to Clang. We'll see if that scales.

Thanks, that would be awesome if it works out!

2. I'll look into sending blame emails from the ongoing (every 3h) job that runs the whole libc++ CI. This is basically equivalent to the buildbot functionality and it's arguably something that we're missing here. I have no idea how to do that, though, but it must be possible.

Thank you!!

3. Usually, for these types of failures, we actually notice and fix it on our end without requesting a revert. For this kind of change, my opinion is that the burden of fixing the code should have been on us, kind of like when the LLDB data formatters break because we made a totally legal change in libc++. Nobody jumped in to fix it on the libc++ side this time because (I assume) we're racing for the release, but normally I or someone else would have chimed in to fix this without annoying Clang.

FWIW, I've convinced myself that I agree with you here that the burden probably should have been on libc++ maintainers in this case. libc++ almost feels more like a downstream consumer of Clang in terms of testing needs, so I think when tests break because Clang diagnostic wording or behavior changes in a standards conforming way, libc++ folks should address it, but if Clang changes in a way that's not standards conforming, then Clang folks should address it. (Roughly speaking.) That said, I absolutely think we all need to continue to collaborate closely with one another as best we can when issues arise, and I really appreciate the discussion on how we can do that!

For this particular issue, I'd like @Codesbyusman to continue to try to fix the libc++ testing issues (it's good experience), but if that takes significantly longer (say, more than 8 hours of his effort), perhaps @ldionne or someone else from libc++ will have a moment to step in to help? (For reference, Usman is a Google Summer of Code mentee and this was his first patch for Clang. I think this has been a really good way for him to see how open source collaborations work in practice but I also don't want him to get bogged down too much if I can help it. Normally I'd step in to help directly, but I've been in WG14 meetings all week and so I am pretty swamped at the moment.)

In D129048#3662972, @xgupta wrote:

In D129048#3662634, @Codesbyusman wrote:

the libcxx updates but not tested. working on them

I just run the check-cxx on local system. It seems to pass all testcases after applying the current patch.

Testing Time: 2147.75s

Unsupported      :  307
Passed           : 7184
Expectedly Failed:   41

the libcxx updates but not tested. working on them

*ERR_CLOSED: <differential.updaterevision> This revision has already been
closed*

ok. I will take care of this next time. Thank you.

In D129048#3648964, @erichkeane wrote:

Please format your patch with clang-format. See the command at the end of this: https://clang.llvm.org/docs/ClangFormat.html

git diff -U0 --color=never | clang-format-diff.py -i -p1

run the clang-format

Done.

updating new lines

updated the format issues

updated the comment styles suggested

the tests for the coverage of changes being made for the static assertion

In D129048#3647918, @aaron.ballman wrote:

I think this is getting pretty close! I did have some style guideline changes, and at least one test addition I'd like to see though. Also, it looks like precommit CI caught a test that still needs to be updated:

FAIL: Clang :: SemaCXX/cxx98-compat.cpp (16448 of 68726)

******************** TEST 'Clang :: SemaCXX/cxx98-compat.cpp' FAILED ********************

Script:

--

: 'RUN: at line 1';   /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -Wc++98-compat -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp

: 'RUN: at line 2';   /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -fsyntax-only -std=c++14 -Wc++98-compat -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp -DCXX14COMPAT

: 'RUN: at line 3';   /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -fsyntax-only -std=c++17 -Wc++98-compat -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp -DCXX14COMPAT -DCXX17COMPAT

--

Exit Code: 1



Command Output (stderr):

--

error: 'warning' diagnostics expected but not seen: 

  File /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp Line 155: static_assert declarations are incompatible with C++98

error: 'warning' diagnostics seen but not expected: 

  File /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp Line 155: 'static_assert' declarations are incompatible with C++98

2 errors generated.



--



********************

updated the styles and other small things

the parser updates maked for the static asserion

Yes I am looking into the DiagnosticParseKinds.td. Kindly also look another error encountered in testing while updating the test. Although not having in my system but this is giving .
Look here

Yes I am looking into the DiagnosticParseKinds.td

updating the test files for the last update

updating some to the back strings becaue the orginal were more better

new changes as required -- final

update

all

all changes in one patch revision

All changes in one place

updating

Updating the files as required

Ok ... I am working on the tests I will update it