This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Add UNW_AARCH64_* beside UNW_ARM64_*
ClosedPublic

Authored by MaskRay on Aug 12 2021, 2:18 PM.

Details

Summary

The original libunwind project defines UNW_AARCH64_* instead of UNW_ARM64_*.
Rename the enum members to match. This allows some applications with simple
unw_init_local usage to migrate to llvm-project libunwind.

Note: the canonical names of UNW_ARM_D{0..31} are now UNW_AARCH64_V{0..31},
to match the original libunwind.

UNW_ARM64_* are kept for now for compatibility. Some may be unneeded and can be
cleaned up in the future.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 12 2021, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2021, 2:18 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
MaskRay requested review of this revision.Aug 12 2021, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2021, 2:18 PM
MaskRay updated this revision to Diff 366099.Aug 12 2021, 2:20 PM

(accidentally mixed unrelated changes.)

Not sure keeping UNW_ARM64_* has any compatibility importance.

There’s code using the llvm version of libunwind that expects these names, see e.g. https://source.winehq.org/git/wine.git/blob/6b58d34a625ffaad181a7316009398f3c6444181:/dlls/ntdll/unix/signal_arm64.c#l250.

(Also, the same file, on lines 188-208, has a different codepath for setting registers with llvm’s libunwind, iirc because our context is opaque. The corresponding file for x86_64 also has that distinction, even if the enum names match there.)

Not sure keeping UNW_ARM64_* has any compatibility importance.

There’s code using the llvm version of libunwind that expects these names, see e.g. https://source.winehq.org/git/wine.git/blob/6b58d34a625ffaad181a7316009398f3c6444181:/dlls/ntdll/unix/signal_arm64.c#l250.

(Also, the same file, on lines 188-208, has a different codepath for setting registers with llvm’s libunwind, iirc because our context is opaque. The corresponding file for x86_64 also has that distinction, even if the enum names match there.)

Can wine's __APPLE__ code path be fixed to use UNW_AARCH64_* (defined(UNW_AARCH64_X0)) instead? :)

Not sure keeping UNW_ARM64_* has any compatibility importance.

There’s code using the llvm version of libunwind that expects these names, see e.g. https://source.winehq.org/git/wine.git/blob/6b58d34a625ffaad181a7316009398f3c6444181:/dlls/ntdll/unix/signal_arm64.c#l250.

(Also, the same file, on lines 188-208, has a different codepath for setting registers with llvm’s libunwind, iirc because our context is opaque. The corresponding file for x86_64 also has that distinction, even if the enum names match there.)

Can wine's __APPLE__ code path be fixed to use UNW_AARCH64_* (defined(UNW_AARCH64_X0)) instead? :)

Hmm, as these are enums, not defines, the code can’t check that :-( I guess worst case it’d require a configure test to see what the libunwind actually provides.

compnerd added inline comments.
libunwind/include/libunwind.h
567

This is a public header, I don't think that changing this like this without compatibility is good. I _could_ be convinced that this isn't too big of a deal if @Bigcheese signs off on this on behalf of Apple.

However, I wouldn't mind if we had aliases for the name.

MaskRay updated this revision to Diff 367051.Aug 17 2021, 4:10 PM

Add some UNW_ARM64_* values used by wine for __APPLE__

MaskRay updated this revision to Diff 367892.Aug 20 2021, 2:12 PM
MaskRay edited the summary of this revision. (Show Details)

keep all UNW_ARM64_*

compnerd accepted this revision.Aug 20 2021, 2:17 PM

I'll assume that you caught all the instances of _ARM64_ when you switched the usage to the _AARCH64_ spelling. I think it may be nicer to keep the current spelling as the default, but it doesn't really matter to the users.

libunwind/include/libunwind.h
495–534

Wouldn't mind a new line before this block either.

570

Might be nice to have a comment like // Compatibility aliases or something.

This revision is now accepted and ready to land.Aug 20 2021, 2:17 PM
MaskRay updated this revision to Diff 367895.Aug 20 2021, 2:22 PM
MaskRay marked 2 inline comments as done.

comments

MaskRay retitled this revision from [libunwind] Rename UNW_ARM64_* to UNW_AARCH64_* to [libunwind] Add UNW_AARCH64_* beside UNW_ARM64_*.Aug 20 2021, 2:23 PM
This revision was landed with ongoing or failed builds.Aug 20 2021, 2:26 PM
This revision was automatically updated to reflect the committed changes.