[llvm] r233595 - MC: For variable symbols, maintain MCSymbol::Section as a cache.
Peter Collingbourne
peter at pcc.me.uk
Wed Apr 1 14:13:30 PDT 2015
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 <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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150401/6a388626/attachment.html>
More information about the llvm-commits
mailing list