[lld-macho][nfc] Use includeInSymtab for all symtab-skipping logic

[int3]

Summary

D123302: [lld/mac] Add some comments and asserts got me looking deeper at includeInSymtab. I thought it was a
little odd that there were excluded (live) symbols for which
includeInSymtab was false; we shouldn't have so many different ways to
exclude a symbol. As such, this diff makes the L-prefixed-symbol
exclusion code use includeInSymtab too. (Note that as part of our
support for __eh_frame, we will also be excluding all __eh_frame
symbols from the symtab in a future diff.)

Another thing I noticed is that the emitStabs code never has to deal
with excluded symbols because SymtabSection::finalize() already
filters them out. As such, I've updated the comments and asserts from
D123302: [lld/mac] Add some comments and asserts to reflect this.

[thakis]

Nice!

lld/MachO/ConcatOutputSection.cpp

333

For the lines that are just reformatted without changes, maybe commit those in a separate commit.

lld/MachO/SymbolTable.cpp

233

Doesn't this still make sense to explain the /*file=*/nullptr (instead of the isec->getFile() you'd expect -- or at least I expected that.)

239

I think this one is just reformatted without any other changes too (ie maybe put in separate commit)

[int3]

lld/MachO/ConcatOutputSection.cpp

333

oh yeah I had initially added the extra param to addDefined as well but removed it after, hence the formatting changes. will revert those

lld/MachO/SymbolTable.cpp

233

sure, I can leave it in. IMO it's conceivable that synthetic symbols could have an associated InputSection, but it's true that we don't create any of those at the moment

[int3]

address comments

lld/MachO/ConcatOutputSection.cpp

