[PATCH] D113167: [lld-macho]Allow exporting weak_def_can_be_hidden(AKA "autohide") symbols

[Vincent Lee via Phabricator via llvm-commits]

thevinster added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1467-1468
           if (config->exportedSymbols.match(symbolName)) {
             if (defined->privateExtern) {
-              warn("cannot export hidden symbol " + symbolName +
-                   "\n>>> defined in " + toString(defined->getFile()));
+              if (!defined->weakDefCanBeHidden)
+                warn("cannot export hidden symbol " + symbolName +
----------------
The demotion/promotion is bit hard to follow unless you stare at the code for a good while. Would it better to just have createDefined check if the symbol is in the exported list and skip the whole round trip of promoting and then demoting?


================
Comment at: lld/MachO/InputFiles.cpp:577-582
+    if (isWeakDefCanBeHidden) {
+      if (isPrivateExtern)
+        isWeakDefCanBeHidden = false;
+      else
+        isPrivateExtern = true;
+    }
----------------
oontvoo wrote:
> int3 wrote:
> > what happens if there is an existing symbol that's `isWeakDefCanBeHidden` and another one that's `isPrivateExtern`? I.e. what if those two properties are defined on different symbols with the same name?
> > 
> > (should also add a test that covers this)
> will add a test - but I'd think the privateExtern symbol would be picked over the weakdef one, which means it won't be exported (makes sense logically).
> 
> Now the question is, what if both are weak? ....
Nested ifs always is kinda making things a bit confusing to read. Not that it's significantly better, but I think hoisting the inner if and being explicit looks better (shows the actual combinations we are overriding)

```
if (isPrivateExtern && isWeakDefCanBeHidden)
  isWeakDefCanBeHidden = false;
else if (!isPrivateExtern && isWeakDefCanBeHidden)
  isPrivateExtern = true;
```


================
Comment at: lld/MachO/MarkLive.cpp:70
         // explicitUndefineds code below would handle this automatically.
-        assert(!defined->privateExtern &&
+        assert((!defined->privateExtern || defined->weakDefCanBeHidden) &&
                "should have been rejected by driver");
----------------
Didn't we demote the symbol? so the `|| defined->weakDefCanBeHidden` shouldn't be needed?


================
Comment at: lld/test/MachO/export-options.s:138
+# RUN: -o /dev/null  2>&1 | FileCheck %s --check-prefix=AUTOHIDE-PRIVATE
+# AUTOHIDE-PRIVATE: error: cannot export hidden symbol _foo
+
----------------
nit: Can we move the check after all the `RUN` statements?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113167