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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
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()) {
----------------
int3 wrote:
> 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?
we should cover this with a test too


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

https://reviews.llvm.org/D123752