MC: Simplify handling of temporary symbols in COFF writer.

[pcc]

Summary

The COFF object writer was previously adding unnecessary symbols to its
temporary data structures and cleaning them up later. This made the code
harder to understand and caused a bug (aliases classed as temporary symbols
would cause an assertion failure). A much simpler way of handling such
symbols is to ask the layout for their section-relative position when needed.

Tested with a bootstrap on Windows and by building Chrome.

[• rafael]

LGTM. Thanks!

lib/MC/WinCOFFObjectWriter.cpp

public:
   COFFSection *Section;
   int Relocations;
 
   const MCSymbol *MC;
 
   COFFSymbol(StringRef name);
   void set_name_offset(uint32_t Offset);
 
-  bool should_keep() const;
-
   int64_t getIndex() const { return Index; }
   void setIndex(int Value) {
     Index = Value;
     if (MC)
       MC->setIndex(static_cast<uint32_t>(Value));
   }
 };
 
public:
 
   void defineSection(MCSectionCOFF const &Sec);
   void DefineSymbol(const MCSymbol &Symbol, MCAssembler &Assembler,
                     const MCAsmLayout &Layout);
 
   void SetSymbolName(COFFSymbol &S);
   void SetSectionName(COFFSection &S);
 
-  bool ExportSymbol(const MCSymbol &Symbol, MCAssembler &Asm);
-
   bool IsPhysicalSection(COFFSection *S);
 
   // Entity writing methods.
 
   void WriteFileHeader(const COFF::header &Header);
   void WriteSymbol(const COFFSymbol &S);
   void WriteAuxiliarySymbols(const COFFSymbol::AuxiliarySymbols &S);
   void writeSectionHeader(const COFF::section &S);

 // In the case that the name does not fit within 8 bytes, the offset
 // into the string table is stored in the last 4 bytes instead, leaving
 // the first 4 bytes as 0.
 void COFFSymbol::set_name_offset(uint32_t Offset) {
   write_uint32_le(Data.Name + 0, 0);
   write_uint32_le(Data.Name + 4, Offset);
 }
 
