[lld-macho] Allow dead_strip to work with exported private extern symbols

[thevinster]

Summary

It seems like we are overly asserting when running -dead_strip with
exported symbols. ld64 treats exported private extern symbols as a liveness
root. Loosen the assert to match ld64's behavior.

[thakis]

lld/MachO/MarkLive.cpp

235

Should we call addSym for privateExterns? That seems wrong (?)

[oontvoo]

lld/MachO/MarkLive.cpp

235

Should we call addSym for privateExterns

normally we shouldn't but if a symbol is both private extern and autohide, then i think it should be ok,

relatedly, stray comment should be deleted or moved?

[thevinster]

lld/MachO/MarkLive.cpp

235

I thought the comment was referring to the addSym, but I don't think it will be changed so I'm happy to remove it.

if a symbol is both private extern and autohide, then i think it should be ok,

This is what I found from testing as well. Happy to re-add the assert back and only skip it if it is both privateExtern and autohide. I think that's the one I'm fixing in my local builds.

[thevinster]

lld/MachO/MarkLive.cpp

235

So with some further testing, this is looking fairly strange. I think ld64 is just preserving all the symbols that are exported even if they cannot be exported. It does feel strange to preserve the symbol if it's privateExtern... I can skip "unable to be exported syms" from being marked live which seems like the correct behavior, but this will deviate from the ld64's behavior. What do y'all think should be the correct approach here?

@int3 @oontvoo @thakis

[oontvoo]

lld/MachO/MarkLive.cpp

235

I thought the comment was referring to the addSym, but I don't think it will be changed so I'm happy to remove it.

The comment was referring to the fact that we shouldn't see "privateExtern" on a symbol from symtab and that such check could have been done in Driver (when we process the exported symbols).
Now that you're removing the assert, the comment can either move to Driver or just delete it.

(btw, the comment was lacking the case where a symbol is both privateExtern and autohide)

[thevinster]

lld/MachO/MarkLive.cpp

235

We have some pretty convoluted logic with privateExterns and autohides. For symbols that are both privateExterns and autohide, we end up nullifying the flag. We've effectively removed lost that information and can't distinguish if a particular symbol was originally privateExtern and autohide which makes checking for that case a harder. Although, from empirical testing w/ ld64, it looks like it just preserves them when symbols from the exported list cannot be exported.

[int3]

lld/MachO/MarkLive.cpp

235

it looks like ld64 does treat a private extern symbol as a liveness root if it's exported, regardless of whether it is autohide. Let's just leave a comment to that effect and follow ld64's behavior. It doesn't cost us in terms of code complexity and it is one less thing for new adopters of LLD to worry about

lld/test/MachO/export-options.s

148–149

it's better to describe what the test should cover (exported-hidden) rather than what it shouldn't do (no_crash). (The comment itself can note that this used to crash.)

let's also mention explicitly that we expect _foo to be treated as a liveness root due to -exported_symbol

also nit: use hyphens instead of underscores for the file names

[int3]

lld/test/MachO/export-options.s

148–149

or maybe AUTOHIDE-PRIVATE-DEAD-STRIP to make it clear that it's an extension to the AUTOHIDE-PRIVATE cases above

[thevinster]

Fixed up comments and renaming

[int3]

Thanks!

lld/MachO/MarkLive.cpp

231–234

maybe something like "NOTE: we are purposely not checking for privateExtern here because ..."

could also mention that exporting private externs is technically an ill-defined operation, so we are just following ld64's behavior

232

lld/MachO/MarkLive.cpp

void markLive() {
   // Add GC roots.
   if (config->entry)
     marker->addSym(config->entry);
   for (Symbol *sym : symtab->getSymbols()) {
     if (auto *defined = dyn_cast<Defined>(sym)) {
       // -exported_symbol(s_list)
       if (!config->exportedSymbols.empty() &&
           config->exportedSymbols.match(defined->getName())) {
-        // FIXME: Instead of doing this here, maybe the Driver code doing
-        // the matching should add them to explicitUndefineds? Then the
-        // explicitUndefineds code below would handle this automatically.
-        assert(!defined->privateExtern &&
-               "should have been rejected by driver");
+        // NOTE: Even though exporting private externs is an ill-defined
+        // operation, we are purposely not checking for privateExtern in
+        // order to follow ld64's behavior of treating all exported private
+        // extern symbols as live, irrespective of whether they are autohide.
         marker->addSym(defined);
         continue;
       }
 
       // public symbols explicitly marked .no_dead_strip
       if (defined->referencedDynamically || defined->noDeadStrip) {
         marker->addSym(defined);
         continue;
       }

lld/test/MachO/export-options.s

 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
 # RUN:     %t/weak-private-extern.s -o %t/weak-private-extern.o
 ## Test that we can export the autohide symbol but not when it's also
 ## private-extern
 # RUN: %lld -dylib -exported_symbol "_foo" %t/autohide.o -o %t/exp-autohide.dylib
 # RUN: llvm-nm -g %t/exp-autohide.dylib | FileCheck %s --check-prefix=EXP-AUTOHIDE
 
 # RUN: not %lld -dylib -exported_symbol "_foo" %t/autohide-private-extern.o \
-# RUN: -o /dev/null  2>&1 | FileCheck %s --check-prefix=AUTOHIDE-PRIVATE
+# RUN:   -o /dev/null  2>&1 | FileCheck %s --check-prefix=AUTOHIDE-PRIVATE
 
 # RUN: not %lld -dylib -exported_symbol "_foo" %t/autohide.o \
 # RUN:   %t/glob-private-extern.o -o /dev/null 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=AUTOHIDE-PRIVATE
 
 # RUN: not %lld -dylib -exported_symbol "_foo" %t/autohide.o \
 # RUN:   %t/weak-private-extern.o -o /dev/null 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=AUTOHIDE-PRIVATE
 
+## Test that exported hidden symbols are still treated as a liveness root.
+## This previously used to crash when enabling -dead_strip since it's unconventional
+## to add treat private extern symbols as a liveness root.
+# RUN: %no-fatal-warnings-lld -dylib -exported_symbol "_foo" %t/autohide-private-extern.o \
+# RUN:   -dead_strip -o %t/exported-hidden
+# RUN: llvm-nm -m %t/exported-hidden | FileCheck %s --check-prefix=AUTOHIDE-PRIVATE-DEAD-STRIP
+
+# RUN: %no-fatal-warnings-lld -dylib -exported_symbol "_foo" %t/autohide.o \
+# RUN:   -dead_strip %t/glob-private-extern.o -o %t/exported-hidden
+# RUN: llvm-nm -m %t/exported-hidden | FileCheck %s --check-prefix=AUTOHIDE-PRIVATE-DEAD-STRIP
+
+# RUN: %no-fatal-warnings-lld -dylib -exported_symbol "_foo" %t/autohide.o \
+# RUN:   -dead_strip %t/weak-private-extern.o -o %t/exported-hidden
+# RUN: llvm-nm -m %t/exported-hidden | FileCheck %s --check-prefix=AUTOHIDE-PRIVATE-DEAD-STRIP
+
 # EXP-AUTOHIDE: T _foo
 # AUTOHIDE-PRIVATE: error: cannot export hidden symbol _foo
+# AUTOHIDE-PRIVATE-DEAD-STRIP: (__TEXT,__text) non-external (was a private external) _foo
 
 #--- default.s
 
 .globl _keep_globl, _hide_globl
 _keep_globl:
   retq
 _hide_globl:
   retq