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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked 3 inline comments as done and an inline comment as not done.
oontvoo 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 +
----------------
thevinster wrote:
> 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?
not sure it'd be better because that would mean have to do the check for *every* single autohide symbols. (whereas right now the cost is only paid by autohide symbols that are exported)



================
Comment at: lld/MachO/Driver.cpp:1478-1484
+              else
+                // weak_def_can_be_hidden symbols behaves similarly to
+                // private_extern symbols in most cases, except for when
+                // it is explicitly exported.
+                // LD64 allows exporting the former, but not the latter.
+                // So we do the same here.
+                defined->privateExtern = false;
----------------
int3 wrote:
> oontvoo wrote:
> > int3 wrote:
> > > This is basically undoing the promotion work in InputFiles.cpp, right? Would it be simpler if we didn't attempt that promotion, and just treated these two booleans as independent values?
> > > 
> > > Also, what happens when a symbol is marked both autohide and private_extern?
> > Well, it's a bit more convoluted than that (sorry!)
> > It's only undoing the promotion IF the symbol is in the exported list. (if not, it needs to stay privateExtern so we dont put it in the dynamic table).
> > However, if a symbol is both autohide and private-extern, then the autohide doesn't mean anything - it can't be exported either. (added the test)
> > 
> > We can keep the two flags independent in theory, but that means I'd have to go fix up all the places where privateExtern is treated to also take into account this extra flag, which (aside from being a bit of work) is unnecessary because as mentioned the symbol acts exactly like a privateExtern, only difference is that it can be exported.
> I was going to say that I guess it's only a bit awkward but then I read the new code in InputFiles.cpp and I changed my mind :P
> 
> I think it's super confusing: for one, `isWeakDefCanBeHidden == true` means that a privateExtern symbol can be *un*hidden. Also, to prevent that un-hiding from happening when both are set, we flip `isWeakDefCanBeHidden` inside `createDefined` because it is only in that function that we know that `privateExtern` *really* means `.private_extern`. I.e. it has different semantics within that function vs everywhere else.
Missed this earlier - right I agree it's confusing.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113167