[PATCH] D14975: MC: Simplify handling of temporary symbols in COFF writer.
Peter Collingbourne via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 25 17:14:55 PST 2015
(FYI: this should unbreak the sanitizer-windows bot, which has been broken
since I added the CFI tests.)
On Wed, Nov 25, 2015 at 02:58:46AM +0000, Peter Collingbourne wrote:
> pcc created this revision.
> pcc added reviewers: rnk, majnemer, rafael.
> pcc added a subscriber: llvm-commits.
>
> 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.
>
> http://reviews.llvm.org/D14975
>
> Files:
> lib/MC/WinCOFFObjectWriter.cpp
> test/MC/COFF/temporary-alias.s
>
> Index: test/MC/COFF/temporary-alias.s
> ===================================================================
> --- /dev/null
> +++ test/MC/COFF/temporary-alias.s
> @@ -0,0 +1,21 @@
> +// 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
> Index: lib/MC/WinCOFFObjectWriter.cpp
> ===================================================================
> --- lib/MC/WinCOFFObjectWriter.cpp
> +++ lib/MC/WinCOFFObjectWriter.cpp
> @@ -78,8 +78,6 @@
> 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;
> @@ -162,8 +160,6 @@
> void SetSymbolName(COFFSymbol &S);
> void SetSectionName(COFFSection &S);
>
> - bool ExportSymbol(const MCSymbol &Symbol, MCAssembler &Asm);
> -
> bool IsPhysicalSection(COFFSection *S);
>
> // Entity writing methods.
> @@ -217,38 +213,6 @@
> 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
>
> @@ -394,7 +358,6 @@
> 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;
> @@ -517,25 +480,6 @@
> 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;
> @@ -665,7 +609,7 @@
> defineSection(static_cast<const MCSectionCOFF &>(Section));
>
> for (const MCSymbol &Symbol : Asm.symbols())
> - if (ExportSymbol(Symbol, Asm))
> + if (!Symbol.isTemporary())
> DefineSymbol(Symbol, Asm, Layout);
> }
>
> @@ -704,8 +648,7 @@
> 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() +
> @@ -724,11 +667,8 @@
> // 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;
>
> @@ -745,12 +685,12 @@
> 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);
> @@ -779,12 +719,19 @@
> 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;
>
> @@ -898,31 +845,26 @@
> // 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);
> - }
> + Symbol->setIndex(Header.NumberOfSymbols++);
> + // Update auxiliary symbol info.
> + Symbol->Data.NumberOfAuxSymbols = Symbol->Aux.size();
> + Header.NumberOfSymbols += Symbol->Data.NumberOfAuxSymbols;
> }
>
> // 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);
> + SetSymbolName(*S);
>
> // Fixup weak external references.
> for (auto &Symbol : Symbols) {
--
Peter
More information about the llvm-commits
mailing list