This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Use fewer indirections in UnwindInfo implementation
ClosedPublic

Authored by int3 on Apr 6 2022, 8:55 PM.

Details

Summary

The previous implementation of UnwindInfoSection materialized
all the compact unwind entries & applied their relocations, then parsed
the resulting data to generate the final unwind info. This design had
some unfortunate conseqeuences: since relocations can only be applied
after their referents have had addresses assigned, operations that need
to happen before address assignment must contort themselves. (See
D113582: [lld-macho] Support renaming of LSDA section and observe how this diff greatly simplifies it.)

Moreover, it made synthesizing new compact unwind entries awkward.
Handling PR50956 will require us to do this synthesis, and is the main
motivation behind this diff.

Previously, instead of generating a new CompactUnwindEntry directly, we
would have had to generate a ConcatInputSection with a number of
Relocs that would then get "flattened" into a CompactUnwindEntry.

This diff introduces an internal representation of CompactUnwindEntry
(the former CompactUnwindEntry has been renamed to
CompactUnwindLayout). The new CompactUnwindEntry stores references to
its personality symbol and LSDA section directly, without the use of
Reloc structs.

In addition to being easier to work with, this diff also allows us to
handle unwind info whose personality symbols are located in sections
placed after the __unwind_info.

Diff Detail

Event Timeline

int3 created this revision.Apr 6 2022, 8:55 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 6 2022, 8:55 PM
int3 requested review of this revision.Apr 6 2022, 8:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 8:55 PM
int3 updated this revision to Diff 421084.Apr 6 2022, 9:14 PM

add comment

int3 added inline comments.Apr 6 2022, 9:15 PM
lld/test/MachO/compact-unwind.s
95

it turns out that _my_personality below was keeping _foo and _bar alive as they were all part of the same section, and because I'd neglected to add .subsections_via_symbols to the test file. With _my_personality now in a different section, we need .no_dead_strip to keep these other functions alive.

thakis added a subscriber: thakis.Apr 7 2022, 5:57 AM

https://llvm.org/PR50300 looks unrelated. Is that the right PR number?

int3 added a comment.Apr 7 2022, 5:59 AM

Ah I meant https://github.com/llvm/llvm-project/issues/50300. GH issues that originated from Bugzilla have to use the original Bugzilla bug number...

int3 edited the summary of this revision. (Show Details)Apr 7 2022, 5:59 AM
oontvoo added a subscriber: oontvoo.Apr 7 2022, 7:20 AM
oontvoo added inline comments.
lld/MachO/UnwindInfoSection.cpp
107

IIRC, this was changed to Ptr (it was originally uint64_t) because of an overflow bug on 32-bit platforms ...

int3 added inline comments.Apr 7 2022, 11:27 AM
lld/MachO/UnwindInfoSection.cpp
107
oontvoo accepted this revision.Apr 8 2022, 12:07 PM

LG , except the duplicate structs but then I don't have a better suggestion /shrug

lld/MachO/UnwindInfoSection.cpp
98

It seems a little iffy to me that we need this additional struct ... (Would be great to have only one of these two...)

107

Ah, yes. :)

This revision is now accepted and ready to land.Apr 8 2022, 12:07 PM
int3 marked an inline comment as done.Apr 8 2022, 7:32 PM
int3 added inline comments.
lld/MachO/UnwindInfoSection.cpp
98

mm I further "simplify" it in D123277: [lld-macho][nfc] De-templatize UnwindInfoSection, lmk what you think :)

This revision was automatically updated to reflect the committed changes.
int3 marked an inline comment as done.