This is an archive of the discontinued LLVM Phabricator instance.

[CMake][compiler-rt] Provide a dedicated option for LLVM unwinder
ClosedPublic

Authored by phosek on Dec 13 2021, 1:46 PM.

Details

Summary

This allows configuring LLVM unwinder separately from the C++ library
matching how we configure it in libcxx.

This also applies changes made to libunwind+libcxxabi+libcxx in D113253
to compiler-rt.

Diff Detail

Event Timeline

phosek created this revision.Dec 13 2021, 1:46 PM
phosek requested review of this revision.Dec 13 2021, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 1:46 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
phosek updated this revision to Diff 394049.Dec 13 2021, 1:57 PM
mstorsjo accepted this revision.Dec 13 2021, 2:00 PM
mstorsjo added a subscriber: llvm-commits.

LGTM

(Why don't the sanitizers default to any of the *-commits mailing lists as subscribers?)

This revision is now accepted and ready to land.Dec 13 2021, 2:00 PM
This revision was landed with ongoing or failed builds.May 6 2022, 5:40 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 5:40 PM
phosek reopened this revision.May 6 2022, 10:18 PM
This revision is now accepted and ready to land.May 6 2022, 10:18 PM
mstorsjo added inline comments.May 9 2022, 2:49 AM
compiler-rt/CMakeLists.txt
512

I presume we should only do this if we actually know we're linking against a libunwind that is built in the same cmake invocation, i.e. either of the TARGET unwind_* OR HAVE_LIBUNWIND cases below, otherwise this does break things.

phosek added inline comments.Jun 2 2022, 12:54 PM
compiler-rt/CMakeLists.txt
512

Yes, I hope this could be more cleanly addressed by D126909.

phosek updated this revision to Diff 436511.Jun 13 2022, 12:20 PM
phosek retitled this revision from [runtime] Build compiler-rt with --unwindlib=none to [CMake][compiler-rt] Provide a dedicated option for LLVM unwinder.
phosek edited the summary of this revision. (Show Details)
mstorsjo added inline comments.Jun 13 2022, 1:59 PM
compiler-rt/CMakeLists.txt
508

This todo refers to this change itself.

512

Hmm, this feels a bit nonobvious, when this here isn't directly related to the COMPILER_RT_USE_LLVM_UNWINDER variable. Is it the case that this only gets called when COMPILER_RT_USE_LLVM_UNWINDER is true, or can you end up with other kinds of mismatches?

phosek updated this revision to Diff 436575.Jun 13 2022, 2:44 PM
phosek marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 2:44 PM
phosek added inline comments.Jun 13 2022, 2:44 PM
compiler-rt/CMakeLists.txt
512

I removed it from this change. Prior to this change, C++ ABI and unwinder in compiler-rt is controlled by a single option, but that's causing issues. This change introduces a separate option for controlling the unwinder. I'm also working on another change that will introduce a new option for controlling C++ library. When those land, I think we should deprecate and remove the original flag.

phosek updated this revision to Diff 436584.Jun 13 2022, 3:03 PM

@mstorsjo let me know if this looks good to you, I tested this locally and it seems to be working fine.

mstorsjo accepted this revision.Jun 13 2022, 11:34 PM

The changes left in this patch seem fine to me now!

This revision was landed with ongoing or failed builds.Jun 14 2022, 10:26 AM
This revision was automatically updated to reflect the committed changes.