Skip to content
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

Add option for the behavior of __register_frame/__deregister_frame in ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp #43419

Closed
albertjin mannequin opened this issue Nov 20, 2019 · 17 comments
Labels
bugzilla Issues migrated from bugzilla mcjit

Comments

@albertjin
Copy link
Mannequin

@albertjin albertjin mannequin commented Nov 20, 2019

Bugzilla Link 44074
Resolution FIXED
Resolved on Aug 21, 2021 12:46
Version 9.0
OS Linux
Attachments add cmake option EXECUTION_ENGINE_USE_LLVM_UNWINDER
CC @AlexDenisov,@hvdijk,@lhames
Fixed by commit(s) 9573343

Extended Description

I use the full set of libraries from LLVM working under Linux with no dependency of gcc. While making use of LLVM MCJIT, I observed that there would be an error message from LLVMRunFunction,

libunwind: __unw_add_dynamic_fde: bad fde: FDE is really a CIE

By searching LLVM source code, I got two facts as follows.

  • This message is produced by function __unw_add_dynamic_fde() in libunwind/src/libunwind.cpp.
  • In llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp, it is assumed that the behavior of __register_frame/__deregister_frame from libunwind depends on OS. It is checked by #ifdef __APPLE__.

libunwind, however, is actually a runtime shared library chosen at compile time. For example, it can be specified to use libgcc or libunwind according to the build option CLANG_DEFAULT_UNWINDLIB. It's true that the native libunwind on macOS is from LLVM, and on Linux it is from gcc. But this does not prevent programs for macOS from using the set of runtime libraries from gcc, and vice versa.

I have made a fix by adding a new cmake option EXECUTION_ENGINE_USE_LLVM_UNWINDER. My suggested patch is as attached.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Oct 15, 2020

*** Bug llvm/llvm-bugzilla-archive#47799 has been marked as a duplicate of this bug. ***

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Jun 24, 2021

I extended the patch to modify llvm/lib/ExecutionEngine/Orc/TargetProcess/RegisterEHFrames.cpp following a suggestion from Lang Hames.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Jun 24, 2021

I don't see anywhere to upload the patch as an attachment.
I'm not a regular user of bugs.llvm.org


diff --git a/llvm/include/llvm/Config/config.h.cmake b/llvm/include/llvm/Config/config.h.cmake
index 3aff8be86fcf..cc5dbe3e7b03 100644
--- a/llvm/include/llvm/Config/config.h.cmake
+++ b/llvm/include/llvm/Config/config.h.cmake
@@ -299,6 +299,9 @@
/* Doesn't use cmakedefine because it is allowed to be empty. */
#define LLVM_DEFAULT_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}"

+/* Define if LLVM libunwind is used for ExecutionEngine. /
+#cmakedefine01 EXECUTION_ENGINE_USE_LLVM_UNWINDER
+
/
Define if zlib compression is available */
#cmakedefine01 LLVM_ENABLE_ZLIB

diff --git a/llvm/lib/ExecutionEngine/Orc/TargetProcess/RegisterEHFrames.cpp b/llvm/lib/ExecutionEngine/Orc/TargetProcess/RegisterEHFrames.cpp
index aff7296cb6e3..cdbaeca7aeab 100644
--- a/llvm/lib/ExecutionEngine/Orc/TargetProcess/RegisterEHFrames.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/TargetProcess/RegisterEHFrames.cpp
@@ -86,7 +86,7 @@ static Error deregisterFrameWrapper(const void *P) {
}
#endif

-#ifdef APPLE
+#if defined(APPLE) || (EXECUTION_ENGINE_USE_LLVM_UNWINDER == 1)