for (Reloc &r : reverse(relocs)) {
       thunkInfo.isec->live = true;
 
       StringRef thunkName = saver().save(funcSym->getName() + ".thunk." +
                                          std::to_string(thunkInfo.sequence++));
       if (!isa<Defined>(funcSym) || cast<Defined>(funcSym)->isExternal()) {
         r.referent = thunkInfo.sym = symtab->addDefined(
             thunkName, /*file=*/nullptr, thunkInfo.isec, /*value=*/0,
             thunkSize, /*isWeakDef=*/false, /*isPrivateExtern=*/true,
             /*isThumb=*/false, /*isReferencedDynamically=*/false,
             /*noDeadStrip=*/false, /*isWeakDefCanBeHidden=*/false);
       } else {
         r.referent = thunkInfo.sym = make<Defined>(
-            thunkName, /*file=*/nullptr, thunkInfo.isec, /*value=*/0,
-            thunkSize, /*isWeakDef=*/false, /*isExternal=*/false,
-            /*isPrivateExtern=*/true, /*isThumb=*/false,
+            thunkName, /*file=*/nullptr, thunkInfo.isec, /*value=*/0, thunkSize,
+            /*isWeakDef=*/false, /*isExternal=*/false, /*isPrivateExtern=*/true,
+            /*includeInSymtab=*/true, /*isThumb=*/false,
             /*isReferencedDynamically=*/false, /*noDeadStrip=*/false,
             /*isWeakDefCanBeHidden=*/false);
       }
       thunkInfo.sym->used = true;
       target->populateThunk(thunkInfo.isec, funcSym);
       finalizeOne(thunkInfo.isec);
       fprintf(stderr, "%" PRIx64 "\n", thunkInfo.isec->outSecOff);
       thunks.push_back(thunkInfo.isec);

lld/MachO/Driver.cpp

for (Symbol *sym : symtab->getSymbols()) {
     auto *isec = make<ConcatInputSection>(*section, data, common->align);
     if (!osec)
       osec = ConcatOutputSection::getOrCreateForInput(isec);
     isec->parent = osec;
     inputSections.push_back(isec);
 
     // FIXME: CommonSymbol should store isReferencedDynamically, noDeadStrip
     // and pass them on here.
-    replaceSymbol<Defined>(sym, sym->getName(), common->getFile(), isec,
-                           /*value=*/0,
-                           /*size=*/0,
-                           /*isWeakDef=*/false,
-                           /*isExternal=*/true, common->privateExtern,
-                           /*isThumb=*/false,
-                           /*isReferencedDynamically=*/false,
-                           /*noDeadStrip=*/false);
+    replaceSymbol<Defined>(
+        sym, sym->getName(), common->getFile(), isec, /*value=*/0, /*size=*/0,
+        /*isWeakDef=*/false, /*isExternal=*/true, common->privateExtern,
+        /*includeInSymtab=*/true, /*isThumb=*/false,
+        /*isReferencedDynamically=*/false, /*noDeadStrip=*/false);
   }
 }
 
 static void initializeSectionRenameMap() {
   if (config->dataConst) {
     SmallVector<StringRef> v{section_names::got,
                              section_names::authGot,
                              section_names::authPtr,

lld/MachO/InputFiles.cpp

if (sym.n_type & N_EXT) {
     return symtab->addDefined(
         name, isec->getFile(), isec, value, size, sym.n_desc & N_WEAK_DEF,
         isPrivateExtern, sym.n_desc & N_ARM_THUMB_DEF,
         sym.n_desc & REFERENCED_DYNAMICALLY, sym.n_desc & N_NO_DEAD_STRIP,
         isWeakDefCanBeHidden);
   }
   assert(!isWeakDefCanBeHidden &&
          "weak_def_can_be_hidden on already-hidden symbol?");
+  bool includeInSymtab = !name.startswith("l") && !name.startswith("L");
   return make<Defined>(
       name, isec->getFile(), isec, value, size, sym.n_desc & N_WEAK_DEF,
-      /*isExternal=*/false, /*isPrivateExtern=*/false,
+      /*isExternal=*/false, /*isPrivateExtern=*/false, includeInSymtab,
       sym.n_desc & N_ARM_THUMB_DEF, sym.n_desc & REFERENCED_DYNAMICALLY,
       sym.n_desc & N_NO_DEAD_STRIP);
 }
 
 // Absolute symbols are defined symbols that do not have an associated
 // InputSection. They cannot be weak.
 template <class NList>
 static macho::Symbol *createAbsolute(const NList &sym, InputFile *file,
                                      StringRef name) {
   if (sym.n_type & N_EXT) {
     return symtab->addDefined(
         name, file, nullptr, sym.n_value, /*size=*/0,
         /*isWeakDef=*/false, sym.n_type & N_PEXT, sym.n_desc & N_ARM_THUMB_DEF,
         /*isReferencedDynamically=*/false, sym.n_desc & N_NO_DEAD_STRIP,
         /*isWeakDefCanBeHidden=*/false);
   }
   return make<Defined>(name, file, nullptr, sym.n_value, /*size=*/0,
                        /*isWeakDef=*/false,
                        /*isExternal=*/false, /*isPrivateExtern=*/false,
-                       sym.n_desc & N_ARM_THUMB_DEF,
+                       /*includeInSymtab=*/true, sym.n_desc & N_ARM_THUMB_DEF,
                        /*isReferencedDynamically=*/false,
                        sym.n_desc & N_NO_DEAD_STRIP);
 }
 
 template <class NList>
 macho::Symbol *ObjFile::parseNonSectionSymbol(const NList &sym,
                                               StringRef name) {
   uint8_t type = sym.n_type & N_TYPE;

lld/MachO/SymbolTable.cpp

Defined *SymbolTable::addDefined(StringRef name, InputFile *file,
 
   // With -flat_namespace, all extern symbols in dylibs are interposable.
   // FIXME: Add support for `-interposable` (PR53680).
   bool interposable = config->namespaceKind == NamespaceKind::flat &&
                       config->outputType != MachO::MH_EXECUTE &&
                       !isPrivateExtern;
   Defined *defined = replaceSymbol<Defined>(
       s, name, file, isec, value, size, isWeakDef, /*isExternal=*/true,
-      isPrivateExtern, isThumb, isReferencedDynamically, noDeadStrip,
-      overridesWeakDef, isWeakDefCanBeHidden, interposable);
+      isPrivateExtern, /*includeInSymtab=*/true, isThumb,
+      isReferencedDynamically, noDeadStrip, overridesWeakDef,
+      isWeakDefCanBeHidden, interposable);
   return defined;
 }
 
 Symbol *SymbolTable::addUndefined(StringRef name, InputFile *file,
                                   bool isWeakRef) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(name, file);
Symbol *SymbolTable::addLazyObject(StringRef name, InputFile &file) {
   }
   return s;
 }
 
 Defined *SymbolTable::addSynthetic(StringRef name, InputSection *isec,
                                    uint64_t value, bool isPrivateExtern,
                                    bool includeInSymtab,
                                    bool referencedDynamically) {
   assert(!isec || !isec->getFile()); // See makeSyntheticInputSection().
   Defined *s =
       addDefined(name, /*file=*/nullptr, isec, value, /*size=*/0,
-                 /*isWeakDef=*/false, isPrivateExtern,
-                 /*isThumb=*/false, referencedDynamically,
-                 /*noDeadStrip=*/false, /*isWeakDefCanBeHidden=*/false);
+                 /*isWeakDef=*/false, isPrivateExtern, /*isThumb=*/false,
+                 referencedDynamically, /*noDeadStrip=*/false,
+                 /*isWeakDefCanBeHidden=*/false);
   s->includeInSymtab = includeInSymtab;
   return s;
 }
 
 enum class Boundary {
   Start,
   End,
 };

lld/MachO/Symbols.h

public:
   // True if this symbol is used from a live section.
   bool used : 1;
 };
 
 class Defined : public Symbol {
 public:
   Defined(StringRefZ name, InputFile *file, InputSection *isec, uint64_t value,
           uint64_t size, bool isWeakDef, bool isExternal, bool isPrivateExtern,
-          bool isThumb, bool isReferencedDynamically, bool noDeadStrip,
-          bool canOverrideWeakDef = false, bool isWeakDefCanBeHidden = false,
-          bool interposable = false);
+          bool includeInSymtab, bool isThumb, bool isReferencedDynamically,
+          bool noDeadStrip, bool canOverrideWeakDef = false,
+          bool isWeakDefCanBeHidden = false, bool interposable = false);
 
   bool isWeakDef() const override { return weakDef; }
   bool isExternalWeakDef() const {
     return isWeakDef() && isExternal() && !privateExtern;
   }
   bool isTlv() const override;
 
   bool isExternal() const { return external; }

lld/MachO/Symbols.cpp

 }
 
 uint64_t Symbol::getStubVA() const { return in.stubs->getVA(stubsIndex); }
 uint64_t Symbol::getGotVA() const { return in.got->getVA(gotIndex); }
 uint64_t Symbol::getTlvVA() const { return in.tlvPointers->getVA(gotIndex); }
 
 Defined::Defined(StringRefZ name, InputFile *file, InputSection *isec,
                  uint64_t value, uint64_t size, bool isWeakDef, bool isExternal,
-                 bool isPrivateExtern, bool isThumb,
+                 bool isPrivateExtern, bool includeInSymtab, bool isThumb,
                  bool isReferencedDynamically, bool noDeadStrip,
                  bool canOverrideWeakDef, bool isWeakDefCanBeHidden,
                  bool interposable)
     : Symbol(DefinedKind, name, file), overridesWeakDef(canOverrideWeakDef),
