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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> but it does 🤔 ... at least, the code looks like should. (it's not included in the symbol table though)

Ah I see -- we can link against it, but it doesn't appear in the final symbol table...

I'm trying to understand the significance of `__mh_execute_header` being defined while the others are undefined though. Why not make all of them Defined?

Also I think the tests still need updating :)



================
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:
> 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>


================
Comment at: lld/MachO/SyntheticSections.cpp:1034-1036
+#define addSynHelper(name)                                                     \
+  symtab->addSynthetic(name, in.header->isec, 0, /*privateExtern=*/false,      \
+                       SymbolTablePresence::ForcedAbscence)
----------------
this could be a lambda. We should avoid macros if possible


================
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:
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97007