[PATCH] D103324: [lld/mac] Implement -dead_strip

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Nice work :D



================
Comment at: lld/MachO/Driver.cpp:930
 
+  config->deadStrip = args.hasArg(OPT_dead_strip);
+
----------------
can we put this together with the other config init lines around line 969?

edit: ah, I guess it's because Symbol's ctor uses it... a comment would be good. Although -- what do you think about always marking Symbols/InputSections as not-live and then have `isLive()` check for `!config->deadStrip`? I guess there'll be a bit more runtime overhead but not sure it'd be significant...

hm I see the other LLD ports have also implemented things this way :/


================
Comment at: lld/MachO/InputSection.h:51
 
-  // True if this InputSection could not be written to the output file.
   // With subsections_via_symbols, most symbol have its own InputSection,
   // and for weak symbols (e.g. from inline functions), only the
----------------



================
Comment at: lld/MachO/InputSection.h:59
+  bool isCoalescedWeak() const { return wasCoalesced && numRefs == 0; }
+  bool shouldOmitFromOutput() const { return !live || isCoalescedWeak(); }
+
----------------
it seems a bit counterintuitive to me that something can be live even though it has been coalesced away :) having live default to `false` (as mentioned in my comment in Driver.cpp) would avoid this issue too


================
Comment at: lld/MachO/MarkLive.cpp:27
+// from the final output.
+void markLive() {
+  ScopedTimer t(gctimer);
----------------
This is a pretty large function, can we break it up? Maybe have the seed-marking vs transitive-closure-marking as separate functions


================
Comment at: lld/MachO/MarkLive.cpp:28
+void markLive() {
+  ScopedTimer t(gctimer);
+
----------------
when should we use ScopedTimer over TimeTraceScope? also, I don't think we call Timer::print() anywhere...


================
Comment at: lld/MachO/MarkLive.cpp:107-108
+    // mod_init_funcs, mod_term_funcs sections
+    if ((isec->flags & SECTION_TYPE) == S_MOD_INIT_FUNC_POINTERS ||
+        (isec->flags & SECTION_TYPE) == S_MOD_TERM_FUNC_POINTERS) {
+      enqueue(isec);
----------------



================
Comment at: lld/MachO/MarkLive.cpp:123
+      isec->live = true;
+      const int compactUnwindEntrySize = 3 * target->wordSize +
+                                         sizeof(compact_unwind_encoding_t) +
----------------
I'd personally prefer `target->wordSize == 8 ? sizeof(CompactUnwindEntry<uint64_t>) : sizeof(CompactUnwindEntry<uint32_t>)`. More verbose but also more explicit


================
Comment at: lld/MachO/Symbols.cpp:43
+    // have to use the section's `live` bit as source of truth.
+    return d->isec ? d->isec->live : used;
+  }
----------------
seems clearer this way imo


================
Comment at: lld/MachO/Symbols.h:113
+
+  // True if an undefined or shared symbol is used from a live section.
+  bool used : 1;
----------------
nit for mach-o terminology consistency


================
Comment at: lld/test/MachO/dead-strip.s:11
+## Dead-stripped symbols should also not be in a map file output.
+# RUN: %lld -lSystem -dead_strip -map %t/map  -u _ref_private_extern_u \
+# RUN:     %t/basics.o -o %t/basics
----------------



================
Comment at: lld/test/MachO/dead-strip.s:27-37
+# EXEC-NEXT:   {{.*}} l {{.*}} _ref_data
+# EXEC-NEXT:   {{.*}} l {{.*}} _ref_local
+# EXEC-NEXT:   {{.*}} l {{.*}} _ref_from_no_dead_strip_globl
+# EXEC-NEXT:   {{.*}} l {{.*}} _no_dead_strip_local
+# EXEC-NEXT:   {{.*}} l {{.*}} _ref_from_no_dead_strip_local
+# EXEC-NEXT:   {{.*}} l {{.*}} _ref_private_extern_u
+# EXEC-NEXT:   {{.*}} l {{.*}} _main
----------------
nit: the first `{{.*}}` on each line is unnecessary


================
Comment at: lld/test/MachO/dead-strip.s:42
+# EXECDATA-LABEL: Contents of (__DATA,__ref_section) section
+# EXECDATA-NEXT: 04 00 00 00 00 00 00 00 05 00 00 00 00 00 00 00
+# EXECDATA-LABEL: Exports trie:
----------------



================
Comment at: lld/test/MachO/dead-strip.s:127-128
+# STRIPDYLIB-NEXT: {{.*}} *UND* _ref_dylib_fun
+# STRIPDYLIB:      Bind table:
+# STRIPDYLIB:      Lazy bind table:
+# STRIPDYLIB:      __DATA   __la_symbol_ptr {{.*}} flat-namespace _ref_undef_fun
----------------
is this line meant to check that no bindings are present in the bind table? I don't think it does the job... I think you want something like

```
STRIPDYLIB: Bind table:
STRIDYLIB-EMPTY:
STRIPDYLIB-NEXT: Lazy bind table:
```


================
Comment at: lld/test/MachO/dead-strip.s:213
+
+## If a dead stripped function has a strong ref to a dylibsymbol but
+## a live function only a weak ref, the dylib is still not a WEAK_DYLIB.
----------------



================
Comment at: lld/test/MachO/dead-strip.s:242-244
+# DEADWEAK: Weak bind table:
+# DEADWEAK: segment  section            address     type       addend   symbol
+# DEADWEAK-NOT: strong              _weak_in_dylib
----------------
nit: please align :D 


================
Comment at: lld/test/MachO/dead-strip.s:561
+## ld64 ignores this, but that doesn't look intentional. So lld honors it.
+# do better.
+.no_dead_strip
----------------



================
Comment at: lld/test/MachO/mh-header-link.s:12
 # RUN: llvm-mc %t/dylib.s -triple=x86_64-apple-macos10.15 -filetype=obj -o %t/dylib.o
-# RUN: %lld -pie -dylib %t/dylib.o -o %t/dylib.out
+# RUN: %lld -pie -dylib -dead_strip %t/dylib.o -o %t/dylib.out
 # RUN: llvm-objdump -m --syms %t/dylib.out | FileCheck %s --check-prefix DYLIB
----------------
why is this necessary?


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

https://reviews.llvm.org/D103324