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.
Details
- Reviewers
gkm smeenai thakis int3 - Group Reviewers
Restricted Project - Commits
- rG9b29dae3cae1: [lld-macho] Allow exporting weak_def_can_be_hidden(AKA "autohide") symbols
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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 :)
(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. |
Yep, I've realised that now :)
Maybe we should be tracking autohide and private extern separately on symbols, to avoid this issue?
SG
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)
- 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
lld/MachO/Driver.cpp | ||
---|---|---|
1478–1484 | 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 | could we place this test in export-options.s? |
updated diff
lld/MachO/Driver.cpp | ||
---|---|---|
1478–1484 | Well, it's a bit more convoluted than that (sorry!) 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. |
lld/MachO/Driver.cpp | ||
---|---|---|
1478–1484 | 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 | ||
570–575 | 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) |
lld/MachO/InputFiles.cpp | ||
---|---|---|
570–575 | 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? .... |
lld/MachO/Driver.cpp | ||
---|---|---|
1474–1484 | 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 | ||
570–575 | 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 | Didn't we demote the symbol? so the || defined->weakDefCanBeHidden shouldn't be needed? | |
lld/test/MachO/export-options.s | ||
138 ↗ | (On Diff #386530) | nit: Can we move the check after all the RUN statements? |
- 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. |
Nice perf win! But please break the parallelization into a separate diff...
lld/MachO/Driver.cpp | ||
---|---|---|
1475–1484 | use braces; block comments should be treated as turning this if into a multiline statement | |
1476 | ||
1479–1480 | 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 | ||
98–100 | 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 ↗ | (On Diff #386690) | ultra nit: on line 203 above you put private_extern after weak_def*, here you put it before. would prefer to be consistent :P |
lld/MachO/SymbolTable.cpp | ||
---|---|---|
98–100 | good question - dunno why it was written this way. |
lld/MachO/SymbolTable.cpp | ||
---|---|---|
98–100 | yeah sounds good |