In order to keep signal:noise high for the __eh_frame diff, I have teased-out the NFC changes and put them here.
Details
- Reviewers
oontvoo - Group Reviewers
Restricted Project - Commits
- rG9cc489a4b2b5: [lld-macho][nfc] Factor-out NFC changes from main __eh_frame diff
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lld/MachO/InputFiles.cpp | ||
---|---|---|
712–713 | Copying is only necessary to prevent 13 test cases from crashing ... 😵💫🤮 |
- 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)
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. |
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 |
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. |
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?