[PATCH] D113167: [lld-macho]Allow exporting weak_def_can_be_hidden(AKA "autohide") symbols
Vincent Lee via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 11 10:30:36 PST 2021
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
More information about the llvm-commits
mailing list