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

[Jez Ng via Phabricator via llvm-commits]

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:
> 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)


================
Comment at: lld/MachO/InputFiles.cpp:235
+      return make<Defined>(name, isec, value, sym.n_desc & N_WEAK_DEF);
   };
 
----------------
compnerd wrote:
> It's okay if you want to use braces, but please use them on both sides.  However, I think that this is better written as:
> 
> ```
>   if (sym.n_type & N_EXT)
>     return symtab->addDefined(name, isec, value, sym.n_desc & N_WEAK_DEF);
>   return make<Defined>(name, isec, value, sym.n_desc & N_WEAK_DEF);
> ```
oh yeah the braces were accidentally left over after some editing


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83532