[lld-macho] Handle error cases properly for -exported_symbol(s_list)

[gkm]

Summary

This fixes defects in D98223 [lld-macho] implement options -(un)exported_symbol(s_list):

• disallow export of hidden symbols
• verify that whitelisted literal names are defined in the symbol table
• reflect export-status overrides in nlist attribute of N_EXT or N_PEXT

Thanks to @thakis for raising these issues

[gkm]

• add missing test for erroneous export of private symbol

[thakis]

Thanks!

lld/MachO/SyntheticSections.cpp

588

Should we error in this acccessor style function? Won't we get two errors if we call this for relocs and for symbol table?

nit: If you end strings after \n, clang-format formats them nicer: replace "\n>>> defined in" with "\n" ">>> defined in"

I'd find the diff easier to read if it just extracted the existing conditional out of the loop for now. Maybe this diag could move to a follow-on patch?

612

Could we do this like -u handling in by adding Undefineds in the driver code instead of having a special case here? Then we wouldn't have to make the SymbolTable interface more complicated for this feature.

lld/test/MachO/export-options.s

54

https://en.wikipedia.org/wiki/Blacklist_(computing)#Controversy_over_use_of_the_term

Probably good to just use the actual flag names here (exported symbol list, unexported symbol list)

79

Should this have a -NOT for _hide_globl?

[gkm]

lld/MachO/SyntheticSections.cpp

588

• You are correct that an accessor should not error. In my limited testing, I don't see the message twice because the first error precludes writing the output where the second error might happen.

• With the proposed change, clang-format turns two lines onto four, which isn't so nice after all:

error("cannot export hidden symbol " + symbolName +
      "\n"
      ">>> defined in " +
      toString(defined->getFile()));

612

I moved the check for undefined exported symbols the the same place where we check undefined -u options. It didn't make sense to lump exported symbols in with explicit undefined symbols because they have different error messages.

Open question: Should literals on the exported symbols list also be added to the symbol table as undefined symbols in order to pull definitions from archive inputs? It is unclear from my initial reading of ld64 sources. I'll look closer tomorrow and/or experiment.

lld/test/MachO/export-options.s

79

llvm-objdump --export-trie only prints the exported symbols, so the -NOT is necessary to catch an unexpected line for _hide_globl as TRIE-NOT. By contrast, llvm-nm -m prints a line for every symbol to report its visibility, and we already have a pattern asserting as expected non-external (was a private external).

[gkm]

• revise according to review feedback

[int3]

lld/MachO/Driver.cpp

1040

unnecessary braces

lld/MachO/SymbolTable.cpp

19–22

this seems small enough to move into the header

lld/MachO/SyntheticSections.cpp

585–588

I'm wondering if this will be a performance bottleneck. If a build has mostly global symbols in the input but uses -exported_symbols to filter out most of them, then it would be better to set the value of privateExtern at parse time instead of calling exportedSymbols.match() more than once.

My measurements show that symbol ordering (which again looks up every symbol in a hashmap) is the biggest bottleneck when linking chromium_framework, so it wouldn't surprise me if this is worth optimizing.

We can punt on that for now though, but it would be good to leave a TODO

612

Please file a task re the open question if you're not getting to it soon :) It's a good case to consider, let's not forget about it

lld/test/MachO/export-options.s

30

IMO there's no point in matching this if we're not also matching the filename (I'm fine with just removing this line)

lld/MachO/Config.h

enum class UndefinedSymbolTreatment {
   unknown,
   error,
   warning,
   suppress,
   dynamic_lookup,
 };
 
 class SymbolPatterns {
+public:
   // GlobPattern can also match literals,
   // but we prefer the O(1) lookup of DenseSet.
   llvm::DenseSet<llvm::CachedHashStringRef> literals;
   std::vector<llvm::GlobPattern> globs;
 
-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;
 };
 

lld/MachO/Driver.cpp

if (config->timeTraceEnabled)
     // via -u flag twice.
     for (const Symbol *undefined : config->explicitUndefineds) {
       if (isa<Undefined>(undefined)) {
         error("undefined symbol: " + toString(*undefined) +
               "\n>>> referenced by flag -u " + toString(*undefined));
         return false;
       }
     }
+    // Literal exported-symbol names must be defined, but glob
+    // patterns need not match.
+    for (const CachedHashStringRef &cachedName :
+         config->exportedSymbols.literals) {
+      if (const Symbol *sym = symtab->find(cachedName))
+        if (isa<Defined>(sym))
+          continue;
+      error("undefined symbol " + cachedName.val() +
+            "\n>>> referenced from option -exported_symbo(s_list)");
+    }
 
     createSyntheticSections();
 
     // The Itanium C++ ABI requires dylibs to pass a pointer to __cxa_atexit
     // which does e.g. cleanup of static global variables. The ABI document says
     // that the pointer can point to any address in one of the dylib's segments,
     // but in practice ld64 seems to set it to point to the header, so that's
     // what's implemented here.

lld/MachO/SymbolTable.h

public:
 
   Symbol *addLazy(StringRef name, ArchiveFile *file,
                   const llvm::object::Archive::Symbol &sym);
 
   Defined *addSynthetic(StringRef name, InputSection *, uint32_t value,
                         bool isPrivateExtern, bool isLinkerInternal);
 
   ArrayRef<Symbol *> getSymbols() const { return symVector; }
-  Symbol *find(StringRef name);
+  Symbol *find(llvm::CachedHashStringRef name);
+  Symbol *find(StringRef name) { return find(llvm::CachedHashStringRef(name)); }
 
 private:
   std::pair<Symbol *, bool> insert(StringRef name);
   llvm::DenseMap<llvm::CachedHashStringRef, int> symMap;
   std::vector<Symbol *> symVector;
 };
 
 void treatUndefinedSymbol(const Undefined &);
 
 extern SymbolTable *symtab;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/SymbolTable.cpp

 #include "InputFiles.h"
 #include "Symbols.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 
 using namespace llvm;
 using namespace lld;
 using namespace lld::macho;
 
