[llvm] r233595 - MC: For variable symbols, maintain MCSymbol::Section as a cache.
Peter Collingbourne
peter at pcc.me.uk
Wed Apr 1 13:27:16 PDT 2015
Tim approved this on IRC. Apologies if the commit message was not
clear, the new tests show a couple of cases that we were getting
wrong before. I also elaborated in the discussion in the code review
(http://reviews.llvm.org/D8586).
Peter
On Wed, Apr 01, 2015 at 01:15:36PM -0700, Jim Grosbach wrote:
> 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
>
>
--
Peter
More information about the llvm-commits
mailing list