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

[Jez Ng via Phabricator via llvm-commits]

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

Thanks for getting through this surprisingly hairy feature (and putting up with my nits ;) )



================
Comment at: lld/MachO/SymbolTable.h:54
 
-  Defined *addSynthetic(StringRef name, InputSection *, uint32_t value,
-                        bool isPrivateExtern, bool isLinkerInternal);
+  Symbol *addSynthetic(StringRef name, InputSection *, uint32_t value,
+                       bool isPrivateExtern, bool includeInSymtab);
----------------
the return type can still be Defined


================
Comment at: lld/MachO/SyntheticSections.cpp:1005
+  switch (config->outputType) {
+    // FIXME: Assign the right addresse value for these symbols
+    // (rather than 0). But we need to do that after assignAddresses().
----------------



================
Comment at: lld/MachO/SyntheticSections.cpp:1022-1024
+  // The following symbols are
+  // N_SECT symbols, even though the header is not part of any section
+  // and that they are private to the bundle/dylib/object they are part of.
----------------
nit: formatting (line length)


================
Comment at: lld/MachO/SyntheticSections.cpp:1038
+  default:
+    break;
+  }
----------------
would be nice to add a `llvm_unreachable("unexpected output type")` here just to be safe


================
Comment at: lld/MachO/Writer.cpp:755-758
+      if (!seenMachHeader && isa<MachHeaderSection>(osec)) {
+        osec->index = 1;
+        seenMachHeader = true;
+      }
----------------
could just assign the `index = 1` in the constructor of the MachHeaderSection, no need to check for it in a loop here


================
Comment at: lld/test/MachO/mh_dylib_header.s:1-2
+## This tests that we can link against these synthetic symbols even
+## if they are not in the symbol table.
+
----------------
file naming nit: `mh_dylib_header.s` is a bit misleading since we're doing more than just testing that symbol. How about `mh-header-link.s` or `mh-header-resolution.s`?


================
Comment at: lld/test/MachO/mh_dylib_header.s:4
+
+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
----------------
nit: REQUIRES should be the first line in the file


================
Comment at: lld/test/MachO/mh_dylib_header.s:7-8
+
+## Test that in a dybib, we can link against __mh_dylib_header in a dylib
+## (but not in other type)
+# RUN: llvm-mc %t/dylib.s -triple=x86_64-apple-macos11.0 -filetype=obj -o %t/dylib.o
----------------
(no need to repeat "in a dylib")


================
Comment at: lld/test/MachO/mh_dylib_header.s:11
+# RUN: %lld -arch x86_64 -platform_version macOS 10 11 -dylib %t/dylib.o -o %t/dylib.out
+# RUN: llvm-objdump --m --syms %t/dylib.out | FileCheck %s --check-prefix DYLIB
+
----------------
I mean I guess both flags work but one-char flags typically use one dash


================
Comment at: lld/test/MachO/mh_dylib_header.s:13
+
+# RUN: not %lld -arch x86_64 -platform_version macOS 10 11 %t/dylib.o 2>&1 | FileCheck %s --check-prefix ERR-DYLIB
+
----------------
we typically add `-o /dev/null` so that if it unexpectedly passes we're not creating an `a.out` in a random directory. ditto for ERR-EXEC


================
Comment at: lld/test/MachO/mh_dylib_header.s:20
+
+## Test that in an execute, we can link against __mh_execute_header
+# RUN: llvm-mc %t/main.s -triple=x86_64-apple-macos11.0 -filetype=obj -o %t/exec.o
----------------



================
Comment at: lld/test/MachO/mh_dylib_header.s:22
+# RUN: llvm-mc %t/main.s -triple=x86_64-apple-macos11.0 -filetype=obj -o %t/exec.o
+# RUN: %lld -arch x86_64 -platform_version macOS 10 11  %t/exec.o -o %t/exec.out
+
----------------
The "-arch x86_64 -platform_version macOS 10 11" shouldn't be necessary since we are already defining that in lit.local.cfg

(same for other %lld invocations)


================
Comment at: lld/test/MachO/mh_dylib_header.s:33-35
+## FIXME: probably better to use
+## mov__mh_*_header at GOTPCREL(%rip), %rax
+## But we can't do it yet because of some GOT_LOAD-out of range error.
----------------
I think this is happening because we are linking a non-PIE, and due to the FIXME in SyntheticSections.cpp, LLD thinks that it is an absolute symbol at address 0, so the jump is too large (because it needs to cross `__PAGEZERO`). Whereas if it were actually pointing to the header, then it would be a much shorter jump.

I think we should just invoke `%lld -pie` and use GOTPCREL here, and skip testing the non-PIE case for now.

It would also be nice to check that the relocation resolves to the right address via `llvm-objdump -m -d`


================
Comment at: lld/test/MachO/mh_execute_header.s:1
+# REQUIRES: x86
+# RUN: rm -rf %t; mkdir %t
----------------
nit: mh-execute-header.s


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97007