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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks!



================
Comment at: lld/MachO/SyntheticSections.cpp:613
+  error("cannot export hidden symbol " + symbolName + "\n>>> defined in " +
+        toString(defined->getFile()));
+  return false;
----------------
Should we error in this acccessor style function? Won't we get two errors if we call this for relocs and for symbol table?

nit: If you end strings after \n, clang-format formats them nicer: replace `"\n>>> defined in"` with `"\n" ">>> defined in"`

I'd find the diff easier to read if it just extracted the existing conditional out of the loop for now. Maybe this diag could move to a follow-on patch?


================
Comment at: lld/MachO/SyntheticSections.cpp:627
+    error("undefined symbol " + cachedName.val() +
+          "\n>>> referenced from option -exported_symbo(s_list)");
+  }
----------------
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.


================
Comment at: lld/test/MachO/export-options.s:55
+## Check that the export trie is shaped by a whitelist and then
+## by a blacklist. Both lists are designed to yield the same result.
+
----------------
https://en.wikipedia.org/wiki/Blacklist_(computing)#Controversy_over_use_of_the_term

Probably good to just use the actual flag names here (exported symbol list, unexported symbol list)


================
Comment at: lld/test/MachO/export-options.s:80
+# NM-DAG: non-external (was a private external) _hide_globl
+# NM-DAG: non-external (was a private external) _private
 
----------------
Should this have a -NOT for _hide_globl?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98381