[cmake] Disable GCC lifetime DSE

[xry111]

Summary

LLVM data structures like llvm::User and llvm::MDNode rely on
the value of object storage persisting beyond the lifetime of the
object (#24952). This is not standard compliant and causes a runtime
crash if LLVM is built with GCC and LTO enabled (#57740). Until
these issues are fixed, we need to disable dead store eliminations
eliminations based on object lifetime.

Depends on rG626849c71e85.

Bug: https://github.com/llvm/llvm-project/issues/24952
Bug: https://github.com/llvm/llvm-project/issues/57740
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106943

[nikic]

LGTM

llvm/cmake/modules/HandleLLVMOptions.cmake

601–603

[xry111]

Update comment and commit message to fix grammar errors according to suggestion from @nikic.

[xry111]

Thanks @nikic for grammar fix.

I don't have write access to llvm-project.git, please land the change if it looks good.

[thesamesam]

Thank you!

[dyung]

@thesamesam, your change is causing a clang-tools-extra test failure, can you take a look and fix it or revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/139/builds/40594
https://lab.llvm.org/buildbot/#/builders/247/builds/4473

[thesamesam]

Apologies - I got the attribution wrong (I forgot that arc doesn't preserve it by default) so I reverted and reapplied with the correct git author.

[thesamesam]

@dyung Sure, I'll take a look now. If I can't fix it tonight, I'll revert it to keep the bots green.

[xry111]

I'm also investigating.

[thesamesam]

I can only reproduce it with a GCC-built LLVM.

Anyway, given LLVM_COMPILER_IS_GCC_COMPATIBLE is true for Clang (see llvm/cmake/modules/DetermineGCCCompatible.cmake), it seems easiest for us to just check CMAKE_COMPILER_IS_GNUCXX instead (given Clang doesn't acknowledge -fno-lifetime-dse at all).

I'll test that now.

llvm/cmake/modules/HandleLLVMOptions.cmake

597–603

bi

597–603

sorry, ignore my kb spam

[thesamesam]

Reverted for now to avoid confusing other people. I'll look into this more later today if @xry111 doesn't beat me to it.

[xry111]

In D150505#4340437, @thesamesam wrote:

I can only reproduce it with a GCC-built LLVM.

Anyway, given LLVM_COMPILER_IS_GCC_COMPATIBLE is true for Clang (see llvm/cmake/modules/DetermineGCCCompatible.cmake), it seems easiest for us to just check CMAKE_COMPILER_IS_GNUCXX instead (given Clang doesn't acknowledge -fno-lifetime-dse at all).

I'll test that now.

Using LLVM_COMPILER_IS_GCC_COMPATIBLE will be helpful to prevent Clang from breaking itself when the lifetime DSE is added in the future. I'm trying to figure out why CMAKE_CXX_FLAGS will affect the behavior of clang-tidy...

[xry111]

Wow. The problem is clang-tidy using the compile_commands.json file in the build directory as a database for options (because the trivially-destructible.cpp.tmp.cpp file is in the build directory). As trivially-destructible.cpp.tmp.cpp is not really in compile_commands.json, it uses the options for tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar-driver.cpp.o which is compiled by GCC with -fno-lifetime-dse...

I think the solution is using the -p option of clang-tidy to override the option database, will try...

[xry111]

Revised changeset at D150524.

[MaskRay]

In D150505#4340623, @xry111 wrote:

Revised changeset at D150524.

For this kind of fixes, the practice is to click "Add Action" - "Reopen Revision" and update a new diff, instead of creating a new differential :)

626849c71e85d546a004cc91866beab610222194 shall fix clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp

[xry111]

With rG626849c71e85d546a004cc91866beab610222194 landed, this should be safe to land now.

[thesamesam]

Phew. Thank you again both.

[xry111]

Added a "depends on" line in the commit msg, so not to puzzle people to wonder why this is landed again w/o change.

[jhuber6]

This patch seems to cause errors with clang-tidy when run inside the build directory. The -fno-lifetime-dse argument is put inside compile_commands.json which then causes errors when clang-tidy is run. For example,

> clang-tidy test.h --checks=-'*,llvmlibc*'
4 warnings and 1 error generated.
Error while processing /home/jhuber/Documents/llvm/llvm-project/build/test.h.
error: unknown argument: '-fno-lifetime-dse' [clang-diagnostic-error]
Suppressed 4 warnings (4 with check filters).
Found compiler error(s).

[xry111]

This comment was removed by xry111.

[jhuber6]

In D150505#4341921, @xry111 wrote:

It's already fixed by rG626849c71e85d546a004cc91866beab610222194. See above.

I'm fairly certain that patch only fixes the failing test and not the underlying problem. I don't know if there's a good way to work around this.

[xry111]

In D150505#4341931, @jhuber6 wrote:

In D150505#4341921, @xry111 wrote:

It's already fixed by rG626849c71e85d546a004cc91866beab610222194. See above.

I'm fairly certain that patch only fixes the failing test and not the underlying problem. I don't know if there's a good way to work around this.

Is it a valid use case to run clang-tidy for a file in a build directory (any build directory, not only LLVM build directory) which is not using clang as the compiler? If we consider it valid we need to fix the general case, i. e. modify clang-tidy to ignore unrecognizable compiler flags.

[uabelho]

Hello,

The following four testcases fail for me with this patch:

Failed Tests (4):
  Clangd :: check-fail.test
  Clangd :: check-lines.test
  Clangd :: check.test
  Clangd :: path-mappings.test

Seems like some bots fail as well, e.g. https://lab.llvm.org/buildbot/#/builders/121/builds/30482

[jhuber6]

llvm/cmake/modules/HandleLLVMOptions.cmake

597

Maybe we could restrict this to LLVM_ENABLE_LTO?

[xry111]

llvm/cmake/modules/HandleLLVMOptions.cmake

597

No, generally we are invoking undefined behavior and generally we need to tell GCC not to consider it undefined. LTO just exploited it.

During our field test -flifetime-dse=1 is enough to cover up the issue, but we decided to "do things correctly" instead of just "make it work".

[xry111]

In D150505#4342005, @uabelho wrote:

Hello,

The following four testcases fail for me with this patch:

Failed Tests (4):
  Clangd :: check-fail.test
  Clangd :: check-lines.test
  Clangd :: check.test
  Clangd :: path-mappings.test

Seems like some bots fail as well, e.g. https://lab.llvm.org/buildbot/#/builders/121/builds/30482

Ouch. Similar to the clang-tidy breakage.

-- trick does not work for clangd. I guess I'll copy an empty option database file into clangd test directory, this is much cleaner than fix it test by test... Wait for an hour or two and I'll submit a diff.

[xry111]

In D150505#4342005, @uabelho wrote:

Hello,

The following four testcases fail for me with this patch:

Failed Tests (4):
  Clangd :: check-fail.test
  Clangd :: check-lines.test
  Clangd :: check.test
  Clangd :: path-mappings.test

Seems like some bots fail as well, e.g. https://lab.llvm.org/buildbot/#/builders/121/builds/30482

Should be fixed by D150582.

[Dinistro]

It seems this change lets us no longer build LLVM with GCC and then run clang-tidy on it.
In our specific case, we have a downstream project that relies on HandleLLVMOptions, and we run the built clang-tidy on our project.

Is there a possible workaround apart from using Clang to compile?

[thesamesam]

In D150505#4349380, @Dinistro wrote:

It seems this change lets us no longer build LLVM with GCC and then run clang-tidy on it.
In our specific case, we have a downstream project that relies on HandleLLVMOptions, and we run the built clang-tidy on our project.

Is there a possible workaround apart from using Clang to compile?

Could you explain what error / issue you're hitting (ideally with some output as it's easier to understand then)? Is it a CMake/configure-time issue, or a build-time issue? Also, which version? (16.0.4 or trunk (if so, what commit?))

[xry111]

I guess the most easy way is adding -flifetime-dse{,=1,=2} and -fno-lifetime-dse as no-op into Clang (and we can really implement -flifetime-dse{,=1,=2} in the future). But not sure if it's the "correct" thing to do.

[jhuber6]

Here's a fun and hacky workaround, make clang map -fno-lifetime-dse to a no-op so the compiler stops complaining about the unknown argument.

[thesamesam]

I think we can get away with doing that. We already do it with plenty of other GCC args..

[Dinistro]

Could you explain what error / issue you're hitting (ideally with some output as it's easier to understand then)? Is it a CMake/configure-time issue, or a build-time issue? Also, which version? (16.0.4 or trunk (if so, what commit?))

We are working with trunk, currently with c04cf58dfc5430f0c82c8ef42c3a8cb43f84020b.

When we run clang-tidy-diff.py from within our CI on the downstream project, we get a crash that looks as follows:

Error while processing /path/to/file.cpp
Suppressed 11764 warnings (11762 in non-user code, 2 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).

error: unknown argument: '-fno-lifetime-dse' [clang-diagnostic-error]

As both LLVM and our own project are built with GCC, the compile_comands.json contains the -fno-lifetime-dse flag.

I suspect that the suggested no-op flag solution would work nicely, though.

[thesamesam]

Thanks for explaining. @xry111 Are you taking a look or shall I?

[MaskRay]

In D150505#4351561, @thesamesam wrote:

Thanks for explaining. @xry111 Are you taking a look or shall I?

I disagree that Clang should ignore -fno-lifetime-dse. Historically Clang have added many ignored options to support a wide range of projects.
Time has passed and there is less motivation to do so.

For -fno-lifetime-dse compile_commands.json users, they or the generator (Bear) are responsible for removing the Clang unaware options, not Clang.

[MaskRay]

In D150505#4349380, @Dinistro wrote:

It seems this change lets us no longer build LLVM with GCC and then run clang-tidy on it.
In our specific case, we have a downstream project that relies on HandleLLVMOptions, and we run the built clang-tidy on our project.

Is there a possible workaround apart from using Clang to compile?

Sorry, but I think your project is responsible for removing -fno-lifetime-dse. It's not Clang's responsibility to parse and ignore every option GCC supports.

[tstellar]

I tend to agree with @MaskRay. I don't think we should be adding more ignored gcc options to clang.

[thesamesam]

The counterpoint would be that _we're_ adding this flag (LLVM needs it) so it makes sense for us to swallow it, i.e. it's not an arbitrary user-defined flag.

But I don't feel comfortable enough deciding what the right call is here and I'll defer to maskray & tstellar. Just let me know what I need to do, if anything.

[jhuber6]

In D150505#4351629, @thesamesam wrote:

The counterpoint would be that _we're_ adding this flag (LLVM needs it) so it makes sense for us to swallow it, i.e. it's not an arbitrary user-defined flag.

But I don't feel comfortable enough deciding what the right call is here and I'll defer to maskray & tstellar. Just let me know what I need to do, if anything.

This was my thought, this is a problem caused directly by LLVM including it against the user's will. I don't think we can simply say it's the build tool's or user's job to remove something they cannot control. Personally, I'm going to get very annoyed very fast deleting this flag from my compiler_commands.json when it gets regenerated every time I do a git pull or change a CMake file.

llvm/cmake/modules/HandleLLVMOptions.cmake

 endif( LLVM_COMPILER_IS_GCC_COMPATIBLE )
 
 # Specific default warnings-as-errors for compilers accepting GCC-compatible warning flags:
 if ( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
   add_flag_if_supported("-Werror=date-time" WERROR_DATE_TIME)
   add_flag_if_supported("-Werror=unguarded-availability-new" WERROR_UNGUARDED_AVAILABILITY_NEW)
 endif( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
 
+if ( LLVM_COMPILER_IS_GCC_COMPATIBLE )
+  # LLVM data structures like llvm::User and llvm::MDNode rely on
+  # the value of object storage persisting beyond the lifetime of the
+  # object (#24952).  This is not standard compliant and causes a runtime
+  # crash if LLVM is built with GCC and LTO enabled (#57740).  Until
+  # these bugs are fixed, we need to disable dead store eliminations
+  # based on object lifetime.
+  add_flag_if_supported("-fno-lifetime-dse" CMAKE_CXX_FLAGS)
+endif ( LLVM_COMPILER_IS_GCC_COMPATIBLE )
+
 # Modules enablement for GCC-compatible compilers:
 if ( LLVM_COMPILER_IS_GCC_COMPATIBLE AND LLVM_ENABLE_MODULES )
   set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
   set(module_flags "-fmodules -fmodules-cache-path=${PROJECT_BINARY_DIR}/module.cache")
   if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
     # On Darwin -fmodules does not imply -fcxx-modules.
     set(module_flags "${module_flags} -fcxx-modules")
   endif()