[lld-macho] Support -non_global_symbols_strip_list, -non_global_symbols_no_strip_list, -x

[oontvoo]

Summary

PR/55600

[oontvoo]

more tests

[oontvoo]

add tests for interaction with -exported_symbol

[MaskRay]

lld/MachO/Config.h

97

This is for local symbols so the name can be more specific. One idea is

enum class LocalSymbolPolicy {
  All,
  None,
  IncludeSelected,
  ExcludeSelected,
};

lld/MachO/Driver.cpp

1443

FWIW ld.lld uses error: ... and ... may not be used together

lld/MachO/SyntheticSections.cpp

708

Drop formatting

966

It seems simpler to use bool(Symbol *) and let the for loop call addSymbol(localSymbols, sym);

[oontvoo]

lld/MachO/Driver.cpp

1443

why?

This was worded this way to be consistent with the error message w.r.t exported_symbol (right a few lines above) AND LD64's error messages.

lld/MachO/SyntheticSections.cpp

966

I dont think that'd be simpler because all the callers would have to repeat the same check.

[oontvoo]

undo formatting changes

[oontvoo]

hi, friendly 🔔 :)

[oontvoo]

consolidate exclude/included patterns set into one

[thevinster]

lld/MachO/Driver.cpp

1421

nit: variable name seems clearer if it's named includeLocal/excludeLocal.

lld/MachO/SyntheticSections.cpp

971

Does this case need to be enumerated if it's already being filtered out below?

lld/test/MachO/local-symbol-output.s

74

Can we also have a test for interactions with -unexported_symbol as well? Specifically, that a global doesn't get included in the symbol table?

[oontvoo]

rename variables per suggestion

lld/MachO/SyntheticSections.cpp

971

Does this case need to be enumerated if it's already being filtered out below?

Yes, because we don't need to do the check on line 1009 and line 1019. We only do that check on line 991+ is because iterating through all the inputfiles and their symbols is expensive - hence if we can avoid it (ie. when -x is set), then we should

lld/test/MachO/local-symbol-output.s

74

I'm not sure I understand the test you're proposing. Can you give an example?
Specifically, -x and -non_global_symbols_[no_]strip_list options don't have an effect on global symbols. So how would using them with -unexported_symbol be meaningful?

[thevinster]

lld/MachO/Driver.cpp

1416–1417

Just to check here, does ld64 also do the same here? Wondering if this specific to our implementation or inheriting behavior for ld64. I do agree it's confusing and maybe we should disallow it if no one actually does this in practice.

lld/MachO/SyntheticSections.cpp

971

Whoops missed that part. Thanks for clarifying

lld/test/MachO/local-symbol-output.s

74

Right, -x and -non_global_symbols_[no_]strip_list don't matter to global symbols but unexported symbols can make them non-global. For example,

.globl _foo
.globl _bar
_foo:
  ret
_bar:
  ret

What I'm thinking is if _foo in the test case above is unexported with -x, I would to expect to see only _bar in the symbol table since -x would've stripped away _foo.

We can also apply this to -non_global_symbols_strip_list and non_global_symbols_no_strip_list well by checking that the unexported symbol should've been stripped and not stripped from the symbol table, respectively.

[oontvoo]

lld/MachO/Driver.cpp

1416–1417

Yes, that's from LD64. People may not do it on purpose but perhaps generated link commands do this.

I'd slightly prefer we error-ing out for simplicity but given the behaviour is not too complex to imitate, I don't feel strongly about it, hence the TODO note.

Does anyone else have a strong opinion on this? Otherwise, happy to just delete the TODO and be done with this edge case :)

lld/test/MachO/local-symbol-output.s

74

added tests

[oontvoo]

added additional tests

[thevinster]

LGTM

[oontvoo]

rebase

[oontvoo]

Thanks!

[int3]

Sorry for the belated review. Took me a little longer than expected to get back in the swing of things :)

