[PATCH] D98381: [lld-macho] Handle error cases properly for -exported_symbol(s_list)

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1039
+          continue;
+      }
+      error("undefined symbol " + cachedName.val() +
----------------
unnecessary braces


================
Comment at: lld/MachO/SymbolTable.cpp:20-22
 Symbol *SymbolTable::find(StringRef name) {
-  auto it = symMap.find(CachedHashStringRef(name));
+  return find(CachedHashStringRef(name));
+}
----------------
this seems small enough to move into the header


================
Comment at: lld/MachO/SyntheticSections.cpp:604-607
+static bool shouldExportSymbol(const Defined *defined) {
+  if (defined->privateExtern)
+    return false;
+  return config->exportedSymbols.empty()
----------------
I'm wondering if this will be a performance bottleneck. If a build has mostly global symbols in the input but uses `-exported_symbols` to filter out most of them, then it would be better to set the value of `privateExtern` at parse time instead of calling `exportedSymbols.match()` more than once.

My measurements show that symbol ordering (which again looks up every symbol in a hashmap) is the biggest bottleneck when linking chromium_framework, so it wouldn't surprise me if this is worth optimizing.

We can punt on that for now though, but it would be good to leave a TODO


================
Comment at: lld/MachO/SyntheticSections.cpp:627
+    error("undefined symbol " + cachedName.val() +
+          "\n>>> referenced from option -exported_symbo(s_list)");
+  }
----------------
gkm wrote:
> thakis wrote:
> > Could we do this like `-u` handling in by adding Undefineds in the driver code instead of having a special case here? Then we wouldn't have to make the SymbolTable interface more complicated for this feature.
> I moved the check for undefined exported symbols the the same place where we check undefined `-u` options. It didn't make sense to lump exported symbols in with explicit undefined symbols because they have different error messages.
> 
> Open question: Should literals on the exported symbols list also be added to the symbol table as undefined symbols in order to pull definitions from archive inputs? It is unclear from my initial reading of ld64 sources. I'll look closer tomorrow and/or experiment.
Please file a task re the open question if you're not getting to it soon :) It's a good case to consider, let's not forget about it


================
Comment at: lld/test/MachO/export-options.s:30
+# PRIVATE: error: cannot export hidden symbol _private
+# PRIVATE-NEXT: >>> defined in
+
----------------
IMO there's no point in matching this if we're not also matching the filename (I'm fine with just removing this line)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98381