[PATCH] D114017: [lld-macho][nfc] Factor-out NFC changes from main __eh_frame diff

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:694
 
-    InputSection *lastIsec = subsections.back().isec;
+    Subsection subsec = subsections.back();
+    InputSection *lastIsec = subsec.isec;
----------------
oontvoo wrote:
> gkm wrote:
> > oontvoo wrote:
> > > (dont think it's necessary to copy it)
> > Copying is only necessary to prevent 13 test cases from crashing ... 😵‍💫🤮
> 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 )
> 
> 
> 
+1, this seems unexpected


================
Comment at: lld/MachO/UnwindInfoSection.cpp:207-210
+      // Functions and LSDA entries always reside in the same object file as the
+      // compact unwind entries that references them, and thus appear as section
+      // relocs. There is no need to prepare them. We only prepare relocs for
+      // personality functions.
----------------
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


================
Comment at: lld/MachO/UnwindInfoSection.cpp:215
+      // Personality functions are nearly always system-defined (e.g.,
+      // ___gxx_personality_v0 for C++) and relocated as DyLib symbols.
+      // When an application provides its own personality function,
----------------
it's become a noun in its own right, I don't think most people spell it as an abbreviation...


================
Comment at: lld/MachO/UnwindInfoSection.cpp:217
+      // When an application provides its own personality function,
+      // it might be an extern Defined symbol reloc, or a local section reloc.
       if (auto *defined = dyn_cast<Defined>(s)) {
----------------
oontvoo wrote:
> oontvoo wrote:
> > could also be a Lazy symbol (D110040) ?
> > 
> (p.s : this is a question, not a request to change the comment :) )
"it" would refer to the function here, not the reloc


================
Comment at: lld/MachO/UnwindInfoSection.cpp:217
+      // When an application provides its own personality function,
+      // it might be an extern Defined symbol reloc, or a local section reloc.
       if (auto *defined = dyn_cast<Defined>(s)) {
----------------
int3 wrote:
> oontvoo wrote:
> > oontvoo wrote:
> > > could also be a Lazy symbol (D110040) ?
> > > 
> > (p.s : this is a question, not a request to change the comment :) )
> "it" would refer to the function here, not the reloc
@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 `fetch`ed in the parsing phase already


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114017/new/

https://reviews.llvm.org/D114017