[llvm] r233595 - MC: For variable symbols, maintain MCSymbol::Section as a cache.
Peter Collingbourne
peter at pcc.me.uk
Thu Apr 2 00:51:47 PDT 2015
Reverted in r233898 and sent out a fresh review in http://reviews.llvm.org/D8798
On Thu, Apr 02, 2015 at 06:30:39AM +0000, Eric Christopher wrote:
> Hi Peter,
>
> I think at this point it's best to revert and work with Jim before
> recommitting.
>
> Thanks!
>
> -eric
>
> On Wed, Apr 1, 2015 at 2:19 PM Peter Collingbourne <peter at pcc.me.uk> wrote:
>
> > Fair enough. Perhaps in this case i overassumed that Tim would be able to
> > approve the mach-o changes.
> >
> > Do you have any further specific concerns around the patch?
> >
> > Peter
> >
> >
> >
> >
> > On April 1, 2015 1:36:13 PM PDT, Jim Grosbach <grosbach at apple.com> wrote:
> >>
> >> The whole point of my question was that the commit message needs to be
> >> explicit about a) what the problem is and b) why the patch fixes it and is
> >> the right way to do so. That was a pre-requisite to actually being able to
> >> review the patch itself. The comments in phab helped in that regard, but
> >> that was the beginning of the conversation, not the conclusion. Before
> >> committing, you need to make sure that reviewers' comments have been
> >> addressed and that they have explicitly acknowledged such.
> >>
> >>
> >>
> >> On Apr 1, 2015, at 1:27 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> >>
> >> 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
> >>
> >>
> >>
> > --
> > Sent from my Android device with K-9 Mail. Please excuse my brevity.
> > _______________________________________________
> > 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