[PATCH] D123752: [lld] Implement safe icf for MachO

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1510-1512
+      if (config->icfLevel == ICFLevel::safe) {
+        markAddrSigSymbols(inputFiles, &*symtab, &*config);
+      }
----------------
nit: can omit `if` braces


================
Comment at: lld/MachO/ICF.cpp:361-365
+  if (auto *d = dyn_cast_or_null<Defined>(s)) {
+    if (d->isec) {
+      d->isec->keepUnique = true;
+    }
+  }
----------------
can omit braces


================
Comment at: lld/MachO/ICF.cpp:370-371
+                               SymbolTable *symtab, Configuration *config) {
+  // Symbols in the dynsym could be address-significant in other executables
+  // or DSOs, so we conservatively mark them as address-significant.
+  for (Symbol *sym : symtab->getSymbols()) {
----------------
hmm if I understand correctly, the failure case we are trying to avoid here is that a DSO may export two symbols `a` and `b`, and whatever loads that DSO may end up comparing the addresses of `a` and `b` for equality? If that's the case I think any exported symbol in a DSO should be marked as address-significant. But... that seems like we would end up marking a lot of symbols :/

`referencedDynamically` is only used to mark symbols that shouldn't be stripped (and currently only applies to `__mh_execute_header`, as per its block comment); but stripped symbols can still have their addresses taken if they are in the export trie.

What does LLD-ELF do here?


================
Comment at: lld/MachO/ICF.cpp:373
+  for (Symbol *sym : symtab->getSymbols()) {
+    Defined *def = dyn_cast_or_null<Defined>(sym);
+    if (def && def->referencedDynamically) {
----------------
I think the `_or_null` isn't necessary, this should always be nonnull


================
Comment at: lld/MachO/ICF.cpp:380
+  for (InputFile *file : inputFiles) {
+    auto *obj = dyn_cast_or_null<ObjFile>(file);
+    if (!obj)
----------------



================
Comment at: lld/MachO/ICF.cpp:389-390
+
+    auto *subSection = &addrSigSection->subsections[0];
+    auto &contents = subSection->isec->data;
+
----------------
codebase convention is to avoid `auto` for simple types


================
Comment at: lld/MachO/ICF.cpp:427
                        isClassRefsSection(isec)) &&
+                      (isec->keepUnique == false) &&
                       !isec->shouldOmitFromOutput() &&
----------------



================
Comment at: lld/MachO/ICF.h:24
+void markAddrSigSymbols(llvm::SetVector<InputFile *> &inputFiles,
+                        SymbolTable *symtab, Configuration *config);
+void markSymAsAddrSig(Symbol *s);
----------------
`symtab` and `config` are globals in LLD; no need to pass them explicitly


================
Comment at: lld/MachO/InputSection.h:74
   bool isFinal = false;
+  bool keepUnique = false;
   uint32_t align = 1;
----------------
would be nice to have a comment here


================
Comment at: lld/test/MachO/icf-options.s:12
+# RUN: %lld -lSystem --icf=safe -o %t/safe %t/main.o 2>&1 \
+# RUN:     | FileCheck %s --check-prefix=DIAG-SAFE --allow-empty
 # RUN: not %lld -lSystem --icf=junk -o %t/junk %t/main.o 2>&1 \
----------------
can just reuse `DIAG-EMPTY` here, no need for `DIAG-SAFE`


================
Comment at: lld/test/MachO/icf-safe.s:16-18
+; ICFALL-LABEL: _main
+; ICFALL:  bl _func05
+; ICFALL-NEXT: bl _func05
----------------
nit: align


================
Comment at: lld/test/MachO/icf-safe.s:34-39
+  %0 = load i32, ptr @nr1
+  %1 = load i32, ptr @nr2
+  %2 = load i32, ptr @nr3
+  %mul = mul nsw i32 %2, %1
+  %add = add nsw i32 %mul, %0
+  ret i32 %add
----------------
we usually try to create test cases that are as minimal as possible. can this just be a `ret void`?


================
Comment at: lld/test/MachO/icf-safe.s:104-106
+attributes #0 = { minsize mustprogress nofree noinline norecurse nosync nounwind optsize readonly ssp willreturn uwtable "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="apple-a7" "target-features"="+aes,+crypto,+fp-armv8,+neon,+sha2,+v8a,+zcm,+zcz" }
+attributes #1 = { minsize nofree norecurse nounwind optsize ssp uwtable "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="apple-a7" "target-features"="+aes,+crypto,+fp-armv8,+neon,+sha2,+v8a,+zcm,+zcz" }
+attributes #2 = { minsize optsize }
----------------
ditto, I suppose we need `noinline` but can we omit the others?


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

https://reviews.llvm.org/D123752