This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho]Allow exporting weak_def_can_be_hidden(AKA "autohide") symbols
ClosedPublic

Authored by oontvoo on Nov 3 2021, 10:21 PM.

Details

Summary
autohide symbols behaves similarly to private_extern symbols.
However, LD64 allows exporting autohide symbols. LLD currently does not.
This patch allows LLD to export them.

Diff Detail

Event Timeline

oontvoo created this revision.Nov 3 2021, 10:21 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Nov 3 2021, 10:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2021, 10:21 PM
oontvoo updated this revision to Diff 384762.Nov 4 2021, 8:24 AM

added test

oontvoo added a subscriber: thakis.Nov 4 2021, 8:35 AM

@thakis would you like to review this too? (related to discussions in D112977)
specifically, the part where weak-def-can-be-hiddens symbols got "promoted" to privateExtern... (this's side effect is that lld currently refuses to export it ...)

thakis added a comment.Nov 4 2021, 9:11 AM

That honestly sounds like a bug in your build config, independently of if ld64 allows it, no?

That honestly sounds like a bug in your build config, independently of if ld64 allows it, no?

I still think that LLD shouldn't conflate the following two cases:

// hidden.cc
extern __attribute__((weak)) __attribute__((visibility("hidden"))) void foo()  {return 5;}
int bar() { return foo(); }

VS

// can_be_hidden.cc (Must be C++, not C)
__attribute__((noinline)) inline int foo() { return 5; }
int bar() { return foo(); }

For the hidden.cc, clearly we shouldn't export foo. (current behaviour).
But for the latter, it should be fine

smeenai added a subscriber: smeenai.Nov 4 2021, 1:40 PM

That honestly sounds like a bug in your build config, independently of if ld64 allows it, no?

I still think that LLD shouldn't conflate the following two cases:

// hidden.cc
extern __attribute__((weak)) __attribute__((visibility("hidden"))) void foo()  {return 5;}
int bar() { return foo(); }

VS

// can_be_hidden.cc (Must be C++, not C)
__attribute__((noinline)) inline int foo() { return 5; }
int bar() { return foo(); }

For the hidden.cc, clearly we shouldn't export foo. (current behaviour).
But for the latter, it should be fine

In your can_be_hidden.cc example, why does LLD think foo is privateExtern to begin with? It's not marked that way in the object file (as confirmed with nm -m). That potentially seems like the actual issue here.

oontvoo added a comment.EditedNov 4 2021, 1:41 PM

In your can_be_hidden.cc example, why does LLD think foo is privateExtern to begin with? It's not marked that way in the object file (as confirmed with nm -m). That potentially seems like the actual issue here.

Because createDefined() in InputFiles "promote" it to privateExtern :)

P.S: link https://github.com/llvm/llvm-project/blob/48dc5c8e731b1198c4ba4b2f08b235c55e6d5aef/lld/MachO/InputFiles.cpp#L573

(it has explanation on why)

In your can_be_hidden.cc example, why does LLD think foo is privateExtern to begin with? It's not marked that way in the object file (as confirmed with nm -m). That potentially seems like the actual issue here.

Because createDefined() in InputFiles "promote" it to privateExtern :)

P.S: link https://github.com/llvm/llvm-project/blob/48dc5c8e731b1198c4ba4b2f08b235c55e6d5aef/lld/MachO/InputFiles.cpp#L573

(it has explanation on why)

Ah, got it. Took me a while to understand, because the combination of N_WEAK_DEF and N_WEAK_REF meaning autohide is incredibly confusing (sigh), but I get it now.

If I'm understanding your comment correctly now, you're saying that it's incorrect for LLD to conflate the autohide case with the private extern case, which I agree with you about.

After your patch, if I link your hidden.cc example with lld and add -exported_symbol __Z3foov, LLD is able to export the symbol, which seems incorrect. ld64 doesn't export the symbol, which makes sense to me.

Maybe we should be tracking autohide and private extern separately on symbols, to avoid this issue?

