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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 4 inline comments as done.
gkm added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:613
+  error("cannot export hidden symbol " + symbolName + "\n>>> defined in " +
+        toString(defined->getFile()));
+  return false;
----------------
thakis wrote:
> 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?
* You are correct that an accessor should not error. In my limited testing, I don't see the message twice because the first error precludes writing the output where the second error might happen.

* With the proposed change, clang-format turns two lines onto four, which isn't so nice after all:
```
error("cannot export hidden symbol " + symbolName +
      "\n"
      ">>> defined in " +
      toString(defined->getFile()));
```


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


================
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
 
----------------
thakis wrote:
> Should this have a -NOT for _hide_globl?
`llvm-objdump --export-trie` only prints the exported symbols, so the `-NOT` is necessary to catch an unexpected line for `_hide_globl` as `TRIE-NOT`. By contrast, `llvm-nm -m` prints a line for every symbol to report its visibility, and we already have a pattern asserting as expected `non-external (was a private external)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98381