Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: upgrade LLVM to 18 #14148

Merged
merged 3 commits into from May 14, 2024
Merged

ci: upgrade LLVM to 18 #14148

merged 3 commits into from May 14, 2024

Conversation

alevenberg
Copy link
Member

@alevenberg alevenberg commented May 7, 2024

#14076

This change is Reviewable

@alevenberg alevenberg added the do not review Indicates a PR is not ready for review label May 7, 2024
Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.23%. Comparing base (10e7751) to head (1c8672a).
Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14148      +/-   ##
==========================================
- Coverage   93.79%   93.23%   -0.57%     
==========================================
  Files        2293     2206      -87     
  Lines      202903   192133   -10770     
==========================================
- Hits       190317   179135   -11182     
- Misses      12586    12998     +412     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alevenberg alevenberg removed the do not review Indicates a PR is not ready for review label May 7, 2024
@alevenberg alevenberg marked this pull request as ready for review May 7, 2024 13:50
@alevenberg alevenberg requested a review from a team as a code owner May 7, 2024 13:50
@alevenberg alevenberg added the do not review Indicates a PR is not ready for review label May 7, 2024
@alevenberg
Copy link
Member Author

https://releases.llvm.org/18.1.0/projects/libcxx/docs/ReleaseNotes.html#build-system-changes

This might be relevant

When the shared/static library is built with -fno-exceptions, the behavior of operator new was changed to make it standards-conforming. In LLVM 17 and before, the throwing versions of operator new would return nullptr upon failure to allocate, when the shared/static library was built with exceptions disabled. This was non-conforming, since the throwing versions of operator new are never expected to return nullptr, and this non-conformance could actually lead to miscompiles in subtle cases.

Starting in LLVM 18, the throwing versions of operator new will abort the program when they fail to allocate if the shared/static library has been built with -fno-exceptions. This is consistent with the behavior of all other potentially-throwing functions in the library, which abort the program instead of throwing when -fno-exceptions is used.

Furthermore, when the shared/static library is built with -fno-exceptions, users who override the throwing version of operator new will now need to also override the std::nothrow_t version of operator new if they want to use it. Indeed, this is because there is no way to implement a conforming operator new(nothrow) from a conforming potentially-throwing operator new when compiled with -fno-exceptions. In that case, using operator new(nothrow) without overriding it explicitly but after overriding the throwing operator new will result in an error.

Note that this change only impacts vendors/users that build the shared/static library themselves and pass -DLIBCXX_ENABLE_EXCEPTIONS=OFF, which is not the default configuration. If you are using the default configuration of the library, the libc++ shared/static library will be built with exceptions enabled, and there is no change between LLVM 17 and LLVM 18, even for users who build their own code using -fno-exceptions.

It seems like the failures have to do with throwing exceptions

@alevenberg
Copy link
Member Author

Running locally, getting things like you can't use try or throw when exceptions are disabled

In file included from ./generator/internal/codegen_utils.h:18:
./generator/internal/printer.h:79:5: error: cannot use 'throw' with exceptions disabled
   79 |     throw std::runtime_error(
      |     ^
./generator/internal/printer.h:76:39: error: cannot use 'try' with exceptions disabled
   76 |              std::string const& text) try {
      |                                       ^
./generator/internal/printer.h:93:5: error: cannot use 'throw' with exceptions disabled
   93 |     throw std::runtime_error(
      |     ^
./generator/internal/printer.h:90:30: error: cannot use 'try' with exceptions disabled
   90 |              Args&&... args) try {
      |                              ^

@coryan
Copy link
Member

coryan commented May 10, 2024

https://releases.llvm.org/18.1.0/projects/libcxx/docs/ReleaseNotes.html#build-system-changes

This might be relevant

I am not sure how that would affect the msan build. We only have one build that compiles with -fno-exceptions, that is noex. The msan build compiles with exceptions enabled.

It seems like the failures have to do with throwing exceptions

I suggested turning exceptions off as a way to verify that hypothesis. When we turn exceptions off, nothing throws (we get more crashes). If that fixes the problem then there is something weird with exceptions.

@coryan
Copy link
Member

coryan commented May 10, 2024

Running locally, getting things like you can't use try or throw when exceptions are disabled

Blegh. Can we skip the generator just to test things? As in bazel test //google/cloud/... vs. bazel test //.... It is possible (but not certain) that ci/etc/cloudcxxrc can be used to configure that.

@alevenberg
Copy link
Member Author

Running locally, getting things like you can't use try or throw when exceptions are disabled

Blegh. Can we skip the generator just to test things? As in bazel test //google/cloud/... vs. bazel test //.... It is possible (but not certain) that ci/etc/cloudcxxrc can be used to configure that.

It seems like we also use try in our quickstarts

This worked for me on my workstation. The tests did not pass before this change, and started to pass afterwards.

I decided to use LLVM 18.1.1 because this is the version of Clang included with Fedora:40. I am a coward and fear upgrading the library beyond the corresponding version of the compiler.
@alevenberg alevenberg removed the do not review Indicates a PR is not ready for review label May 14, 2024
@coryan
Copy link
Member

coryan commented May 14, 2024

FTR, it was related to exceptions: by default libc++ now compiles with the LLVM implementation of stack unwinding under exceptions. Meanwhile, the compiler on Fedora is (naturally) trying to use the "native" implementation of stack unwinding. The two things do not work together.

@alevenberg alevenberg merged commit 2bc2215 into googleapis:main May 14, 2024
63 checks passed
@alevenberg alevenberg deleted the llvm branch May 14, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants