[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