[PATCH] D76908: [lld-macho] Add support for emitting dylibs with a single symbol

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/SyntheticSections.cpp:142
+  for (const Symbol *sym : symtab->getSymbols())
+    if (auto *defined = dyn_cast<Defined>(sym))
+      exported.push_back(defined);
----------------
smeenai wrote:
> This can be done in a follow-up, but we should be checking if the symbol has hidden visibility before exporting it, right?
yeah probably. I'll add a TODO


================
Comment at: lld/MachO/SyntheticSections.h:26
 constexpr const char *pageZero = "__pagezero";
+constexpr const char *export_ = "__export";
 constexpr const char *stringPool = "__string_pool";
----------------
smeenai wrote:
> Nit: why the trailing underscore?
`export` is a reserved keyword in C++ :)


================
Comment at: lld/include/lld/Common/Sort.h:1
+//===- Sort.h ---------------------------------------------------*- C++ -*-===//
+//
----------------
smeenai wrote:
> Was this meant to be part of a previous diff?
ugh, rebasing is hard


================
Comment at: lld/test/MachO/dylib.s:6
+# RUN:   %t.o -o %t.dylib
+# CHECK:     cmd: LC_ID_DYLIB
+# CHECK-NOT: cmd:
----------------
smeenai wrote:
> Where's the corresponding FileCheck for these?
oops


================
Comment at: lld/test/MachO/dylib.s:10
+
+## Check that -e is a no-op when -dylib is also passed.
+# RUN: lld -flavor darwinnew -dylib %t.o -o %t.dylib -e missing_entry
----------------
smeenai wrote:
> Which part of the test is checking this?
The line directly below -- it's checking that lld doesn't error out even when passed `-e missing_entry`. I'll elaborate the comment.


================
Comment at: lld/test/MachO/dylib.s:12
+# RUN: lld -flavor darwinnew -dylib %t.o -o %t.dylib -e missing_entry
+# RUN: obj2yaml %t.dylib | FileCheck %s -DOUTPUT=%t.dylib --check-prefix=DEFAULT-INSTALL-NAME
+# DEFAULT-INSTALL-NAME:     cmd: LC_ID_DYLIB
----------------
smeenai wrote:
> Is it possible to test these via objdump or readobj instead of obj2yaml? That's more conventional in LLD tests, and their output is probably a bit more stable.
Yeah we can use `llvm-objdump --macho --dylib-id`. While checking that out I realize we can look for load commands in general using `llvm-objdump --macho --all-headers`, so `load-commands.s` can use that too and the test suite can be entirely free of obj2yaml


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76908