aaron.ballman added a comment.

In D130689#3706377 <https://reviews.llvm.org/D130689#3706377>, @thieta wrote:
> In D130689#3706336 <https://reviews.llvm.org/D130689#3706336>, @aaron.ballman 
> wrote:
>
>> That's the only reason this hasn't been reverted already. Landing sweeping 
>> changes on a weekend is a good way to reduce the pain, but we really need to 
>> be sure someone watches the build lab and reacts when subsequent changes 
>> break everything like this.
>
> Agreed, I think we need to update the protocol for changing the C++ standard 
> in the future to account for more testing beforehand. I might push some 
> changes to the policy document when all this has settled down to see if we 
> can make sure it will be smoother the time we move to C++20. It's unfortunate 
> that some stuff broke considering we where running some bots before it was 
> merged and it didn't show any errors. And local windows builds for me have 
> been clean as well.

+1, thank you for thinking about how we can improve this process in the future! 
Given that C++17 adoption across compilers has been far better than C++20, I 
suspect the next time we bump the language version will be even more of a 
challenge with these sort of weird issues.

>>> Can you try to locally rebuild with this patch 
>>> https://reviews.llvm.org/D131382 ?
>>
>> That improves things but the build still isn't clean:
>> ...
>> (FWIW, I don't know if any of the Windows builders in the lab are building 
>> with /WX)
>
> I am not sure either - but at least this removed the problem with the 
> std=c++17 not being passed around correctly.

Agreed, I can look into fixing up those warnings when I have a spare moment if 
nobody else gets to them first.

>>> I think all the runtime errors is because of that one above - basically we 
>>> don't set std=c++17 for any of the compiler-rt projects.
>>
>> I wasn't building compiler-rt, so no idea why this improved things for me. 
>> FWIW, he's my CMake config: `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON 
>> -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_ENABLE_IDE=ON 
>> -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" 
>> -DLLVM_PARALLEL_COMPILE_JOBS=112 -DLLVM_PARALLEL_LINK_JOBS=16`
>
> That is very odd. I would suspect that the problem was the compiler-rt not 
> getting the right flag. But it seems to influence something else as well?

Yeah, this is rather deeply concerning to be honest. I triple-checked and the 
only changes in my tree were yours to compiler-rt and when I looked at the 
configure logs, they clearly showed `compiler-rt project is disabled` in the 
logs. It is really weird that a change to compiler-rt's cmake would have any 
impact on Clang's ability to build when compiler-rt is disabled. I think that's 
worth some investigative work.

>>> I also think we should merge https://reviews.llvm.org/D131367 for now - we 
>>> can revert that later on if we think it adds to much complexity, since it 
>>> will delete the bad cache values automatcially.
>>
>> Seems reasonable to me.
>
> I landed this now - hopefully that fixes some of the issues seen in the bots 
> - but we might need https://reviews.llvm.org/D131382 as well before they go 
> green. I am not sure what the protocol is here, since @MaskRay suggested the 
> change maybe we should wait for him to land it, or should we get it in ASAP 
> to see if that fixes the bots?

It seems like we might need that, but it also seems like it would be good to 
understand why compiler-rt is impacting the rest of Clang when it's disabled. 
That said, the goal is to get the build lab back to green as quickly as 
possible, so I think it may make sense to land sooner rather than later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to