-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[runtimes] Append -nostd*++
flags only for Clang
#151930
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
Conversation
-nostdlib++
added to CMAKE_REQUIRED_FLAGS
breaks hwloc detection for OMPT
#90332
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we need the required flags for again? I'm guessing this is just for checking C/C++ flags?
That's my understanding — we're adding them, so CMake checks don't fail prior to the runtimes being built. Unfortunately, looks like my idea breaks some stuff still. That said, I'm not 100% convinced it just doesn't reveal an existing bug:
I'm guessing that the code relied on |
I think it was added because originally, LLVM_ENABLE_RUNTIMES was invented for libc++(abi). It is C++, but obviously cannot link against libc++.so before libc++.so is built. For any library that is not libc++ but uses the C++ standard library (flang-rt, hwloc through openmp, maybe others), as long as there is a libc++/libstdc++ installed on the system and found by Clang, it can use that. If we want those runtimes to link together with just-built libc++, we need a 4-stage bootstrapping build (clang -> compiler-rt builtins -> libc++ -> flang-rt/openmp). Or, at least this seems to be the current approach, make the configure step not depend on the existence of the C++ standard library, but for build-time, one can add a dependency to libc++ if built in the same runtimes build. Also see #131010 Note that you can temporarily get rid of the flags using There is a mechanism to add flags only for C++: |
Oh, that's an interesting approach. I need to set the build right locally to reproduce the issue, then I'll try that. |
Unfortunately, it doesn't seem to:
|
Hmm, perhaps another approach would be to apply them only when the compiler is actually clang. |
Append `-nostdlib++` and `-nostdinc++` flags to `CMAKE_REQUIRED_FLAGS` only if we are actually building with Clang. These flags are also passed to the C compiler, which is not allowed in GCC. Since CMake implicitly performs some tests using the C compiler, this can lead to incorrect check results. This should be safe, since FWIU we only need them when bootstrapping Clang. Even though we know that Clang supports these flags, we still need to explicitly check if they work, as in some scenarios adding `-nostdlib++` actually breaks the build. See PR llvm#108357 for examples of that. Fixes llvm#90332 Signed-off-by: Michał Górny <[email protected]>
6fdffd0
to
0e83408
Compare
-nostd*++
flags only if necessary-nostd*++
flags only for Clang
Ok, let's see if this works. I've combined check for clang with explicit checks to prevent the fallout from #108357. I suppose another approach would be to test both the C and C++ compiler, and add the flags only if both work. |
I don't think we fully need to bootstrap it, we already have |
Okay, I think this version does not introduce any buildbot regressions. It also works for Gentoo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks fine (enough) for me if others are happy.
@jhuber6, do you have an opinion on this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems innocuous enough.
Okay, thanks. Let's give it a shot. |
Append
-nostdlib++
and-nostdinc++
flags toCMAKE_REQUIRED_FLAGS
only if we are actually building with Clang. These flags are also
passed to the C compiler, which is not allowed in GCC. Since CMake
implicitly performs some tests using the C compiler, this can lead
to incorrect check results. This should be safe, since FWIU we only
need them when bootstrapping Clang.
Even though we know that Clang supports these flags, we still need
to explicitly check if they work, as in some scenarios adding
-nostdlib++
actually breaks the build. See PR #108357 for examplesof that.
Fixes #90332