-/// logic to decide if the symbol should be reported in the symbol table
-bool COFFSymbol::should_keep() const {
-  // no section means its external, keep it
-  if (!Section)
-    return true;
-
-  // if it has relocations pointing at it, keep it
-  if (Relocations > 0) {
-    assert(Section->Number != -1 && "Sections with relocations must be real!");
-    return true;
-  }
-
-  // if this is a safeseh handler, keep it
-  if (MC && (cast<MCSymbolCOFF>(MC)->isSafeSEH()))
-    return true;
-
-  // if the section its in is being droped, drop it
-  if (Section->Number == -1)
-    return false;
-
-  // if it is the section symbol, keep it
-  if (Section->Symbol == this)
-    return true;
-
-  // if its temporary, drop it
-  if (MC && MC->isTemporary())
-    return false;
-
-  // otherwise, keep it
-  return true;
-}
-
 //------------------------------------------------------------------------------
 // Section class implementation
 
 COFFSection::COFFSection(StringRef name)
     : Name(name), MCSection(nullptr), Symbol(nullptr) {
   memset(&Header, 0, sizeof(Header));
 }
 

 }
 
 /// This function takes a symbol data object from the assembler
 /// and creates the associated COFF symbol staging object.
 void WinCOFFObjectWriter::DefineSymbol(const MCSymbol &Symbol,
                                        MCAssembler &Assembler,
                                        const MCAsmLayout &Layout) {
   COFFSymbol *coff_symbol = GetOrCreateCOFFSymbol(&Symbol);
-  SymbolMap[&Symbol] = coff_symbol;
 
   if (cast<MCSymbolCOFF>(Symbol).isWeakExternal()) {
     coff_symbol->Data.StorageClass = COFF::IMAGE_SYM_CLASS_WEAK_EXTERNAL;
 
     if (Symbol.isVariable()) {
       const MCSymbolRefExpr *SymRef =
           dyn_cast<MCSymbolRefExpr>(Symbol.getVariableValue());
 

 
 void WinCOFFObjectWriter::SetSymbolName(COFFSymbol &S) {
   if (S.Name.size() > COFF::NameSize)
     S.set_name_offset(Strings.getOffset(S.Name));
   else
     std::memcpy(S.Data.Name, S.Name.c_str(), S.Name.size());
 }
 
-bool WinCOFFObjectWriter::ExportSymbol(const MCSymbol &Symbol,
-                                       MCAssembler &Asm) {
-  // This doesn't seem to be right. Strings referred to from the .data section
-  // need symbols so they can be linked to code in the .text section right?
-
-  // return Asm.isSymbolLinkerVisible(Symbol);
-
-  // Non-temporary labels should always be visible to the linker.
-  if (!Symbol.isTemporary())
-    return true;
-
-  // Temporary variable symbols are invisible.
-  if (Symbol.isVariable())
-    return false;
-
-  // Absolute temporary labels are never visible.
-  return !Symbol.isAbsolute();
-}
-
 bool WinCOFFObjectWriter::IsPhysicalSection(COFFSection *S) {
   return (S->Header.Characteristics & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA) ==
          0;
 }
 
 //------------------------------------------------------------------------------
 // entity writing methods
 

 void WinCOFFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
                                                    const MCAsmLayout &Layout) {
   // "Define" each section & symbol. This creates section & symbol
   // entries in the staging area.
   for (const auto &Section : Asm)
     defineSection(static_cast<const MCSectionCOFF &>(Section));
 
   for (const MCSymbol &Symbol : Asm.symbols())
-    if (ExportSymbol(Symbol, Asm))
+    if (!Symbol.isTemporary())
       DefineSymbol(Symbol, Asm, Layout);
 }
 
 bool WinCOFFObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
     const MCAssembler &Asm, const MCSymbol &SymA, const MCFragment &FB,
     bool InSet, bool IsPCRel) const {
   // MS LINK expects to be able to replace all references to a function with a
   // thunk to implement their /INCREMENTAL feature.  Make sure we don't optimize
bool WinCOFFObjectWriter::isWeak(const MCSymbol &Sym) const {
   return true;
 }
 
 void WinCOFFObjectWriter::recordRelocation(
     MCAssembler &Asm, const MCAsmLayout &Layout, const MCFragment *Fragment,
     const MCFixup &Fixup, MCValue Target, bool &IsPCRel, uint64_t &FixedValue) {
   assert(Target.getSymA() && "Relocation must reference a symbol!");
 
-  const MCSymbol &Symbol = Target.getSymA()->getSymbol();
-  const MCSymbol &A = Symbol;
+  const MCSymbol &A = Target.getSymA()->getSymbol();
   if (!A.isRegistered()) {
     Asm.getContext().reportError(Fixup.getLoc(),
                                       Twine("symbol '") + A.getName() +
                                           "' can not be undefined");
     return;
   }
   if (A.isTemporary() && A.isUndefined()) {
     Asm.getContext().reportError(Fixup.getLoc(),
                                       Twine("assembler label '") + A.getName() +
                                           "' can not be undefined");
     return;
   }
 
   MCSection *Section = Fragment->getParent();
 
   // Mark this symbol as requiring an entry in the symbol table.
   assert(SectionMap.find(Section) != SectionMap.end() &&
          "Section must already have been defined in executePostLayoutBinding!");
-  assert(SymbolMap.find(&A) != SymbolMap.end() &&
-         "Symbol must already have been defined in executePostLayoutBinding!");
 
   COFFSection *coff_section = SectionMap[Section];
-  COFFSymbol *coff_symbol = SymbolMap[&A];
   const MCSymbolRefExpr *SymB = Target.getSymB();
   bool CrossSection = false;
 
   if (SymB) {
     const MCSymbol *B = &SymB->getSymbol();
     if (!B->getFragment()) {
       Asm.getContext().reportError(
           Fixup.getLoc(),
           Twine("symbol '") + B->getName() +
               "' can not be undefined in a subtraction expression");
       return;
     }
 
     if (!A.getFragment()) {
       Asm.getContext().reportError(
           Fixup.getLoc(),
-          Twine("symbol '") + Symbol.getName() +
+          Twine("symbol '") + A.getName() +
               "' can not be undefined in a subtraction expression");
       return;
     }
 
-    CrossSection = &Symbol.getSection() != &B->getSection();
+    CrossSection = &A.getSection() != &B->getSection();
 
     // Offset of the symbol in the section
     int64_t OffsetOfB = Layout.getSymbolOffset(*B);
 
     // In the case where we have SymbA and SymB, we just need to store the delta
     // between the two symbols.  Update FixedValue to account for the delta, and
     // skip recording the relocation.
     if (!CrossSection) {
void WinCOFFObjectWriter::recordRelocation(
   }
 
   COFFRelocation Reloc;
 
   Reloc.Data.SymbolTableIndex = 0;
   Reloc.Data.VirtualAddress = Layout.getFragmentOffset(Fragment);
 
   // Turn relocations for temporary symbols into section relocations.
-  if (coff_symbol->MC->isTemporary() || CrossSection) {
-    Reloc.Symb = coff_symbol->Section->Symbol;
-    FixedValue += Layout.getFragmentOffset(coff_symbol->MC->getFragment()) +
-                  coff_symbol->MC->getOffset();
-  } else
-    Reloc.Symb = coff_symbol;
+  if (A.isTemporary() || CrossSection) {
+    MCSection *TargetSection = &A.getSection();
+    assert(
+        SectionMap.find(TargetSection) != SectionMap.end() &&
+        "Section must already have been defined in executePostLayoutBinding!");
+    Reloc.Symb = SectionMap[TargetSection]->Symbol;
+    FixedValue += Layout.getSymbolOffset(A);
+  } else {
+    assert(
+        SymbolMap.find(&A) != SymbolMap.end() &&
+        "Symbol must already have been defined in executePostLayoutBinding!");
+    Reloc.Symb = SymbolMap[&A];
+  }
 
   ++Reloc.Symb->Relocations;
 
   Reloc.Data.VirtualAddress += Fixup.getOffset();
   Reloc.Data.Type = TargetObjectWriter->getRelocType(
       Target, Fixup, CrossSection, Asm.getBackend());
 
   // FIXME: Can anyone explain what this does other than adjust for the size
for (auto &Aux : file->Aux) {
       Offset += SymbolSize;
     }
   }
 
   for (auto &Symbol : Symbols) {
     // Update section number & offset for symbols that have them.
     if (Symbol->Section)
       Symbol->Data.SectionNumber = Symbol->Section->Number;
-    if (Symbol->should_keep()) {
-      Symbol->setIndex(Header.NumberOfSymbols++);
-      // Update auxiliary symbol info.
-      Symbol->Data.NumberOfAuxSymbols = Symbol->Aux.size();
-      Header.NumberOfSymbols += Symbol->Data.NumberOfAuxSymbols;
-    } else {
-      Symbol->setIndex(-1);
-    }
   }
 
   // Build string table.
   for (const auto &S : Sections)
     if (S->Name.size() > COFF::NameSize)
       Strings.add(S->Name);
   for (const auto &S : Symbols)
-    if (S->should_keep() && S->Name.size() > COFF::NameSize)
+    if (S->Name.size() > COFF::NameSize)
       Strings.add(S->Name);
   Strings.finalize();
 
   // Set names.
   for (const auto &S : Sections)
     SetSectionName(*S);
   for (auto &S : Symbols)
-    if (S->should_keep())
-      SetSymbolName(*S);
 
   // Fixup weak external references.
   for (auto &Symbol : Symbols) {
     if (Symbol->Other) {
       assert(Symbol->getIndex() != -1);
       assert(Symbol->Aux.size() == 1 && "Symbol must contain one aux symbol!");
       assert(Symbol->Aux[0].AuxType == ATWeakExternal &&
              "Symbol's aux symbol must be a Weak External!");

test/MC/COFF/temporary-alias.s

+// RUN: llvm-mc -triple=i686-pc-windows -filetype=obj -o %t %s
+// RUN: llvm-objdump -d -r %t | FileCheck %s
+
+.globl _main
+_main:
+// CHECK: 00 00 00 00
+// CHECK-NEXT: 00000002:  IMAGE_REL_I386_DIR32 .rdata
+movb L_alias1(%eax), %al
+// CHECK: 01 00 00 00
+// CHECK-NEXT: 00000008:  IMAGE_REL_I386_DIR32 .rdata
+movb L_alias2(%eax), %al
+retl
+
+.section .rdata,"dr"
+L_sym1:
+.ascii "\001"
+L_sym2:
+.ascii "\002"
+
+L_alias1 = L_sym1
+L_alias2 = L_sym2