-Symbol *SymbolTable::find(StringRef name) {
-  auto it = symMap.find(CachedHashStringRef(name));
+Symbol *SymbolTable::find(CachedHashStringRef cachedName) {
+  auto it = symMap.find(cachedName);
   if (it == symMap.end())
     return nullptr;
   return symVector[it->second];
 }
 
 std::pair<Symbol *, bool> SymbolTable::insert(StringRef name) {
   auto p = symMap.insert({CachedHashStringRef(name), (int)symVector.size()});
 
   // Name already present in the symbol table.

lld/MachO/SyntheticSections.cpp

if (defined->isExternalWeakDef()) {
       }
     }
   }
 }
 
 ExportSection::ExportSection()
     : LinkEditSection(segment_names::linkEdit, section_names::export_) {}
 
+static void validateExportSymbol(const Defined *defined) {
+  StringRef symbolName = defined->getName();
+  if (defined->privateExtern && config->exportedSymbols.match(symbolName))
+    error("cannot export hidden symbol " + symbolName + "\n>>> defined in " +
+          toString(defined->getFile()));
+}
+
+static bool shouldExportSymbol(const Defined *defined) {
+  if (defined->privateExtern)
+    return false;
+  // TODO: Is this a performance bottleneck? If a build has mostly
+  // global symbols in the input but uses -exported_symbols to filter
+  // out most of them, then it would be better to set the value of
+  // privateExtern at parse time instead of calling
+  // exportedSymbols.match() more than once.
+  //
+  // Measurements show that symbol ordering (which again looks up
+  // every symbol in a hashmap) is the biggest bottleneck when linking
+  // chromium_framework, so this will likely be worth optimizing.
+  return config->exportedSymbols.empty()
+             ? !config->unexportedSymbols.match(defined->getName())
+             : config->exportedSymbols.match(defined->getName());
+}
+
 void ExportSection::finalizeContents() {
   trieBuilder.setImageBase(in.header->addr);
   for (const Symbol *sym : symtab->getSymbols()) {
     if (const auto *defined = dyn_cast<Defined>(sym)) {
-      if (config->exportedSymbols.empty()) {
-        if (defined->privateExtern ||
-            config->unexportedSymbols.match(defined->getName()))
-          continue;
-      } else {
-        if (!config->exportedSymbols.match(defined->getName()))
-          continue;
-      }
+      validateExportSymbol(defined);
+      if (!shouldExportSymbol(defined))
       trieBuilder.addSymbol(*defined);
       hasWeakSymbol = hasWeakSymbol || sym->isWeakDef();
     }
   }
   size = trieBuilder.build();
 }
 
 void ExportSection::writeTo(uint8_t *buf) const { trieBuilder.writeTo(buf); }
 
 FunctionStartsSection::FunctionStartsSection()
     : LinkEditSection(segment_names::linkEdit, section_names::functionStarts) {}
 