lld/test/MachO/exported-symbols.s
12 ↗(On Diff #384762)

You'll need either nm -g or objdump -macho --exports-trie to only print the exported symbols.

oontvoo abandoned this revision.Nov 5 2021, 7:56 AM

Ah, got it. Took me a while to understand, because the combination of N_WEAK_DEF and N_WEAK_REF meaning autohide is incredibly confusing (sigh), but I get it now.
If I'm understanding your comment correctly now, you're saying that it's incorrect for LLD to conflate the autohide case with the private extern case, which I agree with you about.
After your patch, if I link your hidden.cc example with lld and add -exported_symbol __Z3foov, LLD is able to export the symbol, which seems incorrect. ld64 doesn't export the symbol, which makes sense to me.

Yep, I've realised that now :)

Maybe we should be tracking autohide and private extern separately on symbols, to avoid this issue?

SG

oontvoo reclaimed this revision.Nov 5 2021, 7:56 AM

oops - meant to "Plan changes" (not abandon)

oontvoo planned changes to this revision.Nov 5 2021, 7:56 AM

That honestly sounds like a bug in your build config, independently of if ld64 allows it, no?

I still think that LLD shouldn't conflate the following two cases:

// hidden.cc
extern __attribute__((weak)) __attribute__((visibility("hidden"))) void foo()  {return 5;}
int bar() { return foo(); }

VS

// can_be_hidden.cc (Must be C++, not C)
__attribute__((noinline)) inline int foo() { return 5; }
int bar() { return foo(); }

For the hidden.cc, clearly we shouldn't export foo. (current behaviour).
But for the latter, it should be fine

You can make a case for this, but as far as I understand the only programs where this can make a difference in practice is one with ODR violations, right?

That honestly sounds like a bug in your build config, independently of if ld64 allows it, no?

I still think that LLD shouldn't conflate the following two cases:

// hidden.cc
extern __attribute__((weak)) __attribute__((visibility("hidden"))) void foo()  {return 5;}
int bar() { return foo(); }

VS

// can_be_hidden.cc (Must be C++, not C)
__attribute__((noinline)) inline int foo() { return 5; }
int bar() { return foo(); }

For the hidden.cc, clearly we shouldn't export foo. (current behaviour).
But for the latter, it should be fine

You can make a case for this, but as far as I understand the only programs where this can make a difference in practice is one with ODR violations, right?

If the copy is private, then I don't think it's strictly ODR violations.
But maybe two orthogonal points here:

  • is it correct to make autohide symbols synonym with private-extern? (agreed that it likely has no observable difference in the output)
  • is it correct to prevent autohide symbols from being exported? (From the example, there's no reason foo shouldn't be exportable - it's a bit weird, yes, but it's not wrong)
oontvoo updated this revision to Diff 385676.Nov 8 2021, 7:00 PM
oontvoo marked an inline comment as done.
  • propagated weak_def_can_be_hidden to symbols so we can distinguish it with the "real" private_extern, if needed.
  • consolidated tests with existing ones
oontvoo retitled this revision from [lld-macho] Allow exporting weak def private-extern symbols. to [lld-macho]Allow exporting weak_def_can_be_hidden(AKA "autohide") symbols.Nov 8 2021, 7:01 PM
oontvoo edited the summary of this revision. (Show Details)
int3 added a subscriber: int3.Nov 10 2021, 10:42 AM
int3 added inline comments.
lld/MachO/Driver.cpp
1471–1477

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?

lld/MachO/Symbols.h
163

hmm, this brings us to 9 bits of booleans :/

probably fine in practice, but maybe do a quick benchmark before landing this

lld/test/MachO/weak-def-can-be-hidden.s
45–46 ↗(On Diff #385676)

could we place this test in export-options.s?

oontvoo updated this revision to Diff 386383.Nov 10 2021, 5:48 PM
oontvoo edited the summary of this revision. (Show Details)

updated diff

lld/MachO/Driver.cpp
1471–1477

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.

int3 added inline comments.Nov 11 2021, 7:20 AM
lld/MachO/Driver.cpp
1471–1477

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.

lld/MachO/InputFiles.cpp
577–582

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)

oontvoo added inline comments.Nov 11 2021, 8:00 AM
lld/MachO/InputFiles.cpp
577–582

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? ....

oontvoo updated this revision to Diff 386530.Nov 11 2021, 8:47 AM

add tests for strong-private + autohide and weak-private + autohide

thevinster added inline comments.
lld/MachO/Driver.cpp
1467–1477

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?

lld/MachO/InputFiles.cpp
577–582

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;
lld/MachO/MarkLive.cpp
70 ↗(On Diff #386530)

Didn't we demote the symbol? so the || defined->weakDefCanBeHidden shouldn't be needed?

lld/test/MachO/export-options.s
138

nit: Can we move the check after all the RUN statements?

oontvoo marked 3 inline comments as done and an inline comment as not done.Nov 11 2021, 11:35 AM
oontvoo added inline comments.
lld/MachO/Driver.cpp
1467–1477

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)

1471–1477

Missed this earlier - right I agree it's confusing.

oontvoo updated this revision to Diff 386613.Nov 11 2021, 11:35 AM
oontvoo marked an inline comment as done.

updated diff

oontvoo planned changes to this revision.Nov 11 2021, 11:41 AM

reworking the flag flip to avoid confusions

oontvoo updated this revision to Diff 386690.EditedNov 11 2021, 4:58 PM
  • re-arrange the condition a bit.
  • also parallelize scanning the symbol tables in export/unexport-ing. This shows a small improvment in our internal apps:
x ./LLD_no_parallel.txt
+ ./LLD_with_parallel.txt
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|      +                                                                                                                                     x                        |
|+++   +++  +      +   +                                                                                          x                     x    x  x             x  x xxx|
| |_____MA______|                                                                                                                   |________________A________M______||
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  10         84.01         89.41         88.64        87.693     1.7424061
+  10          71.9         74.29         72.63        72.753    0.77734663
Difference at 95.0% confidence
	-14.94 +/- 1.26763
	-17.0367% +/- 1.44553%
	(Student's t, pooled s = 1.34912)
  • re-arrange the condition a bit.
  • also parallelize scanning the symbol tables in export/unexport-ing. This shows a small improvment in our internal apps:
x ./LLD_no_parallel.txt
+ ./LLD_with_parallel.txt
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|      +                                                                                                                                     x                        |
|+++   +++  +      +   +                                                                                          x                     x    x  x             x  x xxx|
| |_____MA______|                                                                                                                   |________________A________M______||
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x  10         84.01         89.41         88.64        87.693     1.7424061
+  10          71.9         74.29         72.63        72.753    0.77734663
Difference at 95.0% confidence
	-14.94 +/- 1.26763
	-17.0367% +/- 1.44553%
	(Student's t, pooled s = 1.34912)

Am I reading that table incorrectly, or do you consider a 17% improvement to be small? :)

lld/MachO/Symbols.h
163

It won't change the size of the struct, cos of padding. Turns out these structs are laid out suboptimally with regards to padding though; I'll put up a diff to fix that.

smeenai added inline comments.Nov 12 2021, 3:55 PM
lld/MachO/Symbols.h
163

x 10 84.01 89.41 88.64 87.693 1.7424061
+ 10 71.9 74.29 72.63 72.753 0.77734663
Difference at 95.0% confidence
-14.94 +/- 1.26763
-17.0367% +/- 1.44553%
(Student's t, pooled s = 1.34912)

Am I reading that table incorrectly, or do you consider a 17% improvement to be small? :)

No, 17% is correct :)

lld/MachO/Symbols.h
163

D113813

Thanks!

int3 accepted this revision.Nov 12 2021, 5:20 PM

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

lld/MachO/Driver.cpp
1468–1477

use braces; block comments should be treated as turning this if into a multiline statement

1469
1472–1473

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

lld/MachO/SymbolTable.cpp
102–104

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?

lld/test/MachO/export-options.s
216–217

ultra nit: on line 203 above you put private_extern after weak_def*, here you put it before. would prefer to be consistent :P

This revision is now accepted and ready to land.Nov 12 2021, 5:20 PM
oontvoo added inline comments.Nov 12 2021, 5:43 PM
lld/MachO/SymbolTable.cpp
102–104

good question - dunno why it was written this way.
so make both of them optional?

int3 added inline comments.Nov 12 2021, 5:44 PM
lld/MachO/SymbolTable.cpp
102–104

yeah sounds good

oontvoo updated this revision to Diff 386993.Nov 12 2021, 6:34 PM
oontvoo marked 8 inline comments as done.

addressed review comments

This revision was landed with ongoing or failed builds.Nov 12 2021, 6:57 PM
This revision was automatically updated to reflect the committed changes.