This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Add a _LIBUNWIND_VERSION macro
ClosedPublic

Authored by ldionne on Mar 4 2022, 11:36 AM.

Details

Reviewers
mgorny
emaste
MaskRay
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGf29002a4b71b: [libunwind] Add a _LIBUNWIND_VERSION macro
Summary

This allows us to detect whether we're being compiled with LLVM's libunwind
more easily, without CMake having to set explicit variables.

As discussed in https://llvm.org/D119538.

Diff Detail

Event Timeline

ldionne created this revision.Mar 4 2022, 11:36 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 4 2022, 11:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.Mar 4 2022, 11:36 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 4 2022, 11:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
emaste accepted this revision.Mar 4 2022, 12:02 PM
emaste added a subscriber: dim.

LGTM (modulo the lint complaints)

LGTM (modulo the lint complaints)

I think the code would be inconsistent with surrounding code if I listened to them, so I'm tempted to ignore them instead. LMK if you disagree.

@mgorny Is this patch OK with you? Can you test it locally? I suspect that, in its current form, it's not going to quite work for you unless you have basically ToT libunwind headers installed on your system, right?

MaskRay accepted this revision.Mar 11 2022, 6:48 PM
MaskRay added a subscriber: MaskRay.

Look good to ignore clang-format diagnostics.

mgorny accepted this revision.Mar 14 2022, 9:13 AM

I'm sorry, my replacement PSU just arrived today and I'm slowly catching up with everything. I presume we don't need to worry about backwards compatibility, so LGTM.

I'm sorry, my replacement PSU just arrived today and I'm slowly catching up with everything. I presume we don't need to worry about backwards compatibility, so LGTM.

No worries. If you think this works for your use case, then I think this should be good to go. I verified and we do take that code path when we build the runtimes with LIBCXXABI_USE_LLVM_UNWINDER.

However, upon thinking about it again, I think it's safer to add a temporary workaround to detect LLVM's libunwind in libc++abi for a couple of releases, until we can assume that libunwind headers from LLVM 15 (or newer) are being used to build libc++abi. That should take care of folks who might be building libc++abi against an out-of-sync LLVM libunwind (e.g. as part of a SDK or a system-provided libunwind). I'll add a workaround and you folks can tell me what you think -- sorry for the churn.

ldionne updated this revision to Diff 415154.Mar 14 2022, 10:34 AM

Add workaround for building libc++abi against older libunwind. What do people think about this?

ldionne accepted this revision as: Restricted Project, Restricted Project.Mar 30 2022, 8:21 AM

I'll ship this since it should be strictly safer than the previous version of the patch.

This revision is now accepted and ready to land.Mar 30 2022, 8:21 AM
This revision was landed with ongoing or failed builds.Mar 30 2022, 8:23 AM
This revision was automatically updated to reflect the committed changes.