[llvm] r233595 - MC: For variable symbols, maintain MCSymbol::Section as a cache.

Jim Grosbach grosbach at apple.com
Wed Apr 1 13:15:36 PDT 2015


Uh….. I had not signed off on this change and had already objected to the completely insufficient commit message, which you have not corrected. Why?


> On Mar 30, 2015, at 1:41 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> 
> Author: pcc
> Date: Mon Mar 30 15:41:21 2015
> New Revision: 233595
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=233595&view=rev
> Log:
> MC: For variable symbols, maintain MCSymbol::Section as a cache.
> 
> This fixes the visibility of symbols in certain edge cases involving aliases
> with multiple levels of indirection.
> 
> Fixes PR19582.
> 
> Differential Revision: http://reviews.llvm.org/D8586
> 
> Added:
>    llvm/trunk/test/MC/ELF/alias-resolve.s
> Modified:
>    llvm/trunk/include/llvm/MC/MCMachObjectWriter.h
>    llvm/trunk/include/llvm/MC/MCSymbol.h
>    llvm/trunk/lib/MC/MCExpr.cpp
>    llvm/trunk/lib/MC/MCSymbol.cpp
>    llvm/trunk/lib/MC/MachObjectWriter.cpp
>    llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp
>    llvm/trunk/test/MC/ELF/alias.s
>    llvm/trunk/test/MC/MachO/ARM/aliased-symbols.s
> 
> Modified: llvm/trunk/include/llvm/MC/MCMachObjectWriter.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCMachObjectWriter.h?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/MC/MCMachObjectWriter.h (original)
> +++ llvm/trunk/include/llvm/MC/MCMachObjectWriter.h Mon Mar 30 15:41:21 2015
> @@ -257,8 +257,6 @@ public:
>   void computeSectionAddresses(const MCAssembler &Asm,
>                                const MCAsmLayout &Layout);
> 
> -  void markAbsoluteVariableSymbols(MCAssembler &Asm,
> -                                   const MCAsmLayout &Layout);
>   void ExecutePostLayoutBinding(MCAssembler &Asm,
>                                 const MCAsmLayout &Layout) override;
> 
> 
> Modified: llvm/trunk/include/llvm/MC/MCSymbol.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSymbol.h?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/MC/MCSymbol.h (original)
> +++ llvm/trunk/include/llvm/MC/MCSymbol.h Mon Mar 30 15:41:21 2015
> @@ -15,6 +15,7 @@
> #define LLVM_MC_MCSYMBOL_H
> 
> #include "llvm/ADT/StringRef.h"
> +#include "llvm/MC/MCExpr.h"
> #include "llvm/Support/Compiler.h"
> 
> namespace llvm {
> @@ -42,8 +43,9 @@ namespace llvm {
> 
>     /// Section - The section the symbol is defined in. This is null for
>     /// undefined symbols, and the special AbsolutePseudoSection value for
> -    /// absolute symbols.
> -    const MCSection *Section;
> +    /// absolute symbols. If this is a variable symbol, this caches the
> +    /// variable value's section.
> +    mutable const MCSection *Section;
> 
>     /// Value - If non-null, the value for a variable symbol.
>     const MCExpr *Value;
> @@ -68,6 +70,12 @@ namespace llvm {
> 
>     MCSymbol(const MCSymbol&) = delete;
>     void operator=(const MCSymbol&) = delete;
> +    const MCSection *getSectionPtr() const {
> +      if (Section || !Value)
> +        return Section;
> +      return Section = Value->FindAssociatedSection();
> +    }
> +
>   public:
>     /// getName - Get the symbol name.
>     StringRef getName() const { return Name; }
> @@ -103,7 +111,7 @@ namespace llvm {
>     ///
>     /// Defined symbols are either absolute or in some section.
>     bool isDefined() const {
> -      return Section != nullptr;
> +      return getSectionPtr() != nullptr;
>     }
> 
>     /// isInSection - Check if this symbol is defined in some section (i.e., it
> @@ -119,27 +127,27 @@ namespace llvm {
> 
>     /// isAbsolute - Check if this is an absolute symbol.
>     bool isAbsolute() const {
> -      return Section == AbsolutePseudoSection;
> +      return getSectionPtr() == AbsolutePseudoSection;
>     }
> 
>     /// getSection - Get the section associated with a defined, non-absolute
>     /// symbol.
>     const MCSection &getSection() const {
>       assert(isInSection() && "Invalid accessor!");
> -      return *Section;
> +      return *getSectionPtr();
>     }
> 
>     /// setSection - Mark the symbol as defined in the section \p S.
> -    void setSection(const MCSection &S) { Section = &S; }
> +    void setSection(const MCSection &S) {
> +      assert(!isVariable() && "Cannot set section of variable");
> +      Section = &S;
> +    }
> 
>     /// setUndefined - Mark the symbol as undefined.
>     void setUndefined() {
>       Section = nullptr;
>     }
> 
> -    /// setAbsolute - Mark the symbol as absolute.
> -    void setAbsolute() { Section = AbsolutePseudoSection; }
> -
>     /// @}
>     /// @name Variable Symbols
>     /// @{
> 
> Modified: llvm/trunk/lib/MC/MCExpr.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCExpr.cpp?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/lib/MC/MCExpr.cpp (original)
> +++ llvm/trunk/lib/MC/MCExpr.cpp Mon Mar 30 15:41:21 2015
> @@ -775,6 +775,9 @@ const MCSection *MCExpr::FindAssociatedS
>     if (RHS_S == MCSymbol::AbsolutePseudoSection)
>       return LHS_S;
> 
> +    if (BE->getOpcode() == MCBinaryExpr::Sub)
> +      return MCSymbol::AbsolutePseudoSection;
> +
>     // Otherwise, return the first non-null section.
>     return LHS_S ? LHS_S : RHS_S;
>   }
> 
> Modified: llvm/trunk/lib/MC/MCSymbol.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSymbol.cpp?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/lib/MC/MCSymbol.cpp (original)
> +++ llvm/trunk/lib/MC/MCSymbol.cpp Mon Mar 30 15:41:21 2015
> @@ -55,13 +55,7 @@ void MCSymbol::setVariableValue(const MC
>   assert(!IsUsed && "Cannot set a variable that has already been used.");
>   assert(Value && "Invalid variable value!");
>   this->Value = Value;
> -
> -  // Variables should always be marked as in the same "section" as the value.
> -  const MCSection *Section = Value->FindAssociatedSection();
> -  if (Section)
> -    setSection(*Section);
> -  else
> -    setUndefined();
> +  this->Section = nullptr;
> }
> 
> void MCSymbol::print(raw_ostream &OS) const {
> 
> Modified: llvm/trunk/lib/MC/MachObjectWriter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MachObjectWriter.cpp?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/lib/MC/MachObjectWriter.cpp (original)
> +++ llvm/trunk/lib/MC/MachObjectWriter.cpp Mon Mar 30 15:41:21 2015
> @@ -649,33 +649,12 @@ void MachObjectWriter::computeSectionAdd
>   }
> }
> 
> -void MachObjectWriter::markAbsoluteVariableSymbols(MCAssembler &Asm,
> -                                                   const MCAsmLayout &Layout) {
> -  for (MCSymbolData &SD : Asm.symbols()) {
> -    if (!SD.getSymbol().isVariable())
> -      continue;
> -
> -    // Is the variable is a symbol difference (SA - SB + C) expression,
> -    // and neither symbol is external, mark the variable as absolute.
> -    const MCExpr *Expr = SD.getSymbol().getVariableValue();
> -    MCValue Value;
> -    if (Expr->EvaluateAsRelocatable(Value, &Layout, nullptr)) {
> -      if (Value.getSymA() && Value.getSymB())
> -        const_cast<MCSymbol*>(&SD.getSymbol())->setAbsolute();
> -    }
> -  }
> -}
> -
> void MachObjectWriter::ExecutePostLayoutBinding(MCAssembler &Asm,
>                                                 const MCAsmLayout &Layout) {
>   computeSectionAddresses(Asm, Layout);
> 
>   // Create symbol data for any indirect symbols.
>   BindIndirectSymbols(Asm);
> -
> -  // Mark symbol difference expressions in variables (from .set or = directives)
> -  // as absolute.
> -  markAbsoluteVariableSymbols(Asm, Layout);
> }
> 
> bool MachObjectWriter::
> 
> Modified: llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp Mon Mar 30 15:41:21 2015
> @@ -523,7 +523,6 @@ void X86AsmPrinter::EmitStartOfAsmFile(M
>       // must be registered in .sxdata.  Use of any unregistered handlers will
>       // cause the process to terminate immediately.  LLVM does not know how to
>       // register any SEH handlers, so its object files should be safe.
> -      S->setAbsolute();
>       OutStreamer.EmitSymbolAttribute(S, MCSA_Global);
>       OutStreamer.EmitAssignment(
>           S, MCConstantExpr::Create(int64_t(1), MMI->getContext()));
> 
> Added: llvm/trunk/test/MC/ELF/alias-resolve.s
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ELF/alias-resolve.s?rev=233595&view=auto
> ==============================================================================
> --- llvm/trunk/test/MC/ELF/alias-resolve.s (added)
> +++ llvm/trunk/test/MC/ELF/alias-resolve.s Mon Mar 30 15:41:21 2015
> @@ -0,0 +1,8 @@
> +// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-readobj -r | FileCheck %s
> +
> +a:
> +    .section foo
> +    c = b
> +b:
> +    // CHECK: 0x0 R_X86_64_PC32 .text 0x0
> +    .long a - c
> 
> Modified: llvm/trunk/test/MC/ELF/alias.s
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ELF/alias.s?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/test/MC/ELF/alias.s (original)
> +++ llvm/trunk/test/MC/ELF/alias.s Mon Mar 30 15:41:21 2015
> @@ -24,6 +24,15 @@ bar5 = bar4
> bar6 = bar5
> bar6:
> 
> +// Test that indirect local aliases do not appear as symbols.
> +.data
> +.Llocal:
> +
> +.text
> +leaq .Llocal1(%rip), %rdi
> +.Llocal1 = .Llocal2
> +.Llocal2 = .Llocal
> +
> // CHECK:      Symbols [
> // CHECK-NEXT:   Symbol {
> // CHECK-NEXT:     Name:  (0)
> 
> Modified: llvm/trunk/test/MC/MachO/ARM/aliased-symbols.s
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/MachO/ARM/aliased-symbols.s?rev=233595&r1=233594&r2=233595&view=diff
> ==============================================================================
> --- llvm/trunk/test/MC/MachO/ARM/aliased-symbols.s (original)
> +++ llvm/trunk/test/MC/MachO/ARM/aliased-symbols.s Mon Mar 30 15:41:21 2015
> @@ -45,9 +45,9 @@ Ltmp0:
> // CHECK-NEXT:   Value: 0x[[DEFINED_EARLY]]
> // CHECK-NEXT: }
> 
> -        // defined_late was defined. Just after defined_early.
> +        // alias_to_late was an alias to defined_late. But we can resolve it.
> // CHECK: Symbol {
> -// CHECK-NEXT:   Name: defined_late
> +// CHECK-NEXT:   Name: alias_to_late
> // CHECK-NEXT:   Type: Section (0xE)
> // CHECK-NEXT:   Section: __data (0x2)
> // CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
> @@ -56,9 +56,9 @@ Ltmp0:
> // CHECK-NEXT:   Value: 0x[[DEFINED_LATE:[0-9A-F]+]]
> // CHECK-NEXT: }
> 
> -        // alias_to_late was an alias to defined_late. But we can resolve it.
> +        // defined_late was defined. Just after defined_early.
> // CHECK: Symbol {
> -// CHECK-NEXT:   Name: alias_to_late
> +// CHECK-NEXT:   Name: defined_late
> // CHECK-NEXT:   Type: Section (0xE)
> // CHECK-NEXT:   Section: __data (0x2)
> // CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
> @@ -72,7 +72,7 @@ Ltmp0:
> // CHECK: Symbol {
> // CHECK-NEXT:   Name: alias_to_local (42)
> // CHECK-NEXT:   Type: Section (0xE)
> -// CHECK-NEXT:   Section:  (0x0)
> +// CHECK-NEXT:   Section: __data (0x2)
> // CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
> // CHECK-NEXT:   Flags [ (0x0)
> // CHECK-NEXT:   ]
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits





More information about the llvm-commits mailing list