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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo planned changes to this revision.
oontvoo added a comment.

In D97007#2632689 <https://reviews.llvm.org/D97007#2632689>, @int3 wrote:

> Hm, why is `__mh_execute_header` an undefined?
>
> It seems like it's a defined symbol to me:

Yes, my bad - I thought I was testing against ld64, but ended up using the older darwin port, which seemed to incorrectly implemented it as UNDEF.

> ~~Weirdly enough, when I swap out LLD for ld64 in our unit tests, it seems like load-command.s generates `__mh_execute_header` as an absolute symbol.~~ Aha, turns out that it's an absolute symbol only if we aren't linking a PIE.

No that's not weird - they <https://opensource.apple.com/source/cctools/cctools-466/include/mach-o/ldsyms.h> said the _execute_ is an absolute symbol.
The rest is N_SECT.

> Also, ld64 doesn't appear to emit `__mh_bundle_header`:

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



================
Comment at: lld/MachO/Driver.cpp:1050
+                       /*isDefined*/ false);
+      switch (config->outputType) {
+      case MH_BUNDLE:
----------------
carlokok wrote:
> Would there be a chance of having a __mh_*_header that works for all targets, while this gets added anyway? something like __mh_header?
I'm not sure I understand this question.

Are you asking if they would introduce a new symbol to replace all this?
(if so, I don't know :)  probably not given each of these symbol is unique to each file type )

Or are you asking/suggesting that in this implementation, we would make an `mh_header` class and move this code there?
If so, sure. I've moved the synthetic-symbols generations out-of-line for readability. 




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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97007