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

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


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 <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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150401/9a19f34d/attachment.html>


More information about the llvm-commits mailing list