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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd 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*/)>;
 
----------------
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.


================
Comment at: lld/MachO/SymbolTable.cpp:52
+    }
+  }
 
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83532