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

Wed Jun 2 05:48:39 PDT 2021

thakis added inline comments.

================
Comment at: lld/MachO/Driver.cpp:930

+  config->deadStrip = args.hasArg(OPT_dead_strip);
+
----------------
int3 wrote:
> 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 :/
Added a comment.

Right, it's for consistency with the other LLD ports. I had the explicit check in isLive first but I moved to this for consistency with the other LLDs and for some other reason I no longer remember. Maybe `used` in Symbol needed an accessor for reading then? Not sure that was it. It felt marginally nicer this way :)

================
Comment at: lld/MachO/InputSection.h:59
+  bool isCoalescedWeak() const { return wasCoalesced && numRefs == 0; }
+  bool shouldOmitFromOutput() const { return !live || isCoalescedWeak(); }
+
----------------
int3 wrote:
> 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
If dead stripping is on, all coalesced weak InputSections should also be marked !live.

If dead stripping is not on, then yes, this can happen. It's a bit strange, but it matches the other ports.

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

"""
I first based this on the COFF
implementation, but later realized that things are more similar to ELF.
I think it'd be good to refactor MarkLive.cpp to look more like the ELF
part at some point, but I'd like to get a working state checked in first.
"""

The ELF port has a class with member functions and stuff, and I agree that the root collecting code should be its own function. If it's ok I'd like to iterate on this in-tree though.

(I also have a local branch that implements `-why_live` … actually let me upload that, D103517 … but implementing it the same way ld64 did is too expensive perf wise, so I don't know yet if I need to templatize this on the state bit or made dead_strip independent of why_live or what. And finally, it feels like several pieces of code walk all files just to iterate all local symbols, so maybe I'd to pull the `localSymbols` computation out of SyntheticSections.cpp and then use that in the several places that use it. And speaking of local symbols, the order file processing currently walks all symbols, exported and local, and checks each against the order file, even though if the order file is short and only contains public symbols it should be way faster to walk the order file and look each symbol up in the symbol table instead, then it's small-constant-times-amortized-linear-of-large instead of large-constant-times-amortized-linear-of-small.)

Sorry about the wall of text! Summary: I agree I should do this, but I'd like to do it after getting the first draft committed :)

================
Comment at: lld/MachO/MarkLive.cpp:28
+void markLive() {
+  ScopedTimer t(gctimer);
+
----------------
int3 wrote:
> when should we use ScopedTimer over TimeTraceScope? also, I don't think we call Timer::print() anywhere...
Good catch. Apparently the COFF port uses ScopedTimer (for its fancy `/time` feature), while ELF and MachO use TimeTraceScope (for their fancy trace export). I think I should change TimeTraceScope to also be able to print time reports, then switch COFF to that so that it grows a fancy trace export, and then give the ELF and MachO ports flags for a time report on the console. (For MachO, I guess that'd go behind `-print_statistics` which already exists.)

For now, changed this to TimeTraceScope.

================
Comment at: lld/MachO/MarkLive.cpp:123
+      isec->live = true;
+      const int compactUnwindEntrySize = 3 * target->wordSize +
+                                         sizeof(compact_unwind_encoding_t) +
----------------
int3 wrote:
> I'd personally prefer `target->wordSize == 8 ? sizeof(CompactUnwindEntry<uint64_t>) : sizeof(CompactUnwindEntry<uint32_t>)`. More verbose but also more explicit
Also requires putting CompactUnwindEntry in the header. But sure, done.

================
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
----------------
int3 wrote:
> 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:
> ```
IIRC the `--implicit-check-not _unref` does most of the work and this here just checks that I remembered to pass `--bind`

================
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
----------------
int3 wrote:
> why is this necessary?
Good question. I think it was test coverage for making sure createSyntheticSymbols() works with dead stripping. I think in the end it just worked without any changes, but I figured it's still nice to have the coverage.

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

https://reviews.llvm.org/D103324