This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Remove unused ARM SEH placeholder code
ClosedPublic

Authored by mstorsjo on Jun 2 2022, 2:38 AM.

Details

Summary

There's no such corresponding code for ARM64 (which has been working
in production for years). The SEH version of the Unwind functions
(e.g. _Unwind_GetLanguageSpecificData) doesn't use these fields.

Diff Detail

Event Timeline

mstorsjo created this revision.Jun 2 2022, 2:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 2 2022, 2:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Jun 2 2022, 2:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 2:38 AM
cdavis5x accepted this revision.Jun 2 2022, 1:10 PM

The handler is used when someone calls _Unwind_ForcedUnwind(). (The lsda is only traced.) But that's not used by the C++ runtime. (Does anybody call it?) If it's never come up on ARM64, I guess it's OK.

MaskRay accepted this revision.Jun 5 2022, 11:29 AM
This revision is now accepted and ready to land.Jun 5 2022, 11:29 AM
manojgupta added a subscriber: manojgupta.

Daniel, can you please check if this removal is ok. I am particularly worried about pthread_cancel which is often hard to debug.

Daniel, can you please check if this removal is ok. I am particularly worried about pthread_cancel which is often hard to debug.

The removed code is guarded by #if defined(_LIBUNWIND_SUPPORT_SEH_UNWIND). ELF is not affected.

Daniel, can you please check if this removal is ok. I am particularly worried about pthread_cancel which is often hard to debug.

What does this code have to do with pthread_cancel? This code in libunwind is for the specific combination of defined(__SEH__) && defined(__arm__); no LLVM compiler has ever produced that combination yet, but once all these pieces are in place, I'll switch the clang mingw arm target to take SEH into use (and start defining that combination).

Thanks for clarifying that ELF is not affected, I just wanted someone from ARM to verify.

This revision was automatically updated to reflect the committed changes.