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

[Jez Ng via Phabricator via llvm-commits]

int3 marked 17 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:146
     isec->flags = sec.flags;
-    ret.push_back(isec);
+    subsections.push_back({{0, isec}});
   }
----------------
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?


================
Comment at: lld/MachO/InputFiles.cpp:153
+//
+// \p offset should describe an offset relative to the start of the original
+// InputSection (before any subsection splitting has occurred). It will be
----------------
smeenai wrote:
> I think `\p` only makes sense for Doxygen-style comments (which would have triple slashes)? I'm not very well versed with this though.
Ah, yeah I'm not very familiar with Doxygen either... in any case, this static function probably doesn't merit a Doxygen comment, but I wanted a way to denote a parameter, and typing out "parameter foo" quickly gets repetitive. I guess I could do something like "offset: an offset relative to ..."


================
Comment at: lld/MachO/InputFiles.cpp:252
+
+    // We can't create alt-entry symbols at this point because a later symbol
+    // may split its section, which may affect which subsection the alt-entry
----------------
smeenai wrote:
> The implication is that the nList entries aren't (necessarily) in address order, right? Otherwise we'd know the final subsection for an alt-entry symbol by the time we reached it.
yup. I'll make that explicit


================
Comment at: lld/MachO/InputFiles.cpp:278
+    firstIsec->data = firstIsec->data.slice(0, firstSize);
+    secondIsec->align = MinAlign(firstIsec->align, offset);
+
----------------
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`.


================
Comment at: lld/test/MachO/subsections-section-relocs.s:14
+# CHECK:       [[#%x, ADDR:]]: leaq
+# CHECK-SAME:    [[#%u, CSTRING_ADDR - ADDR - 3]](%rip), %rsi {{.*}} <_bar_str+0x4>
+
----------------
smeenai wrote:
> Could you add a comment explaining the ` - 3`? I believe it's because `L_.str` should end up at `CSTRING_ADDR + 4`, and the `leaq` instruction is 7 bytes long so the relocation is against `ADDR + 7`, so you get `CSTRING_ADDR + 4 - (ADDR + 7)`, but that took a bit of working out :)
yup, you're right that it's not straightforward...


================
Comment at: lld/test/MachO/subsections-section-relocs.s:32
+## References to this generate a section relocation
+## N.B.: ld64 doesn't actually reorder symbols in __cstring based on the order
+##       file. Only our implementation does. However, I'm not sure how else to
----------------
smeenai wrote:
> Hmm, how can it emit a symbol relocation against a private symbol? Or does it convert it into a non-private symbol?
> 
> In general, ld64 atomizes cstring sections by NUL delimiters instead of by symbols, so we'll need to adjust things if we go down that route. This is good for now though.
it doesn't emit a symbol relocation against a private symbol, but private symbols are omitted from the object file, so we have no way of reordering them...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79926