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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 7 inline comments as done.
gkm added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1028
+        continue;
+      if (subsections[0].isec->getName() == section_names::compactUnwind)
+        // Compact unwind entries require special handling elsewhere.
----------------
thevinster wrote:
> 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? 
Yes. Always.


================
Comment at: lld/MachO/InputFiles.cpp:278
             " is too large");
-      sections.push_back({});
+      sections.push_back(sec.addr);
       continue;
----------------
thevinster wrote:
> 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).
`push_back()` supports the constructor argument list.


================
Comment at: lld/MachO/InputFiles.cpp:694
 
-    InputSection *lastIsec = subsections.back().isec;
+    Subsection subsec = subsections.back();
+    InputSection *lastIsec = subsec.isec;
----------------
int3 wrote:
> 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
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.


================
Comment at: lld/MachO/InputSection.h:306
 constexpr const char ehFrame[] = "__eh_frame";
+constexpr const char gccExceptTab[] = "__gcc_except_tab";
 constexpr const char export_[] = "__export";
----------------
thevinster wrote:
> 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. 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114017