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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

Nice perf win! But please break the parallelization into a separate diff...



================
Comment at: lld/MachO/Driver.cpp:1468
             if (defined->privateExtern) {
-              warn("cannot export hidden symbol " + symbolName +
-                   "\n>>> defined in " + toString(defined->getFile()));
+              if (defined->weakDefCanBeHidden)
+                // weak_def_can_be_hidden symbols behaves similarly to
----------------
use braces; block comments should be treated as turning this `if` into a multiline statement


================
Comment at: lld/MachO/Driver.cpp:1469
+              if (defined->weakDefCanBeHidden)
+                // weak_def_can_be_hidden symbols behaves similarly to
+                // private_extern symbols in most cases, except for when
----------------



================
Comment at: lld/MachO/Driver.cpp:1472-1473
+                // it is explicitly exported.
+                // LD64 allows exporting the former, but not the latter.
+                // So we do the same here.
+                defined->privateExtern = false;
----------------
I would just say that "the former can be exported but the latter cannot". It's pretty much implied that we are following ld64's behavior unless otherwise stated


================
Comment at: lld/MachO/SymbolTable.cpp:102-103
       isPrivateExtern, isThumb, isReferencedDynamically, noDeadStrip);
   defined->overridesWeakDef = overridesWeakDef;
+  defined->weakDefCanBeHidden = isWeakDefCanBeHidden;
   return defined;
----------------
I forgot why we chose not to have `overridesWeakDef` as part of the Defined constructor... is it very ugly to add an additional optional param instead of doing this assignment?


================
Comment at: lld/test/MachO/export-options.s:216-217
+.global _foo
+.private_extern _foo
+.weak_definition _foo
+_foo:
----------------
ultra nit: on line 203 above you put private_extern after weak_def*, here you put it before. would prefer to be consistent :P


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113167