-      privateExtern(isPrivateExtern), includeInSymtab(true),
+      privateExtern(isPrivateExtern), includeInSymtab(includeInSymtab),
       wasIdenticalCodeFolded(false), thumb(isThumb),
       referencedDynamically(isReferencedDynamically), noDeadStrip(noDeadStrip),
       interposable(interposable), weakDefCanBeHidden(isWeakDefCanBeHidden),
       weakDef(isWeakDef), external(isExternal), isec(isec), value(value),
       size(size) {
   if (isec) {
     isec->symbols.push_back(this);
     // Maintain sorted order.

lld/MachO/SyntheticSections.cpp

in.imageLoaderCache->parent =
       ConcatOutputSection::getOrCreateForInput(in.imageLoaderCache);
   inputSections.push_back(in.imageLoaderCache);
   // Since this isn't in the symbol table or in any input file, the noDeadStrip
   // argument doesn't matter.
   dyldPrivate =
       make<Defined>("__dyld_private", nullptr, in.imageLoaderCache, 0, 0,
                     /*isWeakDef=*/false,
                     /*isExternal=*/false, /*isPrivateExtern=*/false,
+                    /*includeInSymtab=*/true,
                     /*isThumb=*/false, /*isReferencedDynamically=*/false,
                     /*noDeadStrip=*/false);
   dyldPrivate->used = true;
 }
 
 LazyPointerSection::LazyPointerSection()
     : SyntheticSection(segment_names::data, section_names::lazySymbolPtr) {
   align = target->wordSize;
void SymtabSection::emitStabs() {
 
   std::vector<Defined *> symbolsNeedingStabs;
   for (const SymtabEntry &entry :
        concat<SymtabEntry>(localSymbols, externalSymbols)) {
     Symbol *sym = entry.sym;
     assert(sym->isLive() &&
            "dead symbols should not be in localSymbols, externalSymbols");
     if (auto *defined = dyn_cast<Defined>(sym)) {
+      // Excluded symbols should have been filtered out in finalizeContents().
+      assert(defined->includeInSymtab);
+
       if (defined->isAbsolute())
         continue;
 
       // Constant-folded symbols go in the executable's symbol table, but don't
       // get a stabs entry.
       if (defined->wasIdenticalCodeFolded)
         continue;
 
       InputSection *isec = defined->isec;
       ObjFile *file = dyn_cast_or_null<ObjFile>(isec->getFile());
       if (!file || !file->compileUnit)
         continue;
 
-      // All symbols that set includeInSymtab to false are synthetic symbols.
-      // Those have `file` set to nullptr and were already skipped due to that.
-      assert(defined->includeInSymtab);
-
       symbolsNeedingStabs.push_back(defined);
     }
   }
 
   llvm::stable_sort(symbolsNeedingStabs, [&](Defined *a, Defined *b) {
     return a->isec->getFile()->id < b->isec->getFile()->id;
   });
 
void SymtabSection::finalizeContents() {
   };
 
   // Local symbols aren't in the SymbolTable, so we walk the list of object
   // files to gather them.
   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()) {
-            StringRef name = defined->getName();
-            if (!name.startswith("l") && !name.startswith("L"))
-              addSymbol(localSymbols, sym);
-          }
-        }
-      }
-    }
-  }
+          if (defined->isExternal() || !defined->isLive() ||
+              !defined->includeInSymtab)
+            continue;
 
   // __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);
 
   for (Symbol *sym : symtab->getSymbols()) {
     if (!sym->isLive())

lld/MachO/UnwindInfoSection.cpp

if (auto *referentIsec = r.referent.dyn_cast<InputSection *>()) {
       // symbols for them in the GOT.
       Symbol *&s = personalityTable[{referentIsec, r.addend}];
       if (s == nullptr) {
         // This runs after dead stripping, so the noDeadStrip argument does not
         // matter.
         s = make<Defined>("<internal>", /*file=*/nullptr, referentIsec,
                           r.addend, /*size=*/0, /*isWeakDef=*/false,
                           /*isExternal=*/false, /*isPrivateExtern=*/false,
+                          /*includeInSymtab=*/true,
                           /*isThumb=*/false, /*isReferencedDynamically=*/false,
                           /*noDeadStrip=*/false);
         in.got->addEntry(s);
       }
       r.referent = s;
       r.addend = 0;
     }
   }