[PATCH] D83532: [lld-macho] Partial support for weak definitions

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/ExportTrie.h:40
 using TrieEntryCallback =
-    llvm::function_ref<void(const llvm::Twine & /*name*/, uint64_t /*flags*/)>;
+    llvm::function_ref<void(const llvm::Twine & /*name*/, bool /*isWeakDef*/)>;
 
----------------
compnerd wrote:
> int3 wrote:
> > compnerd wrote:
> > > Are we sure we wont need any other flags?  I wonder if it's better to just treat weakness as a flag.  IIRC, there is a `EXPORT_SYMBOL_FLAGS_REEXPORT` and `EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL` that would be fairly good to account for.
> > Yeah I wasn't super sure about how to organize this. My reason for making it a bool is so that the trie-decoding-specific logic can all live in ExportTrie.cpp. But having a whole bunch of boolean parameters in a callback isn't the prettiest either. But I figure we can switch it back later if necessary as we support more flags (or think of a different, cleaner API)
> I'm pretty confident that we will need to add a few more flags at the very least.  IMO the migration from `uint64_t` to `bool` here is really a step backwards.
okay, I don't feel super strongly about this so I can revert


================
Comment at: lld/MachO/SymbolTable.cpp:52
+    }
+  }
 
----------------
compnerd wrote:
> int3 wrote:
> > compnerd wrote:
> > > What do you think of doing an early return instead if the symbol was inserted?
> > > 
> > > Can you explain why currently it is not an error if the symbol is not inserted and not defined?  Seems like a comment for that would be good.
> > not inserted and not defined => we will fall through to replaceSymbol below which will replace any Undefined/Lazy/Dylib symbols with the Defined one
> I think that my confusion here is that the not inserted implies that it already exists, but because the cast to Defined failed, it cannot be a defined symbol, so you are replacing it with an undefined symbol or or something else?  Do we know what is replacing the symbol?  If it is not defined, then why is this function called `addDefined`?
the cast to Defined failing means that the existing symbol isn't Defined, and so we're replacing it with a Defined symbol. I'm not sure why you think we may be replacing it with an undefined symbol, but note that `replaceSymbol` is changing the value and type of `s` via `placement new` (maybe you thought it was inserting `s`?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83532