[PATCH] D114961: [ORC-RT][ORC] Handle dynamic unwind registration for libunwind

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

Hi @housel,

This looks great -- with this applied I was able to use libunwind with the JIT on linux without issues.

I did run in to a config issue with the unit tests -- they seem to point to the wrong libunwind path when libunwind is enabled via `-DLLVM_ENABLE_RUNTIMES` rather than `-DLLVM_ENABLE_PROJECTS`. Using the following diff for my regression test config (rather than the changes above) got things working for both configs:

  diff --git a/compiler-rt/test/orc/lit.cfg.py b/compiler-rt/test/orc/lit.cfg.py
  index 5ce6c8b1fa7f..b2e97cefbf0e 100644
  --- a/compiler-rt/test/orc/lit.cfg.py
  +++ b/compiler-rt/test/orc/lit.cfg.py
  @@ -18,6 +18,11 @@ if config.host_os == 'Darwin':
   else:
     orc_rt_path = '%s/libclang_rt.orc%s.a' % (config.compiler_rt_libdir, config.target_suffix)
   
  +if config.libunwind_shared:
  +  config.available_features.add('libunwind-available')
  +  shared_libunwind_path = os.path.join(config.libunwind_install_dir, 'libunwind.so')
  +  config.substitutions.append( ("%shared_libunwind", shared_libunwind_path) )
  +
   config.substitutions.append(
       ('%clang ', build_invocation([config.target_cflags])))
   config.substitutions.append(
  diff --git a/compiler-rt/test/orc/lit.site.cfg.py.in b/compiler-rt/test/orc/lit.site.cfg.py.in
  index d5bb51c9be80..cd0671279741 100644
  --- a/compiler-rt/test/orc/lit.site.cfg.py.in
  +++ b/compiler-rt/test/orc/lit.site.cfg.py.in
  @@ -6,6 +6,8 @@ config.orc_lit_source_dir = "@ORC_LIT_SOURCE_DIR@"
   config.target_cflags = "@ORC_TEST_TARGET_CFLAGS@"
   config.target_arch = "@ORC_TEST_TARGET_ARCH@"
   config.built_with_llvm = ("@COMPILER_RT_STANDALONE_BUILD@" != "TRUE")
  +config.libunwind_shared = "@LIBUNWIND_ENABLE_SHARED@"
  +config.libunwind_install_dir = "@LLVM_BINARY_DIR@/@LIBUNWIND_INSTALL_LIBRARY_DIR@"



================
Comment at: compiler-rt/test/orc/TestCases/FreeBSD/ehframe-default.cpp:8
+
+namespace test5 {
+struct A {
----------------
Why `test5`?


================
Comment at: compiler-rt/test/orc/TestCases/Linux/ehframe-default.cpp:4-28
+extern "C" {
+void llvm_jitlink_setTestResultOverride(long Value);
+};
+
+namespace test5 {
+struct A {
+  A() = default;
----------------
I think this can be shrunk to:

```
extern "C" void llvm_jitlink_setTestResultOverride(long Value);

int main(int argc, char *argv[]) {
  llvm_jitlink_setTestResultOverride(1);
  try {
    throw 0;
  } catch (int X) {
    llvm_jitlink_setTestResultOverride(X);
  }
  return 0;
}
```


================
Comment at: compiler-rt/test/orc/TestCases/Linux/ehframe-libunwind.cpp:5-29
+extern "C" {
+void llvm_jitlink_setTestResultOverride(long Value);
+};
+
+namespace test5 {
+struct A {
+  A() = default;
----------------
Ditto here.


================
Comment at: llvm/lib/ExecutionEngine/Orc/ELFNixPlatform.cpp:201-213
+  if (expectedToOptional(
+          ES.lookup(makeJITDylibSearchOrder(
+                        &PlatformJD, JITDylibLookupFlags::MatchAllSymbols),
+                    LibUnwindRegisterFrame))) {
+    LLVM_DEBUG({
+      dbgs() << "Using libunwind " << LibUnwindRegisterFrame
+             << " for unwind info registration\n";
----------------
`MatchAllSymbols` shouldn't be needed here: We expect `__unw_add_dynamic_eh_frame_section` to be exported.

The `expectedToOptional` bit could be more idiomatically expressed as:

```
auto SM =
  ES.lookup(
    makeJITDylibSearchOrder(&PlatformJD),
    SymbolLookupSet()
      .add(LibUnwindRegisterFrame, SymbolLookupFlags::WeaklyReferencedSymbol)
      .add(LibUnwindDeregisterFrame, SymbolLookupFlags::WeaklyReferencedSymbol));

if (!SM) // Weak-ref means no "missing symbol" errors, so this must be something more serious that we should report.
  return SM.takeError();

if (SM.size() == 2) {
  // Use _unw_add/remove_dynamic_eh_frame_section
} else {
  // Use __[de]register_frame.
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114961/new/

https://reviews.llvm.org/D114961