Skip to content

Conversation

ziyao233
Copy link
Contributor

Currently __register_frame and __unw_add_dynamic_fde are probed by default and OrcJIT prefers the later. But on a system with only LLVM libunwind available, we may need to link libunwind explicitly.

This patch tries to link LLVM libunwind if frame registeration is not available by default, preventing LLVM-built systems falling back to __register_frame code path. Such fallback leads to errors like

$ clang-repl
clang-repl> 1 + 1;
libunwind: __unw_add_dynamic_fde: bad fde: FDE is really a CIE

because __register_frame's behavior differs between libgcc and LLVM libunwind.

Currently __register_frame and __unw_add_dynamic_fde are probed by
default and OrcJIT prefers the later. But on a system with only LLVM
libunwind available, we may need to link libunwind explicitly.

This patch tries to link LLVM libunwind if frame registeration is not
available by default, preventing LLVM-built systems falling back to
__register_frame code path. Such fallback leads to errors like

  $ clang-repl
  clang-repl> 1 + 1;
  libunwind: __unw_add_dynamic_fde: bad fde: FDE is really a CIE

because __register_frame's behavior differs between libgcc and LLVM
libunwind.

Signed-off-by: Yao Zi <ziyao@disroot.org>
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the cmake Build system in general and CMake in particular label Oct 12, 2024
@ziyao233
Copy link
Contributor Author

@chandlerc @lhames Thanks for your review.

@ziyao233
Copy link
Contributor Author

@chandlerc @lhames Ping on this PR, thanks for your review :)

Comment on lines +275 to +281
if (NOT (HAVE_REGISTER_FRAME OR HAVE_DEREGISTER_FRAME OR HAVE_UNW_ADD_DYNAMIC_FDE))
check_library_exists(unwind __unw_add_dynamic_fde "" HAVE_LLVM_LIBUNWIND)
if (HAVE_LLVM_LIBUNWIND)
list(APPEND CMAKE_REQUIRED_LIBRARIES "unwind")
set(HAVE_UNW_ADD_DYNAMIC_FDE 1)
endif ()
endif ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause libunwind to be linked for all LLVM binaries? I think we probably just want it for clang-repl, lli and llvm-jitlink for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think it's a problem. If __register_frame symbols aren't available without linking to external libraries, we're definitely working with compiler-rt & libcxx & libcxxabi, which already require linking to libunwind.
This is also why code with reference to __register_frame successfully builds (but not working correctly) on these systems without explicitly link to libunwind: LLVM requires C++ stdlib, and linking to libcxx implies libunwind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think it's a problem.

So this would cause all of these tools to explicitly link libunwind? That seems like a significant change.

If __register_frame symbols aren't available without linking to external libraries, we're definitely working with compiler-rt & libcxx & libcxxabi, which already require linking to libunwind.

That's surprising to me. I thought libcxx / libcxxabi could be mixed with the gcc unwinder, but I've never actually tried (I'm usually developing on Darwin where libunwind is the system unwinder).

Is it possible to mix libunwind with other (non libcxx) c++ standard libraries?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this BSD, or some other ELF-based system with libunwind as the default unwinder?

Copy link
Contributor Author

@ziyao233 ziyao233 Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think it's a problem.

So this would cause all of these tools to explicitly link libunwind?

Yes.

That seems like a significant change.

If __register_frame symbols aren't available without linking to external libraries, we're definitely working with compiler-rt & libcxx & libcxxabi, which already require linking to libunwind.

That's surprising to me. I thought libcxx / libcxxabi could be mixed with the gcc unwinder, but I've never actually tried (I'm usually developing on Darwin where libunwind is the system unwinder).

Is it possible to mix libunwind with other (non libcxx) c++ standard libraries?

My statement is partially wrong, sorry.

Here the main problem isn't underlinking, but we don't use -lunwind when checking for LLVM libunwind symbols, which are contained in an external library, resulting in misdetection.

A better solution may be

  • Detect whether gcc-style unwinding symbols present by default, if not we assume LLVM libunwind (OrcJIT supports only these two unwinders)
  • If we aren't working with gcc unwinder, link libLLVM to libunwind explicitly, for completeness

Linking libunwind only for clang-repl, lli and llvm-jitlink isn't enough, since it's actually LLVM OrcJIT depending on the unwinder, other programs like mesa use LLVM OrcJIT as well.

What do you think about this solution? Thanks for your patience.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this BSD, or some other ELF-based system with libunwind as the default unwinder?

Yes, the problem is found and tested on eweOS1, a Linux distro built with LLVM toolchain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your minimum libunwind version? We should probably change what the JIT uses instead -- there are better options than __unw_add_dynamic_fde these days.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your minimum libunwind version?

We're shipping LLVM 18.

We should probably change what the JIT uses instead -- there are better options than __unw_add_dynamic_fde these days.

I'm glad to see a better solution that doesn't depend on unwinder-specific behaviour/routine.

@ziyao233
Copy link
Contributor Author

ziyao233 commented Nov 1, 2024

@chandlerc @lhames ping on this, thanks for your review :)

@ziyao233 ziyao233 requested a review from lhames November 1, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants