Page MenuHomePhabricator

[CMake][ExecutionEngine] add HAVE_(DE)REGISTER_FRAME as a config.h macros
ClosedPublic

Authored by daltenty on Sep 3 2020, 3:05 PM.

Details

Summary

The macro HAVE_EHTABLE_SUPPORT is used by parts of ExecutionEngine to tell register_frame/deregister_frame is available to register the
FDE for a generated (JIT) code. It's currently set by a slowly growing set of macro tests in the respective headers, which is updated now and then when it fails to link on some platform or another due to the symbols being missing (see for example https://bugs.llvm.org/show_bug.cgi?id=5715).

This change converts the macro in two HAVE_(DE)REGISTER_FRAME config.h macros (like most of the other HAVE_* macros) and set's them based on whether CMake can actually find a definition for these symbols to link to at configuration time.

Diff Detail

Event Timeline

daltenty created this revision.Sep 3 2020, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2020, 3:05 PM
daltenty requested review of this revision.Sep 3 2020, 3:05 PM
daltenty updated this revision to Diff 290476.Sep 8 2020, 7:08 AM
  • Adjust so we don't alter the MVS case

Do we want to take advantage of this patch to move those checks into the cmake configure so we don't need to duplicate the logic in 2 files as @sfertile mentioned previously?

Do we want to take advantage of this patch to move those checks into the cmake configure so we don't need to duplicate the logic in 2 files as @sfertile mentioned previously?

I think that makes a lot of sense, that would allow for making it an overridable configuration setting. We'll just have to be careful to replicate the behaviour of the conditions here. I'll proceed in that direction.

General comment: CMake is not the only mechanism for making the conditions common; a header file is capable of the same.

daltenty updated this revision to Diff 297313.Oct 9 2020, 12:46 PM
  • Upgrade HAVE_EHTABLE_SUPPORT to a real config.h macro
daltenty retitled this revision from [AIX][ExecutionEngine] Disable eh frame support when building with Clang on AIX to [CMake][ExecutionEngine] Upgrade HAVE_EHTABLES to a real config.h macro.Oct 9 2020, 1:02 PM
daltenty edited the summary of this revision. (Show Details)
daltenty added a reviewer: Xiangling_L.
daltenty updated this revision to Diff 297318.Oct 9 2020, 1:04 PM
  • Add a newline
daltenty retitled this revision from [CMake][ExecutionEngine] Upgrade HAVE_EHTABLES to a real config.h macro to [CMake][ExecutionEngine] Upgrade HAVE_EHTABLES_SUPPORT to a real config.h macro.Oct 9 2020, 1:17 PM
daltenty retitled this revision from [CMake][ExecutionEngine] Upgrade HAVE_EHTABLES_SUPPORT to a real config.h macro to [CMake][ExecutionEngine] convert HAVE_EHTABLES_SUPPORT to a real config.h macro.
llvm/include/llvm/Config/config.h.cmake
62 ↗(On Diff #297318)

I think the name is not appropriate in the wider context. I suggest having separate config variables for each of the functions (like the one for _Unwind_Backtrace) and then using those to define HAVE_EHTABLE_SUPPORT where it is defined before this patch.

daltenty added inline comments.Oct 30 2020, 8:03 AM
llvm/include/llvm/Config/config.h.cmake
62 ↗(On Diff #297318)

I think that makes sense, though I'd prefer to drop the macro HAVE_EHTABLE_SUPPORT entirely then. What this code is really looking for the presence of the registration / deregistration functions anyway, nothing else.

daltenty updated this revision to Diff 301901.Oct 30 2020, 8:04 AM
  • Cut out the middle man of 'HAVE_EHTABLE_SUPPORT' and just say what we mean.
daltenty retitled this revision from [CMake][ExecutionEngine] convert HAVE_EHTABLES_SUPPORT to a real config.h macro to [CMake][ExecutionEngine] add HAVE_(DE)REGISTER_FRAME as a config.h macros.Oct 30 2020, 8:08 AM
daltenty edited the summary of this revision. (Show Details)
daltenty edited the summary of this revision. (Show Details)
llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp
35–37

Just noting that, since the fallback path attempts to use the functions in question by finding them at runtime, I guess none of the macros queried here is actually effective at preventing an attempt to register the frame.

It is still possible that the macros were effective at preventing a hard load-dependency on certain DLLs though. @daltenty, should __SEH__ and __USING_SJLJ_EXCEPTIONS__ continue to force the fallback path?

daltenty added inline comments.Nov 5 2020, 8:09 AM
llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp
35–37

I think that seems advisable for the reason you note.

daltenty updated this revision to Diff 303126.Nov 5 2020, 8:09 AM
  • Add back in SEH and SJLJ eh guards
MaskRay added a subscriber: MaskRay.Nov 5 2020, 8:21 AM
MaskRay added inline comments.
llvm/cmake/config-ix.cmake
209 ↗(On Diff #303126)

Is CMAKE_CURRENT_LIST_DIR needed?

daltenty added inline comments.Nov 5 2020, 3:17 PM
llvm/cmake/config-ix.cmake
209 ↗(On Diff #303126)

Yes, looks like CMake just inserts the arg as-is into the test program and we want to use the header stub, not the system unwind.h.

LGTM; thanks!

llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp
35–37

Thanks.

This revision is now accepted and ready to land.Nov 9 2020, 2:54 PM
daltenty updated this revision to Diff 304179.Nov 10 2020, 7:05 AM
  • Prefer cmakedefine to cmakedefine01 because the later doesn't work with defined() macro checks
daltenty updated this revision to Diff 304225.Nov 10 2020, 9:04 AM
  • Suppress linter warnings for header stub
This revision was landed with ongoing or failed builds.Nov 10 2020, 10:11 AM
This revision was automatically updated to reflect the committed changes.