lld/MachO/Driver.cpp

941

I would prefer to avoid naming things "helper" if there is a more descriptive name available. E.g. something like parseSymbolPatternsFile seems like a good fit here

956

nit: insert line break before functions for easy vim { } movement :p

1418–1419

did you have a build that used both together? IMO we should prefer implementation simplicity by default in cases where ld64's behavior is questionable

1429

extra newline

lld/MachO/SyntheticSections.cpp

971

seems worthwhile to explain this in a comment (see below)

990

could state explicitly that localSymbolsHandler will do the right thing regardless, but that this is a perf optimization

1018–1019

I think we lack a test case for this private extern code path

[oontvoo]

addressed in https://reviews.llvm.org/D126792

lld/MachO/Driver.cpp

1418–1419

Yeah - can we keep this for now? (I'll find out from the builds' owner whether the usage was intentional or if it could be cleaned up).

lld/MachO/Config.h

public:
   bool empty() const { return literals.empty() && globs.empty(); }
   void clear();
   void insert(llvm::StringRef symbolName);
   bool matchLiteral(llvm::StringRef symbolName) const;
   bool matchGlob(llvm::StringRef symbolName) const;
   bool match(llvm::StringRef symbolName) const;
 };
 
+enum class SymtabPresence {
+  All,
+  None,
+  SelectivelyIncluded,
+  SelectivelyExcluded,
+};
+
 struct Configuration {
   Symbol *entry = nullptr;
   bool hasReexports = false;
   bool allLoad = false;
   bool applicationExtension = false;
   bool archMultiple = false;
   bool exportDynamic = false;
   bool forceLoadObjC = false;
struct Configuration {
 
   SectionRenameMap sectionRenameMap;
   SegmentRenameMap segmentRenameMap;
 
   SymbolPatterns exportedSymbols;
   SymbolPatterns unexportedSymbols;
   SymbolPatterns whyLive;
 
+  SymtabPresence localSymbolsPresence = SymtabPresence::All;
+  SymbolPatterns localSymbolPatterns;
+
   bool zeroModTime = false;
 
   llvm::StringRef osoPrefix;
 
   llvm::MachO::Architecture arch() const { return platformInfo.target.Arch; }
 
   llvm::MachO::PlatformType platform() const {
     return platformInfo.target.Platform;

lld/MachO/Driver.cpp

if (glob.match(symbolName))
       return true;
   return false;
 }
 
 bool SymbolPatterns::match(StringRef symbolName) const {
   return matchLiteral(symbolName) || matchGlob(symbolName);
 }
 
-static void handleSymbolPatterns(InputArgList &args,
-                                 SymbolPatterns &symbolPatterns,
-                                 unsigned singleOptionCode,
-                                 unsigned listFileOptionCode) {
-  for (const Arg *arg : args.filtered(singleOptionCode))
-    symbolPatterns.insert(arg->getValue());
-  for (const Arg *arg : args.filtered(listFileOptionCode)) {
-    StringRef path = arg->getValue();
-    Optional<MemoryBufferRef> buffer = readFile(path);
-    if (!buffer) {
-      error("Could not read symbol file: " + path);
-      continue;
-    }
-    MemoryBufferRef mbref = *buffer;
-    for (StringRef line : args::getLines(mbref)) {
-      line = line.take_until([](char c) { return c == '#'; }).trim();
-      if (!line.empty())
-        symbolPatterns.insert(line);
-    }
-  }
+static void handleSymbolPatternsListHelper(const Arg *arg,
+                                           SymbolPatterns &symbolPatterns) {
+    return;
+static void handleSymbolPatterns(InputArgList &args,
+                                 SymbolPatterns &symbolPatterns,
+                                 unsigned singleOptionCode,
+                                 unsigned listFileOptionCode) {
+  for (const Arg *arg : args.filtered(singleOptionCode))
+    symbolPatterns.insert(arg->getValue());
+  for (const Arg *arg : args.filtered(listFileOptionCode))
+    handleSymbolPatternsListHelper(arg, symbolPatterns);
 }
 
 static void createFiles(const InputArgList &args) {
   TimeTraceScope timeScope("Load input files");
   // This loop should be reserved for options whose exact ordering matters.
   // Other options should be handled via filtered() and/or getLastArg().
   bool isLazy = false;
   for (const Arg *arg : args) {
handleSymbolPatterns(args, config->exportedSymbols, OPT_exported_symbol,
                        OPT_exported_symbols_list);
   handleSymbolPatterns(args, config->unexportedSymbols, OPT_unexported_symbol,
                        OPT_unexported_symbols_list);
   if (!config->exportedSymbols.empty() && !config->unexportedSymbols.empty()) {
     error("cannot use both -exported_symbol* and -unexported_symbol* options\n"
           ">>> ignoring unexports");
     config->unexportedSymbols.clear();
   }
+
+  // Imitating LD64's:
+  // -non_global_symbols_no_strip_list and -non_global_symbols_strip_list can't
+  // both be present.
+  // But -x can be used with either of these two, in which case, the last arg
+  // takes effect.
+  // (TODO: This is kind of confusing - considering disallowing using them
+  // together for a more straightforward behaviour)
+  {
+    bool includeLocal = false;
+    bool excludeLocal = false;
+    for (const Arg *arg :
+         args.filtered(OPT_x, OPT_non_global_symbols_no_strip_list,
+                       OPT_non_global_symbols_strip_list)) {
+      switch (arg->getOption().getID()) {
+      case OPT_x:
+        config->localSymbolsPresence = SymtabPresence::None;
+
+        break;
+      case OPT_non_global_symbols_no_strip_list:
+        if (excludeLocal) {
+          error("cannot use both -non_global_symbols_no_strip_list and "
+                "-non_global_symbols_strip_list");
+        } else {
+          includeLocal = true;
+          config->localSymbolsPresence = SymtabPresence::SelectivelyIncluded;
+          handleSymbolPatternsListHelper(arg, config->localSymbolPatterns);
+        }
+        break;
+      case OPT_non_global_symbols_strip_list:
+        if (includeLocal) {
+          error("cannot use both -non_global_symbols_no_strip_list and "
+                "-non_global_symbols_strip_list");
+        } else {
+          excludeLocal = true;
+          config->localSymbolsPresence = SymtabPresence::SelectivelyExcluded;
+          handleSymbolPatternsListHelper(arg, config->localSymbolPatterns);
+        }
+        break;
+      default:
+        llvm_unreachable("unexpected option");
+      }
+    }
+  }
   // Explicitly-exported literal symbols must be defined, but might
-  // languish in an archive if unreferenced elsewhere. Light a fire
-  // under those lazy symbols!
+  // languish in an archive if unreferenced elsewhere or if they are in the
+  // non-global strip list. Light a fire under those lazy symbols!
   for (const CachedHashStringRef &cachedName : config->exportedSymbols.literals)
     symtab->addUndefined(cachedName.val(), /*file=*/nullptr,
                          /*isWeakRef=*/false);
 
   for (const Arg *arg : args.filtered(OPT_why_live))
     config->whyLive.insert(arg->getValue());
   if (!config->whyLive.empty() && !config->deadStrip)
     warn("-why_live has no effect without -dead_strip, ignoring");

lld/MachO/Options.td

 
 def grp_symtab : OptionGroup<"symtab">, HelpText<"SYMBOL TABLE">;
 
 def S : Flag<["-"], "S">,
     HelpText<"Strip debug information (STABS or DWARF) from the output">,
     Group<grp_symtab>;
 def x : Flag<["-"], "x">,
     HelpText<"Exclude non-global symbols from the output symbol table">,
-    Flags<[HelpHidden]>,
     Group<grp_symtab>;
 def non_global_symbols_strip_list : Separate<["-"], "non_global_symbols_strip_list">,
     MetaVarName<"<path>">,
     HelpText<"Specify in <path> the non-global symbols that should be removed from the output symbol table">,
-    Flags<[HelpHidden]>,
     Group<grp_symtab>;
 def non_global_symbols_no_strip_list : Separate<["-"], "non_global_symbols_no_strip_list">,
     MetaVarName<"<path>">,
     HelpText<"Specify in <path> the non-global symbols that should remain in the output symbol table">,
-    Flags<[HelpHidden]>,
     Group<grp_symtab>;
 def oso_prefix : Separate<["-"], "oso_prefix">,
     MetaVarName<"<path>">,
     HelpText<"Remove the prefix <path> from OSO symbols in the debug map">,
     Group<grp_symtab>;
 def add_ast_path : Separate<["-"], "add_ast_path">,
     MetaVarName<"<path>">,
     HelpText<"AST paths will be emitted as STABS">,

lld/MachO/SyntheticSections.cpp

 // we cannot encode just the differences between symbols; we have to emit the
 // complete bind information for each symbol.
 uint32_t LazyBindingSection::encode(const Symbol &sym) {
   uint32_t opstreamOffset = contents.size();
   OutputSegment *dataSeg = in.lazyPointers->parent;
   os << static_cast<uint8_t>(BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB |
                              dataSeg->index);
   uint64_t offset =
       in.lazyPointers->addr - dataSeg->addr + sym.stubsIndex * target->wordSize;
   encodeULEB128(offset, os);
   encodeDylibOrdinal(ordinalForSymbol(sym), os);
 
   uint8_t flags = BIND_OPCODE_SET_SYMBOL_TRAILING_FLAGS_IMM;
   if (sym.isWeakRef())
     flags |= BIND_SYMBOL_FLAGS_WEAK_IMPORT;
 
   os << flags << sym.getName() << '\0'

 }
 
 void SymtabSection::finalizeContents() {
   auto addSymbol = [&](std::vector<SymtabEntry> &symbols, Symbol *sym) {
     uint32_t strx = stringTableSection.addString(sym->getName());
     symbols.push_back({sym, strx});
   };
 
+  std::function<void(Symbol *)> localSymbolsHandler;
+  switch (config->localSymbolsPresence) {
+  case SymtabPresence::All:
+    localSymbolsHandler = [&](Symbol *sym) { addSymbol(localSymbols, sym); };
+    break;
+  case SymtabPresence::None:
+    localSymbolsHandler = [&](Symbol *) { /* Do nothing*/ };
+    break;
+  case SymtabPresence::SelectivelyIncluded:
+    localSymbolsHandler = [&](Symbol *sym) {
+      if (config->localSymbolPatterns.match(sym->getName()))
+        addSymbol(localSymbols, sym);
+    };
+    break;
+  case SymtabPresence::SelectivelyExcluded:
+    localSymbolsHandler = [&](Symbol *sym) {
+      if (!config->localSymbolPatterns.match(sym->getName()))
+        addSymbol(localSymbols, sym);
+    };
+    break;
+  }
+
   // Local symbols aren't in the SymbolTable, so we walk the list of object
   // files to gather them.
+  // But if `-x` is set, then we don't need to.
+  if (config->localSymbolsPresence != SymtabPresence::None) {
     for (const InputFile *file : inputFiles) {
       if (auto *objFile = dyn_cast<ObjFile>(file)) {
         for (Symbol *sym : objFile->symbols) {
           if (auto *defined = dyn_cast_or_null<Defined>(sym)) {
             if (defined->isExternal() || !defined->isLive() ||
                 !defined->includeInSymtab)
               continue;
-          addSymbol(localSymbols, sym);
+            localSymbolsHandler(sym);
+          }
         }
       }
     }
   }
 
   // __dyld_private is a local symbol too. It's linker-created and doesn't
   // exist in any object file.
   if (Defined *dyldPrivate = in.stubHelper->dyldPrivate)
-    addSymbol(localSymbols, dyldPrivate);
+    localSymbolsHandler(dyldPrivate);
 
   for (Symbol *sym : symtab->getSymbols()) {
     if (!sym->isLive())
       continue;
     if (auto *defined = dyn_cast<Defined>(sym)) {
       if (!defined->includeInSymtab)
         continue;
       assert(defined->isExternal());
       if (defined->privateExtern)
-        addSymbol(localSymbols, defined);
+        localSymbolsHandler(defined);
       else
         addSymbol(externalSymbols, defined);
     } else if (auto *dysym = dyn_cast<DylibSymbol>(sym)) {
       if (dysym->isReferenced())
         addSymbol(undefinedSymbols, sym);
     }
   }
 

lld/test/MachO/local-symbol-output.s

+# REQUIRES: x86
+
+# RUN: rm -rf %t; split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %t/main.s -o %t/main.o
+
+## Check that -non_global_symbols_no_strip_list and -non_global_symbols_strip_list
+## can't be used at the same time.
+# RUN: not %lld %t/main.o -o /dev/null \
+# RUN:       -non_global_symbols_no_strip_list %t/foo.txt \
+# RUN:       -non_global_symbols_strip_list %t/foo.txt 2>&1 | \
+# RUN:     FileCheck --check-prefix=CONFLICT %s
+
+# CONFLICT: error: cannot use both -non_global_symbols_no_strip_list and -non_global_symbols_strip_list
+
+## Check that -x causes none of the local symbols to be emitted.
+# RUN: %lld %t/main.o -x -o %t/no_local.out
+# RUN: llvm-nm %t/no_local.out | FileCheck --check-prefix NO_LOCAL %s
+
+# NO_LOCAL-NOT: t _foo
+# NO_LOCAL-NOT: t _bar
+# NO_LOCAL-NOT: t _baz
+# NO_LOCAL: T _main
+
+## Check that when using -x with -non_global_symbols_no_strip_list, whichever appears
+## last in the command line arg list will take precedence.
+# RUN: %lld %t/main.o -x -non_global_symbols_no_strip_list %t/foo.txt -o %t/x_then_no_strip.out
+# RUN: llvm-nm %t/x_then_no_strip.out | FileCheck --check-prefix X-NO-STRIP %s
+
+# RUN: %lld %t/main.o -non_global_symbols_no_strip_list %t/foo.txt -x -o %t/no_strip_then_x.out
+# RUN: llvm-nm %t/no_strip_then_x.out | FileCheck --check-prefix NO_LOCAL %s
+
+# X-NO-STRIP-NOT: t _bar
+# X-NO-STRIP-DAG: t _foo
+# X-NO-STRIP-DAG: T _main
+
+## Check that -non_global_symbols_no_strip_list can be specified more than once
+## (The final no-strip list is the union of all these)
+# RUN: %lld %t/main.o -o %t/no_strip_multi.out \
+# RUN:    -non_global_symbols_no_strip_list %t/foo.txt \
+# RUN:    -non_global_symbols_no_strip_list %t/bar.txt
+# RUN: llvm-nm %t/no_strip_multi.out | FileCheck --check-prefix NO-STRIP-MULTI %s
+
+# NO-STRIP-MULTI-NOT: t _baz
+# NO-STRIP-MULTI-DAG: t _foo
+# NO-STRIP-MULTI-DAG: t _bar
+# NO-STRIP-MULTI-DAG: T _main
+
+## Check that when using -x with -non_global_symbols_strip_list, whichever appears
+## last in the command line arg list will take precedence.
+# RUN: %lld %t/main.o -x -non_global_symbols_strip_list %t/foo.txt -o %t/x_then_strip.out
+# RUN: llvm-nm %t/x_then_strip.out | FileCheck --check-prefix X-STRIP %s
+
+# RUN: %lld %t/main.o -non_global_symbols_strip_list %t/foo.txt -x -o %t/strip_then_x.out
+# RUN: llvm-nm %t/no_strip_then_x.out | FileCheck --check-prefix NO_LOCAL %s
+
+# X-STRIP-NOT: t _foo
+# X-STRIP-DAG: t _bar
+# X-STRIP-DAG: t _baz
+# X-STRIP-DAG: T _main
+
+## Check that -non_global_symbols_strip_list can be specified more than once
+## (The final strip list is the union of all these)
+# RUN: %lld %t/main.o -o %t/strip_multi.out \
+# RUN:    -non_global_symbols_strip_list %t/foo.txt \
+# RUN:    -non_global_symbols_strip_list %t/bar.txt
+# RUN: llvm-nm %t/strip_multi.out | FileCheck --check-prefix STRIP-MULTI %s
+
+# STRIP-MULTI-NOT: t _foo
+# STRIP-MULTI-NOT: t _bar
+# STRIP-MULTI-DAG: t _baz
+# STRIP-MULTI-DAG: T _main
+
+## Test interactions with exported_symbol.
+# RUN: %lld %t/main.o -o %t/strip_all_export_one.out \
+# RUN:    -x -exported_symbol _foo \
+# RUN:    -undefined dynamic_lookup
+# RUN: llvm-nm %t/strip_all_export_one.out | FileCheck --check-prefix STRIP-EXP %s
+
+# STRIP-EXP: U _foo
+# STRIP-EXP-EMPTY:
+
+## Test interactions of -x and -non_global_symbols_strip_list with unexported_symbol.
+# RUN: %lld %t/main.o -o %t/strip_x_unexport_one.out \
+# RUN:    -x -unexported_symbol _globby \
+# RUN:    -undefined dynamic_lookup
+
+# RUN: %lld %t/main.o -o %t/strip_all_unexport_one.out \
+# RUN:    -non_global_symbols_strip_list %t/globby.txt \
+# RUN:    -non_global_symbols_strip_list %t/foo.txt \
+# RUN:    -non_global_symbols_strip_list %t/bar.txt \
+# RUN:    -unexported_symbol _globby \
+# RUN:    -undefined dynamic_lookup
+
+# RUN: llvm-nm %t/strip_x_unexport_one.out | FileCheck --check-prefix STRIP-UNEXP %s
+# RUN: llvm-nm %t/strip_all_unexport_one.out | FileCheck --check-prefix STRIP-UNEXP %s
+
+## -unexported_symbol made _globby a local, therefore it should be stripped by -x too
+# STRIP-UNEXP: T __mh_execute_header
+# STRIP-UNEXP-DAG: T _main
+# STRIP-UNEXP-EMPTY:
+
+## Test interactions of -non_global_symbols_strip_list and unexported_symbol.
+# RUN: %lld %t/main.o -undefined dynamic_lookup -o %t/no_strip_unexport.out \
+# RUN:    -non_global_symbols_no_strip_list %t/globby.txt \
+# RUN:    -unexported_symbol _globby
+
+# RUN: llvm-nm %t/no_strip_unexport.out | FileCheck --check-prefix NOSTRIP-UNEXP %s
+
+# NOSTRIP-UNEXP: T __mh_execute_header
+# NOSTRIP-UNEXP-DAG: T _main
+# NOSTRIP-UNEXP-DAG: t _globby
+# NOSTRIP-UNEXP-EMPTY:
+
+#--- foo.txt
+_foo
+
+#--- bar.txt
+_bar
+
+#--- globby.txt
+_globby
+
+#--- main.s
+.globl _main
+.globl _globby
+
+_foo:
+  ret
+
+_bar:
+  ret
+
+_baz:
+  ret
+
+_main:
+  callq _foo
+  ret
+
+ _globby:
+  ret
 No newline at end of file