void SymtabSection::writeTo(uint8_t *buf) const {
   }
 
   for (const SymtabEntry &entry : concat<const SymtabEntry>(
            localSymbols, externalSymbols, undefinedSymbols)) {
     nList->n_strx = entry.strx;
     // TODO populate n_desc with more flags
     if (auto *defined = dyn_cast<Defined>(entry.sym)) {
       uint8_t scope = 0;
-      if (defined->privateExtern) {
+      if (!shouldExportSymbol(defined)) {
         // Private external -- dylib scoped symbol.
         // Promote to non-external at link time.
         assert(defined->isExternal() && "invalid input file");
         scope = N_PEXT;
       } else if (defined->isExternal()) {
         // Normal global symbol.
         scope = N_EXT;
       } else {

lld/test/MachO/export-options.s

 # REQUIRES: x86
 
 # RUN: split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %t/default.s -o %t/default.o
 
 ## Check that mixing exported and unexported symbol options yields an error
 # RUN: not %lld -dylib %t/default.o -o /dev/null \
 # RUN:         -exported_symbol a -unexported_symbol b 2>&1 | \
 # RUN:     FileCheck --check-prefix=CONFLICT %s
 
 # CONFLICT: error: cannot use both -exported_symbol* and -unexported_symbol* options
 # CONFLICT-NEXT: >>> ignoring unexports
 
+## Check that exported literal symbol name is present in symbol table
+# RUN: not %lld -dylib %t/default.o -o /dev/null \
+# RUN:         -exported_symbol absent_literal \
+# RUN:         -exported_symbol absent_gl?b 2>&1 | \
+# RUN:     FileCheck --check-prefix=UNDEF %s
+
+# UNDEF: error: undefined symbol absent_literal
+# UNDEF-NEXT: >>> referenced from option -exported_symbo(s_list)
+# UNDEF-NOT: error: {{.*}} absent_gl{{.}}b
+
+## Check that exported symbol is global
+# RUN: not %lld -dylib %t/default.o -o /dev/null \
+# RUN:         -exported_symbol _private 2>&1 | \
+# RUN:     FileCheck --check-prefix=PRIVATE %s
+
+# PRIVATE: error: cannot export hidden symbol _private
+
 #--- default.s
 
 .macro DEFSYM, type, sym
 \type \sym
 \sym:
   retq
 .endm
 
 DEFSYM .globl, _keep_globl
 DEFSYM .globl, _hide_globl
-DEFSYM .private_extern, _keep_private
-DEFSYM .private_extern, _show_private
+DEFSYM .private_extern, _private
 
 ## Check that the export trie is unaltered
 # RUN: %lld -dylib %t/default.o -o %t/default
 # RUN: llvm-objdump --macho --exports-trie %t/default | \
 # RUN:     FileCheck --check-prefix=DEFAULT %s
 
 # DEFAULT-LABEL: Exports trie:
 # DEFAULT-DAG:   _hide_globl
 # DEFAULT-DAG:   _keep_globl
-# DEFAULT-NOT:   _hide_private
-# DEFAULT-NOT:   _show_private
+# DEFAULT-NOT:   _private
+
+## Check that the export trie is shaped by an allow list and then
+## by a deny list. Both lists are designed to yield the same result.
 
-## Check that the export trie is properly augmented
-## Check that non-matching literal pattern has no effect
-# RUN: %lld -dylib %t/default.o -o %t/export \
-# RUN:     -exported_symbol _show_private \
-# RUN:     -exported_symbol _extra_cruft -exported_symbol '*xtra_cr?ft'
-# RUN: llvm-objdump --macho --exports-trie %t/export | \
-# RUN:     FileCheck --check-prefix=EXPORTED %s
-
-# EXPORTED-LABEL: Exports trie:
-# EXPORTED-DAG:   _show_private
-# EXPORTED-NOT:   _hide_globl
-# EXPORTED-NOT:   _keep_globl
-# EXPORTED-NOT:   _hide_private
-# EXPORTED-NOT:   {{.*}}xtra_cr{{.}}ft
-
-## Check that the export trie is properly diminished
-## Check that non-matching glob pattern has no effect
-# RUN: %lld -dylib %t/default.o -o %t/unexport \
-# RUN:     -unexported_symbol _hide_global
-# RUN: llvm-objdump --macho --exports-trie %t/unexport | \
-# RUN:     FileCheck --check-prefix=UNEXPORTED %s
-
-# UNEXPORTED-LABEL: Exports trie:
-# UNEXPORTED-DAG:   _keep_globl
-# UNEXPORTED-NOT:   _hide_globl
-# UNEXPORTED-NOT:   _show_private
-# UNEXPORTED-NOT:   _hide_private
+## Check the allow list
+# RUN: %lld -dylib %t/default.o -o %t/allowed \
+# RUN:     -exported_symbol _keep_globl
+# RUN: llvm-objdump --macho --exports-trie %t/allowed | \
+# RUN:     FileCheck --check-prefix=TRIE %s
+# RUN: llvm-nm -m %t/allowed | \
+# RUN:     FileCheck --check-prefix=NM %s
+
+## Check the deny list
+# RUN: %lld -dylib %t/default.o -o %t/denied \
+# RUN:     -unexported_symbol _hide_globl
+# RUN: llvm-objdump --macho --exports-trie %t/denied | \
+# RUN:     FileCheck --check-prefix=TRIE %s
+# RUN: llvm-nm -m %t/denied | \
+# RUN:     FileCheck --check-prefix=NM %s
+
+# TRIE-LABEL: Exports trie:
+# TRIE-DAG:   _keep_globl
+# TRIE-NOT:   _hide_globl
+# TRIE-NOT:   _private
+
+# NM-DAG: external _keep_globl
+# NM-DAG: non-external (was a private external) _hide_globl
+# NM-DAG: non-external (was a private external) _private
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos \
 # RUN:     %t/symdefs.s -o %t/symdefs.o
 
 #--- symdefs.s
 
 .macro DEFSYM, sym
-.private_extern \sym
+.globl \sym
 \sym:
   retq
 .endm
 
 DEFSYM literal_only
 DEFSYM literal_also
 DEFSYM globby_only
 DEFSYM globby_also