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

Eric Christopher echristo at gmail.com
Wed Apr 1 23:30:39 PDT 2015


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150402/187bbb7d/attachment.html>


More information about the llvm-commits mailing list