template
Error walkAppleEHFrameSection(const char *const SectionStart,
@@ -128,7 +128,7 @@ Error walkAppleEHFrameSection(const char *const SectionStart,

Error registerEHFrameSection(const void *EHFrameSectionAddr,
size_t EHFrameSectionSize) {
-#ifdef APPLE
+#if defined(APPLE) || (EXECUTION_ENGINE_USE_LLVM_UNWINDER == 1)
// On Darwin __register_frame has to be called for each FDE entry.
return walkAppleEHFrameSection(static_cast<const char *>(EHFrameSectionAddr),
EHFrameSectionSize, registerFrameWrapper);
@@ -144,7 +144,7 @@ Error registerEHFrameSection(const void *EHFrameSectionAddr,

Error deregisterEHFrameSection(const void *EHFrameSectionAddr,
size_t EHFrameSectionSize) {
-#ifdef APPLE
+#if defined(APPLE) || (EXECUTION_ENGINE_USE_LLVM_UNWINDER == 1)
return walkAppleEHFrameSection(static_cast<const char *>(EHFrameSectionAddr),
EHFrameSectionSize, deregisterFrameWrapper);
#else
diff --git a/llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp b/llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp
index b6ccd02405c1..27ade7aa4b50 100644
--- a/llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp
+++ b/llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp
@@ -67,8 +67,7 @@ static void __deregister_frame(void *p) {
}
#endif

-#ifdef APPLE

+#if defined(APPLE) || (EXECUTION_ENGINE_USE_LLVM_UNWINDER == 1)
static const char *processFDE(const char *Entry, bool isDeregister) {
const char *P = Entry;
uint32_t Length = *((const uint32_t *)P);

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Jun 30, 2021

Can we make this patch depends on '-DCLANG_DEFAULT_UNWINDLIB="libunwind"',
rather than a new option 'EXECUTION_ENGINE_USE_LLVM_UNWINDER'?

@hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Jul 15, 2021

Can we make this patch depends on '-DCLANG_DEFAULT_UNWINDLIB="libunwind"',
rather than a new option 'EXECUTION_ENGINE_USE_LLVM_UNWINDER'?

CLANG_DEFAULT_UNWINDLIB is for the clang being built, not the clang used for the build. The compiler used for the build does not even need to be clang.

What we can do is detect whether libunwind is used by looking to see whether __unw_add_dynamic_fde/__unw_remove_dynamic_fde is defined. I'm testing a patch that does that.

@hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Jul 16, 2021

Patch up for review at https://reviews.llvm.org/D106129.

@lhames
Copy link
Contributor

@lhames lhames commented Aug 15, 2021

I've approved Harald's patch.

Longer term I think we should revisit the dynamic registration APIs in libunwind to see if we can come up with something universal, but this seems like a good interim solution.

If anyone wants this urgently there may still be time to request that it be cherry-picked to the LLVM 13 release branch. Otherwise it will be in LLVM 14.

@lhames
Copy link
Contributor

@lhames lhames commented Aug 15, 2021

Peter S. Housel's patch https://reviews.llvm.org/D108081 touches on this issue as well.

Maybe the best option, since we control libunwind, would be to add a symbol to that that could be used to identify it at runtime (if there isn't one already). Then we could dynamically choose the correct behavior based on the presence/absence of that symbol?

@hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Aug 15, 2021

Maybe the best option, since we control libunwind, would be to add a symbol
to that that could be used to identify it at runtime (if there isn't one
already). Then we could dynamically choose the correct behavior based on the
presence/absence of that symbol?

If we make sure to add that symbol at the same time that we add support to libunwind for calling __register_frame with the same arguments as libgcc's, we could do this statically, even.

I think changing libunwind is covered by bug 47750, so we can close this one.

@lhames
Copy link
Contributor

@lhames lhames commented Aug 21, 2021

Hi Harald,

I think this may have broken exceptions on one of the Darwin bots:

https://green.lab.llvm.org/green/job/clang-stage1-RA/23471/testReport/junit/LLVM/ExecutionEngine_MCJIT/eh_lg_pic_ll/

It's hard to be certain though -- the bot was failing with other errors when you first committed the change. Would you mind if I temporarily reverted it to see if the bot goes green again?

@lhames
Copy link
Contributor

@lhames lhames commented Aug 21, 2021

Alternatively we could change the check to

#if defined(HAVE_UNW_ADD_DYNAMIC_FDE) || defined(APPLE)

until we can narrow this down further.

@hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Aug 21, 2021

#if defined(HAVE_UNW_ADD_DYNAMIC_FDE) || defined(APPLE) looks good to me, that effectively reverts the patch for Apple platforms while leaving it in place for non-Apple platforms, that gives time to get a better idea of what's going on and how to come up with a better fix.

@hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Aug 21, 2021

Put up for review as https://reviews.llvm.org/D108511. I am testing locally to double check that this still works on my system, but whether it fixes it on the bot is something we will only see after pushing.

@hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Aug 21, 2021

I am testing locally to double check that this still works on my system

It does, so if that is okay with you, please approve and I'll push.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#47799

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@lygstate
Copy link

@lygstate lygstate commented May 22, 2022

I've approved Harald's patch.

Longer term I think we should revisit the dynamic registration APIs in libunwind to see if we can come up with something universal, but this seems like a good interim solution.

If anyone wants this urgently there may still be time to request that it be cherry-picked to the LLVM 13 release branch. Otherwise it will be in LLVM 14.

What's the longer term should be, I am still facing this issue

@lygstate
Copy link

@lygstate lygstate commented May 22, 2022

can __unw_add_dynamic_fde be detected in runtime instead of compile time?

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla mcjit
Projects
None yet
Development

No branches or pull requests

4 participants