-
Notifications
You must be signed in to change notification settings - Fork 15.9k
[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]> Signed-off-by: Abhishek Varma <[email protected]>
|
I also hit a linking error on my downstream project. I have to add |
…)" 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]>
This should help users who use AddLLVM.cmake without HandleLLVMOptions. Note that remains MSVC is unlikely to work. Follow up of #174084.
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]>
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. Reverts dropped: * llvm/llvm-project#174084 and followups - it works now
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]>
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. Reverts dropped: * llvm/llvm-project#174084 and followups - it works now Local workarounds dropped: * Remove copying around some Python DLLs since upstream seems to put them in the right place now
|
I do wonder whether omitting the exception flags from llvm-config is really the right choice. While not strictly necessary for ABI compatibility, it does seem pretty likely that code directly interfacing with LLVM also wants to use I ran into this change due to a build failure in LLVM bindings on MacOS, because C++17 with deployment target 10.12 is only supported with Manually setting |
|
I'm not fully confident on whether we should include the EH flags or not, but I've put up #176195 anyway, before I start introducing workarounds downstream. |
#173869 accidentally dropped rtti and eh flags from `llvm-config --cxxflags`. Then #174084 restored the rtti flags. The eh flags were not included with the rationale that they are not ABI relevant. This PR restores the eh flags as well. While they are not strictly necessary, I believe that code that directly interfaces with LLVM almost certainly does not want to build with exceptions if LLVM is not built with exceptions. Building in the peculiar `-fexceptions -fno-rtti` configuration is rarely useful and likely not intended. On MacOS, this is also relevant because it's not possible to use C++17 headers without `-fno-exceptions` when using older deployment targets. In that configuration, `-fno-exceptions` is required to interact with LLVM.
…(#176195) llvm/llvm-project#173869 accidentally dropped rtti and eh flags from `llvm-config --cxxflags`. Then llvm/llvm-project#174084 restored the rtti flags. The eh flags were not included with the rationale that they are not ABI relevant. This PR restores the eh flags as well. While they are not strictly necessary, I believe that code that directly interfaces with LLVM almost certainly does not want to build with exceptions if LLVM is not built with exceptions. Building in the peculiar `-fexceptions -fno-rtti` configuration is rarely useful and likely not intended. On MacOS, this is also relevant because it's not possible to use C++17 headers without `-fno-exceptions` when using older deployment targets. In that configuration, `-fno-exceptions` is required to interact with LLVM.
llvm#173869 accidentally dropped rtti and eh flags from `llvm-config --cxxflags`. Then llvm#174084 restored the rtti flags. The eh flags were not included with the rationale that they are not ABI relevant. This PR restores the eh flags as well. While they are not strictly necessary, I believe that code that directly interfaces with LLVM almost certainly does not want to build with exceptions if LLVM is not built with exceptions. Building in the peculiar `-fexceptions -fno-rtti` configuration is rarely useful and likely not intended. On MacOS, this is also relevant because it's not possible to use C++17 headers without `-fno-exceptions` when using older deployment targets. In that configuration, `-fno-exceptions` is required to interact with LLVM.
This should help users who use AddLLVM.cmake without HandleLLVMOptions. Note that remains MSVC is unlikely to work. Follow up of llvm#174084.
llvm#173869 accidentally dropped rtti and eh flags from `llvm-config --cxxflags`. Then llvm#174084 restored the rtti flags. The eh flags were not included with the rationale that they are not ABI relevant. This PR restores the eh flags as well. While they are not strictly necessary, I believe that code that directly interfaces with LLVM almost certainly does not want to build with exceptions if LLVM is not built with exceptions. Building in the peculiar `-fexceptions -fno-rtti` configuration is rarely useful and likely not intended. On MacOS, this is also relevant because it's not possible to use C++17 headers without `-fno-exceptions` when using older deployment targets. In that configuration, `-fno-exceptions` is required to interact with LLVM.
llvm#173869 accidentally dropped rtti and eh flags from `llvm-config --cxxflags`. Then llvm#174084 restored the rtti flags. The eh flags were not included with the rationale that they are not ABI relevant. This PR restores the eh flags as well. While they are not strictly necessary, I believe that code that directly interfaces with LLVM almost certainly does not want to build with exceptions if LLVM is not built with exceptions. Building in the peculiar `-fexceptions -fno-rtti` configuration is rarely useful and likely not intended. On MacOS, this is also relevant because it's not possible to use C++17 headers without `-fno-exceptions` when using older deployment targets. In that configuration, `-fno-exceptions` is required to interact with LLVM. (cherry picked from commit 9bbea75)
…(#176195) llvm/llvm-project#173869 accidentally dropped rtti and eh flags from `llvm-config --cxxflags`. Then llvm/llvm-project#174084 restored the rtti flags. The eh flags were not included with the rationale that they are not ABI relevant. This PR restores the eh flags as well. While they are not strictly necessary, I believe that code that directly interfaces with LLVM almost certainly does not want to build with exceptions if LLVM is not built with exceptions. Building in the peculiar `-fexceptions -fno-rtti` configuration is rarely useful and likely not intended. On MacOS, this is also relevant because it's not possible to use C++17 headers without `-fno-exceptions` when using older deployment targets. In that configuration, `-fno-exceptions` is required to interact with LLVM. (cherry picked from commit 9bbea75)
llvm#173869 accidentally dropped rtti and eh flags from `llvm-config --cxxflags`. Then llvm#174084 restored the rtti flags. The eh flags were not included with the rationale that they are not ABI relevant. This PR restores the eh flags as well. While they are not strictly necessary, I believe that code that directly interfaces with LLVM almost certainly does not want to build with exceptions if LLVM is not built with exceptions. Building in the peculiar `-fexceptions -fno-rtti` configuration is rarely useful and likely not intended. On MacOS, this is also relevant because it's not possible to use C++17 headers without `-fno-exceptions` when using older deployment targets. In that configuration, `-fno-exceptions` is required to interact with LLVM.
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. Signed-off-by: Keshav Vinayak Jha <[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]> --------- Signed-off-by: Abhishek Varma <[email protected]> Co-authored-by: Jakub Kuderski <[email protected]> Signed-off-by: Keshav Vinayak Jha <[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]> --------- Signed-off-by: Abhishek Varma <[email protected]> Signed-off-by: Keshav Vinayak Jha <[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]> Signed-off-by: Keshav Vinayak Jha <[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]> Signed-off-by: Abhishek Varma <[email protected]> Signed-off-by: Keshav Vinayak Jha <[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]> Signed-off-by: Abhishek Varma <[email protected]> Signed-off-by: Keshav Vinayak Jha <[email protected]>
Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. Reverts dropped: * llvm/llvm-project#174084 and followups - it works now Local workarounds dropped: * Remove copying around some Python DLLs since upstream seems to put them in the right place now Signed-off-by: Keshav Vinayak Jha <[email protected]>
Fix for #173869.
If there's no strong reason, we should get rid of per-target RTTI later.