due to this change we have a enourmous number of new warnings, on the other hand -Wunused-variable is a valuable warning. I am not sure what is the policy and best practices for warnings but maybe there is a possiblity to make a transition period for enabling this type of warning separetely and to allow updating existing code?

there is a number of unused vaiables in conditional loops that are firing now, I have submitted https://github.com/llvm/llvm-project/commit/74f4daef0412be33002bd4e24a30cb47d0187ecf but I suspect it's just a start.
How did you checked the project code for that?

as for project specific CIs - maybe, it's debatable. I understand that some projects might want to go extra mile, it's OK. Yet running "configure all + ninja check-all" should work and provide good coverage without much thoughts and tweaks

also, why do we need lld/bar etc.?

I assume you've tried to run it, right? Otherwise I can try setting up PRs.

for the record it was fixed in dae52dd07d633673fe3ff2a6b5e5c71f9affc470 and cd69b0cc88bccf80e11c6d95255139b5db62b17c

I also see failures of under asan caused by this commit

rebase

fix for 9be8219f60e1bbddaebdb271a55ecfb867078899

.

formatting

nice

thanks @jplehr

I also see build timeouts on my builds

hi @matts1, FYI "bazel build" tag was created so that interested people will run bazel build as part of their pre-merge check, not in reviewing changes to bazel. It's also quite large.

Hi! Just FYI "bazel build" tag was created so that interested people will run bazel build as part of their pre-merge check, not in reviewing changes to bazel. It's also quite large.

In D140990#4298798, @goncharov wrote:

Heads up: this seems to make compilation non-determenistic (I am still trying to confirm).

Heads up: this seems to make compilation non-determenistic (I am still trying to confirm).

In D143447#4115063, @mstorsjo wrote:

In D143447#4110327, @ldionne wrote:

In D143447#4110194, @aaron.ballman wrote:

It looks like this is the VCRuntime used by the bots being too old and not supporting constexpr. I think the right thing to do is to UNSUPPORT the test in that case.

@goncharov @mstorsjo Would it be possible to update the VCRuntime on the Windows hosts?

I checked on a test setup, and the test was failing with VS 2022 17.1, but started after updating it to VS 2022 17.4. And as it's about constexpr, I guess it shouldn't have any runtime aspect (it should run fine even with an older version of the vcruntime DLLs).

I guess it should be possible to update the CI images. As building the docker images usually is a bit messy (it often ends up requiring fixing some dependency which no longer installs cleanly) I guess @goncharov doesn't want to do it all the time - but we should probably do it once Clang 16.0 is released stable so we can include that. I didn't prod him for updating them with Clang 15.0 (so I would believe that they're still running 14.0) since I didn't have any concrete detail that would benefit from that upgrade at the point.

I found that most recent version e94619b955104841cc2a4a6febe4025ee140194e miscompiles (?): https://godbolt.org/z/fffnjGa4q .
Will likely revert soon cc @asbirlea

In D135220#3840354, @benlangmuir wrote:

Okay, I was able to reproduce by symlinking all the 0-byte header files to header0 (any choice probably works). The behaviour is deterministic before and after my change.

This was only passing by luck in this setup, because it was relying on mutation of FileEntry->LastRef which mutates the path of header1 to header3. We do not actually track the difference in filename here, it just happened to match the behaviour without symlinks for this case because of the mutation. Note: the mutation is not triggered specifically by the include, it's anything that looks up that path in the file manager, so there is no guarantee that you would get header3 if something triggered accessing the filename header1 again at the wrong time.

I think my change is fine here and we should just update the test files so they will not be accidentally linked. If someone wants to work on tracking the filenames independently for each include that would be fine, but as long as we only have one name it should be the first one not the "one whose path was most recently accessed in FileManager".

@goncharov Was this the only test effected? If so, here's a fix: https://reviews.llvm.org/D135373

thank you!

In D135220#3840177, @dexonsmith wrote:

In D135220#3839671, @goncharov wrote:

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

FWIW, I agree with Ben that this change seems like it should improve consistency for symlinked content. Knowing the failure mode / having reproduction steps would be helpful to track down your corner case!

(There are many subtle interactions around this stuff in clang so it’s hard to make forward progress.)

That change might be problematic for content addressing storages. E.g. clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as clang/test/Driver/header{0,1,3,4}.h are all identical and can be symlinked. cc @dexonsmith

as a headsup: I see this change causes failures in Halide https://github.com/halide/Halide/blob/main/test/correctness/div_round_to_zero.cpp on some architectures

FYI this caused a test breakage at lldb-aarch64-ubuntu, https://lab.llvm.org/buildbot/#/builders/96/builds/25128. I have reverted this in fe6db8d03ff16a65f57af24d2cb04f489e2e9b0c

(landed as 4856bc16c651ec151b16e3d97704901ab6a1c383)

updated patch prefix