This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho][nfc] Factor-out NFC changes from main __eh_frame diff
ClosedPublic

Authored by gkm on Nov 16 2021, 11:25 AM.

Details

Summary

In order to keep signal:noise high for the __eh_frame diff, I have teased-out the NFC changes and put them here.

Diff Detail

Event Timeline

gkm created this revision.Nov 16 2021, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 11:25 AM
gkm requested review of this revision.Nov 16 2021, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2021, 11:25 AM
oontvoo added inline comments.
lld/MachO/ICF.cpp
190
lld/MachO/InputFiles.cpp
712–713

(dont think it's necessary to copy it)

gkm marked 2 inline comments as done.Nov 16 2021, 2:32 PM
gkm added inline comments.
lld/MachO/InputFiles.cpp
712–713

Copying is only necessary to prevent 13 test cases from crashing ... 😵‍💫🤮

gkm updated this revision to Diff 387767.EditedNov 16 2021, 2:43 PM
gkm marked an inline comment as done.
  • Revise according to review feedback.
  • Move 2 more hunks of NFC-ish code into this diff to ignore useless symbols and SUBTRACTOR relocs in __TEXT,__eh_frame. (See code comments)
oontvoo accepted this revision.Nov 16 2021, 4:27 PM

lg thanks!

lld/MachO/InputFiles.cpp
712–713

Oh, that's ... disturbing. why is it crashing(presumably asan issues?) ?
we're not holding on to the subsec (only its isec field, which is already a pointer )

lld/MachO/UnwindInfoSection.cpp
217

could also be a Lazy symbol (D110040) ?

This revision is now accepted and ready to land.Nov 16 2021, 4:27 PM
oontvoo added inline comments.Nov 16 2021, 4:28 PM
lld/MachO/UnwindInfoSection.cpp
217

(p.s : this is a question, not a request to change the comment :) )

Minor nits and stuff. Overall, generally looks good

lld/MachO/Driver.cpp
1028

Do we also need to check for segment_names::ld here as well? or can that be assumed that compact_unwind is always under __ld?

lld/MachO/InputFiles.cpp
280

Don't you need braces on these surrounding the sec.addr? I'm surprised that the push_back supports constructor initialization via its arguments (emplace_back supports it though).

lld/MachO/InputSection.h
301

nit: Generally, it would be nice to see variables also get defined and used in the same diff to make things more "self-contained". I imagine this will be used in your next diff? No strong opinions though.

int3 added a subscriber: int3.Nov 17 2021, 12:38 PM
int3 added inline comments.
lld/MachO/InputFiles.cpp
712–713

+1, this seems unexpected

lld/MachO/UnwindInfoSection.cpp
211–214

nit: can we move this comment above the if statement on line 205? it makes it more obvious that we have a valid single-line unbraced if statement

215

it's become a noun in its own right, I don't think most people spell it as an abbreviation...

217

"it" would refer to the function here, not the reloc

217

@oontvoo D110040 is about changing local (defined) symbol references into dylib symbol references, that's the only reason we may encounter LazySymbols... but "regular" relocation handling should never encounter LazySymbols since they would all have been fetched in the parsing phase already

gkm marked 7 inline comments as done.Nov 17 2021, 2:12 PM
gkm added inline comments.
lld/MachO/Driver.cpp
1028

Yes. Always.

lld/MachO/InputFiles.cpp
280

push_back() supports the constructor argument list.

712–713

I tracked this down. When subsec is a reference, we hit undefined behavior later when subsec is re-seated at the end of the nested loop. I am surprised there was no warning from the compiler. AFAIK, references cannot be re-seated, but I am now C++ expert. I have reworked the offending code.

lld/MachO/InputSection.h
301

While its first use will be in the substantive __eh_frame diff, it has a life independent of __eh_frame, since LSDA is also used by compact unwind.

gkm updated this revision to Diff 388035.Nov 17 2021, 2:13 PM
gkm marked 4 inline comments as done.
  • Revise according to review feedback
This revision was landed with ongoing or failed builds.Nov 17 2021, 2:20 PM
This revision was automatically updated to reflect the committed changes.