[llvm] r203962 - Correctly handle an ELF symbol defined with "a = b + expr".
Alexander Kornienko
alexfh at google.com
Tue Mar 18 03:35:00 PDT 2014
This blocks us and also breaks sanitizer bootstrap:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/2651(and
checked locally by Alexey).
It's been many hours without a fix. Sorry, I'm going to revert this
revision and two dependent revisions: r204028 and r204059. I'm adding a
test for the broken behavior, so you can reproduce and fix it.
On Tue, Mar 18, 2014 at 12:43 AM, Alexander Kornienko <alexfh at google.com>wrote:
> Here's a small repro (clang-3.5-bad is current HEAD, clang-3.5-good is
> built after svn up -r203961 lib/MC/MCSymbol.cpp
> include/llvm/MC/MCSymbol.h lib/MC/ELFObjectWriter.cpp):
>
> $ cat q.c
> void a1();
> void a2() __attribute__((alias("a1")));
> void a3() __attribute__((alias("a2")));
> void a1() {}
>
> $ ~/work/llvm-build/bin/clang-3.5-good -c q.c && mv q.o good.o &&
> ~/work/llvm-build/bin/clang-3.5-bad -c q.c && mv q.o bad.o && objdump -t
> good.o bad.o
>
> good.o: file format elf64-x86-64
>
> SYMBOL TABLE:
> 0000000000000000 l df *ABS* 0000000000000000 q.c
> 0000000000000000 l d .text 0000000000000000 .text
> 0000000000000000 l d .data 0000000000000000 .data
> 0000000000000000 l d .bss 0000000000000000 .bss
> 0000000000000000 l d .comment 0000000000000000 .comment
> 0000000000000000 l d .note.GNU-stack 0000000000000000
> .note.GNU-stack
> 0000000000000000 l d .eh_frame 0000000000000000 .eh_frame
> 0000000000000000 g F .text 0000000000000006 a1
> 0000000000000000 g F .text 0000000000000006 a2
> 0000000000000000 g F .text 0000000000000006 a3
>
>
>
> bad.o: file format elf64-x86-64
>
> SYMBOL TABLE:
> 0000000000000000 l df *ABS* 0000000000000000 q.c
> 0000000000000000 l d .text 0000000000000000 .text
> 0000000000000000 l d .data 0000000000000000 .data
> 0000000000000000 l d .bss 0000000000000000 .bss
> 0000000000000000 l d .comment 0000000000000000 .comment
> 0000000000000000 l d .note.GNU-stack 0000000000000000
> .note.GNU-stack
> 0000000000000000 l d .eh_frame 0000000000000000 .eh_frame
> 0000000000000000 g F .text 0000000000000006 a1
> 0000000000000000 g F .text 0000000000000006 a2
> 0000000000000000 g .text 0000000000000000 a3
>
>
> Difference is in the flags of the symbol a3.
>
> Could you please fix the problem or revert the commit?
>
> Thanks!
>
> On Mon, Mar 17, 2014 at 8:05 PM, Alexander Kornienko <alexfh at google.com>wrote:
>
>> On Mon, Mar 17, 2014 at 5:17 PM, Alexander Kornienko <alexfh at google.com>wrote:
>>
>>> This commit breaks something in our code base. Unfortunately, I don't
>>> know what exactly and I'm struggling to come up with a reasonably-sized
>>> reproduction. Once I have something specific, I'll let you know.
>>>
>>> +David, who may be interested in this.
>>
>> I've found a translation unit, which is handled differently before and
>> after this revision. The difference is that after this revision, the
>> functions marked with __attribute__((alias("..."))) loose the "function"
>> flag (i.e. they get space instead of 'F' in the third column of the objdump
>> -t output). Is this enough or you need more information to get this fixed?
>>
>>
>>>
>>>
>>> On Fri, Mar 14, 2014 at 9:09 PM, Rafael Espindola <
>>> rafael.espindola at gmail.com> wrote:
>>>
>>>> Author: rafael
>>>> Date: Fri Mar 14 15:09:04 2014
>>>> New Revision: 203962
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=203962&view=rev
>>>> Log:
>>>> Correctly handle an ELF symbol defined with "a = b + expr".
>>>>
>>>> We were marking the symbol as absolute instead of computing b's offset
>>>> + the
>>>> expression value.
>>>>
>>>> This fixes pr19126.
>>>>
>>>> Added:
>>>> llvm/trunk/test/MC/ELF/offset.s
>>>> Modified:
>>>> llvm/trunk/include/llvm/MC/MCSymbol.h
>>>> llvm/trunk/lib/MC/ELFObjectWriter.cpp
>>>> llvm/trunk/lib/MC/MCSymbol.cpp
>>>>
>>>> Modified: llvm/trunk/include/llvm/MC/MCSymbol.h
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCSymbol.h?rev=203962&r1=203961&r2=203962&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/MC/MCSymbol.h (original)
>>>> +++ llvm/trunk/include/llvm/MC/MCSymbol.h Fri Mar 14 15:09:04 2014
>>>> @@ -18,6 +18,7 @@
>>>> #include "llvm/Support/Compiler.h"
>>>>
>>>> namespace llvm {
>>>> + class MCAsmLayout;
>>>> class MCExpr;
>>>> class MCSection;
>>>> class MCContext;
>>>> @@ -145,6 +146,11 @@ namespace llvm {
>>>> // itself.
>>>> const MCSymbol &AliasedSymbol() const;
>>>>
>>>> + // If this symbol is not a variable, return itself. If it is a
>>>> variable,
>>>> + // evaluate it and check if it is of the form Base +
>>>> ConstantOffset. If so,
>>>> + // return Base, if not, return nullptr.
>>>> + const MCSymbol *getBaseSymbol(const MCAsmLayout &Layout) const;
>>>> +
>>>> void setVariableValue(const MCExpr *Value);
>>>>
>>>> /// @}
>>>>
>>>> Modified: llvm/trunk/lib/MC/ELFObjectWriter.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/ELFObjectWriter.cpp?rev=203962&r1=203961&r2=203962&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/MC/ELFObjectWriter.cpp (original)
>>>> +++ llvm/trunk/lib/MC/ELFObjectWriter.cpp Fri Mar 14 15:09:04 2014
>>>> @@ -271,6 +271,7 @@ class ELFObjectWriter : public MCObjectW
>>>> /// \param RevGroupMap - Maps a signature symbol to the group
>>>> section.
>>>> /// \param NumRegularSections - Number of non-relocation sections.
>>>> void ComputeSymbolTable(MCAssembler &Asm,
>>>> + const MCAsmLayout &Layout,
>>>> const SectionIndexMapTy &SectionIndexMap,
>>>> RevGroupMapTy RevGroupMap,
>>>> unsigned NumRegularSections);
>>>> @@ -462,33 +463,43 @@ void ELFObjectWriter::WriteSymbolEntry(M
>>>> }
>>>> }
>>>>
>>>> -uint64_t ELFObjectWriter::SymbolValue(MCSymbolData &Data,
>>>> +uint64_t ELFObjectWriter::SymbolValue(MCSymbolData &OrigData,
>>>> const MCAsmLayout &Layout) {
>>>> - if (Data.isCommon() && Data.isExternal())
>>>> - return Data.getCommonAlignment();
>>>> -
>>>> - const MCSymbol &Symbol = Data.getSymbol();
>>>> + MCSymbolData *Data = &OrigData;
>>>> + if (Data->isCommon() && Data->isExternal())
>>>> + return Data->getCommonAlignment();
>>>> +
>>>> + const MCSymbol *Symbol = &Data->getSymbol();
>>>> +
>>>> + uint64_t Res = 0;
>>>> + if (Symbol->isVariable()) {
>>>> + const MCExpr *Expr = Symbol->getVariableValue();
>>>> + MCValue Value;
>>>> + if (!Expr->EvaluateAsRelocatable(Value, &Layout))
>>>> + return 0;
>>>> + if (Value.getSymB())
>>>> + return 0;
>>>> + Res = Value.getConstant();
>>>> +
>>>> + const MCSymbolRefExpr *A = Value.getSymA();
>>>> + if (!A)
>>>> + return Res;
>>>>
>>>> - if (Symbol.isAbsolute() && Symbol.isVariable()) {
>>>> - if (const MCExpr *Value = Symbol.getVariableValue()) {
>>>> - int64_t IntValue;
>>>> - if (Value->EvaluateAsAbsolute(IntValue, Layout))
>>>> - return (uint64_t)IntValue;
>>>> - }
>>>> + Symbol = &A->getSymbol();
>>>> + Data = &Layout.getAssembler().getSymbolData(*Symbol);
>>>> }
>>>>
>>>> - if (!Symbol.isInSection())
>>>> + if (!Symbol->isInSection())
>>>> return 0;
>>>>
>>>> + if (!Data->getFragment())
>>>> + return 0;
>>>>
>>>> - if (Data.getFragment()) {
>>>> - if (Data.getFlags() & ELF_Other_ThumbFunc)
>>>> - return Layout.getSymbolOffset(&Data)+1;
>>>> - else
>>>> - return Layout.getSymbolOffset(&Data);
>>>> - }
>>>> + Res += Layout.getSymbolOffset(Data);
>>>> + if (Data->getFlags() & ELF_Other_ThumbFunc)
>>>> + ++Res;
>>>>
>>>> - return 0;
>>>> + return Res;
>>>> }
>>>>
>>>> void ELFObjectWriter::ExecutePostLayoutBinding(MCAssembler &Asm,
>>>> @@ -586,7 +597,7 @@ void ELFObjectWriter::WriteSymbol(MCData
>>>> uint8_t Other = MCELF::getOther(OrigData) << (ELF_STO_Shift -
>>>> ELF_STV_Shift);
>>>> Other |= Visibility;
>>>>
>>>> - uint64_t Value = SymbolValue(Data, Layout);
>>>> + uint64_t Value = SymbolValue(OrigData, Layout);
>>>> uint64_t Size = 0;
>>>>
>>>> assert(!(Data.isCommon() && !Data.isExternal()));
>>>> @@ -897,10 +908,11 @@ void ELFObjectWriter::ComputeIndexMap(MC
>>>> }
>>>> }
>>>>
>>>> -void ELFObjectWriter::ComputeSymbolTable(MCAssembler &Asm,
>>>> - const SectionIndexMapTy
>>>> &SectionIndexMap,
>>>> - RevGroupMapTy RevGroupMap,
>>>> - unsigned NumRegularSections) {
>>>> +void
>>>> +ELFObjectWriter::ComputeSymbolTable(MCAssembler &Asm, const
>>>> MCAsmLayout &Layout,
>>>> + const SectionIndexMapTy
>>>> &SectionIndexMap,
>>>> + RevGroupMapTy RevGroupMap,
>>>> + unsigned NumRegularSections) {
>>>> // FIXME: Is this the correct place to do this?
>>>> // FIXME: Why is an undefined reference to _GLOBAL_OFFSET_TABLE_
>>>> needed?
>>>> if (NeedsGOT) {
>>>> @@ -948,33 +960,33 @@ void ELFObjectWriter::ComputeSymbolTable
>>>>
>>>> ELFSymbolData MSD;
>>>> MSD.SymbolData = it;
>>>> - const MCSymbol &RefSymbol = Symbol.AliasedSymbol();
>>>> + const MCSymbol *BaseSymbol = Symbol.getBaseSymbol(Layout);
>>>>
>>>> // Undefined symbols are global, but this is the first place we
>>>> // are able to set it.
>>>> bool Local = isLocal(*it, isSignature, Used);
>>>> if (!Local && MCELF::GetBinding(*it) == ELF::STB_LOCAL) {
>>>> - MCSymbolData &SD = Asm.getSymbolData(RefSymbol);
>>>> + assert(BaseSymbol);
>>>> + MCSymbolData &SD = Asm.getSymbolData(*BaseSymbol);
>>>> MCELF::SetBinding(*it, ELF::STB_GLOBAL);
>>>> MCELF::SetBinding(SD, ELF::STB_GLOBAL);
>>>> }
>>>>
>>>> - if (RefSymbol.isUndefined() && !Used && WeakrefUsed)
>>>> - MCELF::SetBinding(*it, ELF::STB_WEAK);
>>>> -
>>>> - if (it->isCommon()) {
>>>> + if (!BaseSymbol) {
>>>> + MSD.SectionIndex = ELF::SHN_ABS;
>>>> + } else if (it->isCommon()) {
>>>> assert(!Local);
>>>> MSD.SectionIndex = ELF::SHN_COMMON;
>>>> - } else if (Symbol.isAbsolute() || RefSymbol.isVariable()) {
>>>> - MSD.SectionIndex = ELF::SHN_ABS;
>>>> - } else if (RefSymbol.isUndefined()) {
>>>> + } else if (BaseSymbol->isUndefined()) {
>>>> if (isSignature && !Used)
>>>> MSD.SectionIndex =
>>>> SectionIndexMap.lookup(RevGroupMap[&Symbol]);
>>>> else
>>>> MSD.SectionIndex = ELF::SHN_UNDEF;
>>>> + if (!Used && WeakrefUsed)
>>>> + MCELF::SetBinding(*it, ELF::STB_WEAK);
>>>> } else {
>>>> const MCSectionELF &Section =
>>>> - static_cast<const MCSectionELF&>(RefSymbol.getSection());
>>>> + static_cast<const MCSectionELF&>(BaseSymbol->getSection());
>>>> MSD.SectionIndex = SectionIndexMap.lookup(&Section);
>>>> if (MSD.SectionIndex >= ELF::SHN_LORESERVE)
>>>> NeedsSymtabShndx = true;
>>>> @@ -1560,8 +1572,8 @@ void ELFObjectWriter::WriteObject(MCAsse
>>>> unsigned NumRegularSections = NumUserSections + NumIndexedSections;
>>>>
>>>> // Compute symbol table information.
>>>> - ComputeSymbolTable(Asm, SectionIndexMap, RevGroupMap,
>>>> NumRegularSections);
>>>> -
>>>> + ComputeSymbolTable(Asm, Layout, SectionIndexMap, RevGroupMap,
>>>> + NumRegularSections);
>>>>
>>>> WriteRelocations(Asm, const_cast<MCAsmLayout&>(Layout), RelMap);
>>>>
>>>>
>>>> Modified: llvm/trunk/lib/MC/MCSymbol.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSymbol.cpp?rev=203962&r1=203961&r2=203962&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/MC/MCSymbol.cpp (original)
>>>> +++ llvm/trunk/lib/MC/MCSymbol.cpp Fri Mar 14 15:09:04 2014
>>>> @@ -9,6 +9,7 @@
>>>>
>>>> #include "llvm/MC/MCSymbol.h"
>>>> #include "llvm/MC/MCExpr.h"
>>>> +#include "llvm/MC/MCValue.h"
>>>> #include "llvm/Support/Debug.h"
>>>> #include "llvm/Support/raw_ostream.h"
>>>> using namespace llvm;
>>>> @@ -51,6 +52,22 @@ const MCSymbol &MCSymbol::AliasedSymbol(
>>>> return *S;
>>>> }
>>>>
>>>> +const MCSymbol *MCSymbol::getBaseSymbol(const MCAsmLayout &Layout)
>>>> const {
>>>> + if (!isVariable())
>>>> + return this;
>>>> +
>>>> + const MCExpr *Expr = getVariableValue();
>>>> + MCValue Value;
>>>> + if (!Expr->EvaluateAsRelocatable(Value, &Layout))
>>>> + return nullptr;
>>>> +
>>>> + if (Value.getSymB())
>>>> + return nullptr;
>>>> + if (const MCSymbolRefExpr *A = Value.getSymA())
>>>> + return &A->getSymbol();
>>>> + return nullptr;
>>>> +}
>>>> +
>>>> void MCSymbol::setVariableValue(const MCExpr *Value) {
>>>> assert(!IsUsed && "Cannot set a variable that has already been
>>>> used.");
>>>> assert(Value && "Invalid variable value!");
>>>>
>>>> Added: llvm/trunk/test/MC/ELF/offset.s
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ELF/offset.s?rev=203962&view=auto
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/MC/ELF/offset.s (added)
>>>> +++ llvm/trunk/test/MC/ELF/offset.s Fri Mar 14 15:09:04 2014
>>>> @@ -0,0 +1,58 @@
>>>> +// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - |
>>>> llvm-readobj -t - | FileCheck %s
>>>> +
>>>> +// Test that a variable declared with "var = other_var + cst" is in
>>>> the same
>>>> +// section as other_var and its value is the value of other_var + cst.
>>>> +
>>>> + .data
>>>> + .globl sym_a
>>>> + .byte 42
>>>> +sym_a:
>>>> +
>>>> +// CHECK: Symbol {
>>>> +// CHECK: Name: sym_a
>>>> +// CHECK-NEXT: Value: 0x1
>>>> +// CHECK-NEXT: Size: 0
>>>> +// CHECK-NEXT: Binding: Global
>>>> +// CHECK-NEXT: Type: None
>>>> +// CHECK-NEXT: Other: 0
>>>> +// CHECK-NEXT: Section: .data
>>>> +// CHECK-NEXT: }
>>>> +
>>>> + .long 42
>>>> + .globl sym_b
>>>> +sym_b:
>>>> + .globl sym_c
>>>> +sym_c = sym_a
>>>> +// CHECK: Symbol {
>>>> +// CHECK: Name: sym_c
>>>> +// CHECK-NEXT: Value: 0x1
>>>> +// CHECK-NEXT: Size: 0
>>>> +// CHECK-NEXT: Binding: Global
>>>> +// CHECK-NEXT: Type: None
>>>> +// CHECK-NEXT: Other: 0
>>>> +// CHECK-NEXT: Section: .data
>>>> +// CHECK-NEXT: }
>>>> +
>>>> + .globl sym_d
>>>> +sym_d = sym_a + 1
>>>> +// CHECK: Symbol {
>>>> +// CHECK: Name: sym_d
>>>> +// CHECK-NEXT: Value: 0x2
>>>> +// CHECK-NEXT: Size: 0
>>>> +// CHECK-NEXT: Binding: Global
>>>> +// CHECK-NEXT: Type: None
>>>> +// CHECK-NEXT: Other: 0
>>>> +// CHECK-NEXT: Section: .data
>>>> +// CHECK-NEXT: }
>>>> +
>>>> + .globl sym_e
>>>> +sym_e = sym_a + (sym_b - sym_a) * 3
>>>> +// CHECK: Symbol {
>>>> +// CHECK: Name: sym_e
>>>> +// CHECK-NEXT: Value: 0xD
>>>> +// CHECK-NEXT: Size: 0
>>>> +// CHECK-NEXT: Binding: Global
>>>> +// CHECK-NEXT: Type: None
>>>> +// CHECK-NEXT: Other: 0
>>>> +// CHECK-NEXT: Section: .data
>>>> +// CHECK-NEXT: }
>>>>
>>>>
--
Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221
77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140318/6366db68/attachment.html>
More information about the llvm-commits
mailing list