-
Notifications
You must be signed in to change notification settings - Fork 15.7k
[CMake] Re-add -fno-rtti to llvm-config --cxxflags #174084
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
Fix for LLVM_CONFIG_HAS_RTTI.
nikic
left a comment
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.
I'm somewhat confused by how this code works, including for the existing LLVM_CONFIG_HAS_RTTI flag. If LLVM_REQUIRES_RTTI is used somewhere, does this depend on whether the last target for which llvm_update_compile_flags is called happens to have that set or not?
Should llvm-config only be using the global LLVM_ENABLE_RTTI instead of this second LLVM_CONFIG_HAS_RTTI variable that also uses LLVM_REQUIRES_RTTI? And if so, can we extract determining the corresponding compiler flag into a separate function that gets explicitly called from the llvm-config cmake instead of relying on the side-effect here?
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
CI failure was a spurious link error, worked fine on second try. |
nikic
left a comment
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.
LGTM
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/43393 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/19609 Here is the relevant piece of the build log for the reference |
…)" This reverts commit d1b88ca.
|
Buildbot failures seem unrelated: first is just a timeout, second is spurious (tests pass in next build). @ronlieb you noted that this breaks your build -- what's happening and is this a pure downstream thing? |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/13697 Here is the relevant piece of the build log for the reference |
|
@aengelke This is spamming MSVC builds with exception override warnings https://lab.llvm.org/buildbot/#/builders/46/builds/28456 |
|
Thanks for noticing, hopefully fixed with 2b903df. |
Thanks - that fixed it |
seems to be purely downstream, i think i am seeing an additionaly flag "-fno-rtti" during our build of offload |
|
Same, I can see failures downstream even with the commit above, which causes: |
Fix for llvm#173869. If there's no strong reason, we should get rid of per-target RTTI later.
to me this indicates something's off with cmake, let me try a clean build, maybe these rtti changes messed up the cache etc. |
…)" This reverts commit d1b88ca.
New reverts: * llvm/llvm-project#174084 -- causes linker errors related to rtti in stablehlo code * llvm/llvm-project@2b903df -- follow up fix to the above (doesn't fix it for us) Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932.
…)" This reverts commit d1b88ca.
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. * llvm/llvm-project#174084 -- causes linker errors related to rtti in stablehlo code * llvm/llvm-project@2b903df -- follow up fix to the above (doesn't fix it for us) Signed-off-by: Abhishek Varma <[email protected]>
Fix for llvm#173869. If there's no strong reason, we should get rid of per-target RTTI later.
|
I'm also hitting some issues with this commit. It looks like a tablegen component in my project that was built with RTTI disabled by default is now built with RTTI enabled so probably some cmake var is not cached or propagated as expected? |
|
Is this reproducible with a clean cmake configuration? Is LLVM_OPTIMIZED_TABLEGEN set or is this a cross compilation? Can you share the cmake configuration and the compilation commands that lead to the error? |
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. * llvm/llvm-project#174084 -- causes linker errors related to rtti in stablehlo code * llvm/llvm-project@2b903df -- follow up fix to the above (doesn't fix it for us) Signed-off-by: Abhishek Varma <[email protected]> --------- Signed-off-by: Abhishek Varma <[email protected]> Co-authored-by: Jakub Kuderski <[email protected]>
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. * llvm/llvm-project#174084 -- causes linker errors related to rtti in stablehlo code * llvm/llvm-project@2b903df -- follow up fix to the above (doesn't fix it for us) Signed-off-by: Abhishek Varma <[email protected]>
…)" This reverts commit d1b88ca.
|
Hi. I'm getting the following error (which goes away on reverting this PR's commit) :- |
|
@Abhishek-Varma Looks like a downstream problem in iree_llvm. It's not possible to just set some CMake values and expect things to work; you also need to run the code in HandleLLVMOptions.cmake. I don't think we support direct access to functions in AddLLVM (and transitively though AddMLIR) without running the outer skeleton. |
|
I don't think we can expect external projects to mandatorily include I think this PR fixes the problem: #174873
I also found a typo in another CMake variable. |
I noticed that this function in upstream LLVM now depends on also including HandleLLVMOptions (llvm/llvm-project#174084). We could pull that in but it enables a lot of warnings globally, and seems likely to create additional churn when updating LLVM. Instead, we can just disable exceptions and rtti ourselves, and remove calls to the function.
I noticed that this function in upstream LLVM now depends on also including HandleLLVMOptions (llvm/llvm-project#174084). We could pull that in but it enables a lot of warnings globally, and seems likely to create additional churn when updating LLVM. Instead, we can just disable exceptions and rtti ourselves, and remove calls to the function.
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. * llvm/llvm-project#174084 -- causes linker errors related to rtti in stablehlo code * llvm/llvm-project@2b903df -- follow up fix to the above (doesn't fix it for us) Signed-off-by: Abhishek Varma <[email protected]> --------- Signed-off-by: Abhishek Varma <[email protected]>
…)" This reverts commit d1b88ca.
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. * llvm/llvm-project#174084 -- causes linker errors related to rtti in stablehlo code * llvm/llvm-project@2b903df -- follow up fix to the above (doesn't fix it for us) -- Along with the above, IREE header fixes were done to accomodate changes introduced via llvm/llvm-project#82190 for `Affine/Transforms/Passes.h`. NOTE: [Twine fix commit](iree-org/llvm-project@2eaa29c) was dropped from our fork since llvm/llvm-project#175077 got merged upstream. Signed-off-by: Abhishek Varma <[email protected]>
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. * llvm/llvm-project#174084 -- causes linker errors related to rtti in stablehlo code * llvm/llvm-project@2b903df -- follow up fix to the above (doesn't fix it for us) -- Along with the above, IREE header fixes were done to accomodate changes introduced via llvm/llvm-project#82190 for `Affine/Transforms/Passes.h`. NOTE: [Twine fix commit](iree-org/llvm-project@2eaa29c) was dropped from our fork since llvm/llvm-project#175077 got merged upstream. Signed-off-by: Abhishek Varma <[email protected]> Signed-off-by: Abhishek Varma <[email protected]>
I noticed that `llvm_update_compile_flags ` in upstream LLVM now depends on also including HandleLLVMOptions (llvm/llvm-project#174084). We could pull that in but it enables a lot of warnings globally, and seems likely to create additional churn when updating LLVM. Instead, we can just disable exceptions and rtti ourselves, and remove calls to the LLVM provided helpers. This makes the library builds easier to follow, and reduces the chance of further upstream LLVM changes breaking the build.
Fix for #173869.
If there's no strong reason, we should get rid of per-target RTTI later.