[PATCH] D97007: [lld-macho] Define __mh_*_header synthetic symbols.

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/Symbols.h:86-88
+  // Whether this symbol should be forced to appear in the output binary's
+  // symbol table.
+  SymbolTablePresence presence : 2;
----------------
int3 wrote:
> int3 wrote:
> > oontvoo wrote:
> > > int3 wrote:
> > > > hm why the need to replace `linkerInternal` with this? (Do we still need it if the `__mh_*` symbols are Defineds?)
> > > How about calling it `presentInSymbolTable`?
> > > 
> > > Calling it `linkerInternal` is weird because all of these are linker internal ... it's just some of them are not to be included in the symbol table
> > > 
> > Let me sleep on this... but naming aside, I'm also trying to understand if we need a tristate enum rather than a boolean
> Well we have "extern" to indicate external symbols that appear in the symbol table -- c.f. `privateExtern`. So the "internal" part is meant to contrast with that. but I agree that having "linker" in any variable name that's part of a linker is criminally redundant :P
> 
> How about `syntheticIntern`, or even just `internal`? I'm not a fan of LongJavaNamesThatSayWhatTheyDo, but if you think those are too cryptic, I'd also be fine with `includeInSymtab` ("include" seems to make more sense than "present" since nothing is present until we write out the binary.)
> 
> </bikeshed>
went with includeSymtab (because it's less cryptic than `internal`) Thanks!


================
Comment at: lld/MachO/SyntheticSections.cpp:1018
+void macho::createSyntheticSymbols(bool isPie) {
+  auto addSynSymMachHeader = [](const char *name, bool privateExtern = true) {
+    symtab->addSynthetic(name, in.header->isec, 0,
----------------
int3 wrote:
> `addSynSymMachHeader` is kind of a mouthful... how about `addHeaderSymbol`?
well no-one reads it out loud, but ok. :-)



================
Comment at: lld/MachO/SyntheticSections.cpp:1033
+      symtab->addSynthetic("__mh_execute_header",
+                           /*isec*/ nullptr, 0,
+                           /*privateExtern*/ false,
----------------
int3 wrote:
> From ld64's output, it looks like the absolute symbol will still have the address of the header. I guess we could support it by putting `in.header->addr` here, but we would need to move `createSyntheticSymbols` after `assignAddresses`.
> 
> (If it turns out to be uglier than that, I think it's fine that we don't have 100% parity; modern macOS binaries are all PIE, so this will be a very rare use case.)
I've added a FIXME instead. (assignAddresses() is in finalize....(), which is way later )


================
Comment at: lld/MachO/SyntheticSections.cpp:1049
+  // N_SECT symbols, even though the header is not part of any section
+  // and that they are private to the TU they are part of.
+  case MH_BUNDLE:
----------------
int3 wrote:
> not sure I follow this last part of the sentence. How are they private to a TU, when they only exist in the output binary?
Updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97007