[PATCH] D79926: [lld-macho] Support .subsections_via_symbols

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lld/MachO/InputFiles.cpp:146
     isec->flags = sec.flags;
-    ret.push_back(isec);
+    subsections.push_back({{0, isec}});
   }
----------------
int3 wrote:
> smeenai wrote:
> > Nit: would `emplace_back` let us get rid of one pair of curlies?
> doesn't seem to work:
> 
> ```
> /Users/jezng/local/llvm-project/lld/MachO/InputFiles.cpp:146:17: error: no matching member function for call to 'emplace_back'
>     subsections.emplace_back({0, isec});
>     ~~~~~~~~~~~~^~~~~~~~~~~~
> /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/vector:722:19: note: candidate function [with _Args = <>] not viable: requires 0 arguments, but 1 was provided
>         void      emplace_back(_Args&&... __args);
> ```
> 
> not really sure what that means, but I'm guessing that the initializer list is screwing up the template resolution?
Ah, yeah, I think there's some interesting clashes between template deduction and initializer lists.


================
Comment at: lld/MachO/InputFiles.cpp:278
+    firstIsec->data = firstIsec->data.slice(0, firstSize);
+    secondIsec->align = MinAlign(firstIsec->align, offset);
+
----------------
int3 wrote:
> smeenai wrote:
> > What's the reason for this?
> The symbol we're splitting on may not be at an aligned offset from the first input section. E.g. we may have a section `__text` aligned to 0x8 and a symbol offset 0x4 from the start of `__text`.
Hmm. I guess choosing the minimum makes sense in that case? I'd be curious if ld64 does that or just preserves the alignment of the original section (and aligns the subsection accordingly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79926