[patch] Fix pr24486
Rafael Espíndola via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 24 07:12:49 PDT 2015
Oh, and an updated (all targets) patch is attached.
On 24 August 2015 at 10:11, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> I just realized that this is not enough. The issues is that when using
> the asm streamer we still have to make sure that in
>
> a:
> b = a
>
> b is considered defined.
>
> It might be possible to do this in two ways:
>
> * Have the asm streamer use dummy fragments.
> * Instead of saving the section or fragment of a symbol, save the base
> symbol if any (for b it is a). Without the first change this would
> require an extra bit in MCSymbol and I am not sure that we have it.
>
> I will not have the time to work on it any time soon. Maxin, can you
> give it a try?
>
> Cheers,
> Rafael
>
>
>
> On 20 August 2015 at 21:33, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> The attached patch fixes pr24486.
>>
>> It extends the work done in r233995 so that now getFragment (in
>> addition to getSection) also works for variable symbols.
>>
>> With that the existing logic to decide if a-b can be computed works
>> and the expression evaluation can avoid expanding variables as
>> aggressively. That in turn lets the relocation code see the original
>> variable.
>>
>> I think the testcase change to COFF is OK. Both relocations mean
>> exactly the same. In fact, gas produces relocations with the .text
>> symbol like we do for ELF. We should probably just implement that for
>> COFF some day.
>>
>> Cheers,
>> Rafael
-------------- next part --------------
diff --git a/include/llvm/MC/MCExpr.h b/include/llvm/MC/MCExpr.h
index b3a6073..76da86c 100644
--- a/include/llvm/MC/MCExpr.h
+++ b/include/llvm/MC/MCExpr.h
@@ -20,6 +20,7 @@ class MCAsmLayout;
class MCAssembler;
class MCContext;
class MCFixup;
+class MCFragment;
class MCSection;
class MCStreamer;
class MCSymbol;
@@ -115,7 +116,7 @@ public:
/// currently defined as the absolute section for constants, or
/// otherwise the section associated with the first defined symbol in the
/// expression.
- MCSection *findAssociatedSection() const;
+ MCFragment *findAssociatedFragment() const;
/// @}
};
@@ -556,7 +557,7 @@ public:
const MCAsmLayout *Layout,
const MCFixup *Fixup) const = 0;
virtual void visitUsedExpr(MCStreamer& Streamer) const = 0;
- virtual MCSection *findAssociatedSection() const = 0;
+ virtual MCFragment *findAssociatedFragment() const = 0;
virtual void fixELFSymbolsInTLSFixups(MCAssembler &) const = 0;
diff --git a/include/llvm/MC/MCMachObjectWriter.h b/include/llvm/MC/MCMachObjectWriter.h
index 175d73e..c89b859 100644
--- a/include/llvm/MC/MCMachObjectWriter.h
+++ b/include/llvm/MC/MCMachObjectWriter.h
@@ -246,6 +246,11 @@ public:
const MCAsmLayout &Layout) override;
bool isSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
+ const MCSymbol &A,
+ const MCSymbol &B,
+ bool InSet) const override;
+
+ bool isSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
const MCSymbol &SymA,
const MCFragment &FB, bool InSet,
bool IsPCRel) const override;
diff --git a/include/llvm/MC/MCObjectWriter.h b/include/llvm/MC/MCObjectWriter.h
index 2211673..7ab912c 100644
--- a/include/llvm/MC/MCObjectWriter.h
+++ b/include/llvm/MC/MCObjectWriter.h
@@ -92,6 +92,11 @@ public:
bool InSet) const;
virtual bool isSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
+ const MCSymbol &A,
+ const MCSymbol &B,
+ bool InSet) const;
+
+ virtual bool isSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
const MCSymbol &SymA,
const MCFragment &FB,
bool InSet,
diff --git a/include/llvm/MC/MCSymbol.h b/include/llvm/MC/MCSymbol.h
index b2910df..e73fc77 100644
--- a/include/llvm/MC/MCSymbol.h
+++ b/include/llvm/MC/MCSymbol.h
@@ -56,19 +56,17 @@ protected:
SymContentsCommon,
};
- // Special sentinal value for the absolute pseudo section.
- //
- // FIXME: Use a PointerInt wrapper for this?
- static MCSection *AbsolutePseudoSection;
+ // Special sentinal value for the absolute pseudo fragment.
+ static MCFragment *AbsolutePseudoFragment;
/// If a symbol has a Fragment, the section is implied, so we only need
/// one pointer.
+ /// The special AbsolutePseudoFragment value is for absolute symbols.
+ /// If this is a variable symbol, this caches the variable value's fragment.
/// FIXME: We might be able to simplify this by having the asm streamer create
/// dummy fragments.
/// If this is a section, then it gives the symbol is defined in. This is null
- /// for undefined symbols, and the special AbsolutePseudoSection value for
- /// absolute symbols. If this is a variable symbol, this caches the variable
- /// value's section.
+ /// for undefined symbols.
///
/// If this is a fragment, then it gives the fragment this symbol's value is
/// relative to, if any.
@@ -180,14 +178,12 @@ private:
MCSymbol(const MCSymbol &) = delete;
void operator=(const MCSymbol &) = delete;
MCSection *getSectionPtr() const {
- if (MCFragment *F = getFragment())
+ if (MCFragment *F = getFragment()) {
+ assert(F != AbsolutePseudoFragment);
return F->getParent();
+ }
const auto &SectionOrFragment = SectionOrFragmentAndHasName.getPointer();
- assert(!SectionOrFragment.is<MCFragment *>() && "Section or null expected");
- MCSection *Section = SectionOrFragment.dyn_cast<MCSection *>();
- if (Section || !isVariable())
- return Section;
- return Section = getVariableValue()->findAssociatedSection();
+ return SectionOrFragment.dyn_cast<MCSection *>();
}
/// \brief Get a reference to the name field. Requires that we have a name
@@ -248,7 +244,7 @@ public:
/// isDefined - Check if this symbol is defined (i.e., it has an address).
///
/// Defined symbols are either absolute or in some section.
- bool isDefined() const { return getSectionPtr() != nullptr; }
+ bool isDefined() const { return isAbsolute() || getSectionPtr() != nullptr; }
/// isInSection - Check if this symbol is defined in some section (i.e., it
/// is defined but not absolute).
@@ -258,7 +254,7 @@ public:
bool isUndefined() const { return !isDefined(); }
/// isAbsolute - Check if this is an absolute symbol.
- bool isAbsolute() const { return getSectionPtr() == AbsolutePseudoSection; }
+ bool isAbsolute() const { return getFragment() == AbsolutePseudoFragment; }
/// Get the section associated with a defined, non-absolute symbol.
MCSection &getSection() const {
@@ -380,7 +376,13 @@ public:
}
MCFragment *getFragment() const {
- return SectionOrFragmentAndHasName.getPointer().dyn_cast<MCFragment *>();
+ const auto &SectionOrFragment = SectionOrFragmentAndHasName.getPointer();
+ MCFragment *Fragment = SectionOrFragment.dyn_cast<MCFragment *>();
+ if (Fragment || !isVariable())
+ return Fragment;
+ Fragment = getVariableValue()->findAssociatedFragment();
+ SectionOrFragmentAndHasName.setPointer(Fragment);
+ return Fragment;
}
void setFragment(MCFragment *Value) const {
SectionOrFragmentAndHasName.setPointer(Value);
diff --git a/lib/MC/ELFObjectWriter.cpp b/lib/MC/ELFObjectWriter.cpp
index e925bc2..3b8321e 100644
--- a/lib/MC/ELFObjectWriter.cpp
+++ b/lib/MC/ELFObjectWriter.cpp
@@ -447,7 +447,7 @@ void ELFObjectWriter::writeSymbol(SymbolTableWriter &Writer,
uint32_t StringIndex, ELFSymbolData &MSD,
const MCAsmLayout &Layout) {
const auto &Symbol = cast<MCSymbolELF>(*MSD.Symbol);
- assert((!Symbol.getFragment() ||
+ assert((!Symbol.isInSection() ||
(Symbol.getFragment()->getParent() == &Symbol.getSection())) &&
"The symbol's section doesn't match the fragment's symbol");
const MCSymbolELF *Base =
diff --git a/lib/MC/MCAssembler.cpp b/lib/MC/MCAssembler.cpp
index 09a27fe..2e712f4 100644
--- a/lib/MC/MCAssembler.cpp
+++ b/lib/MC/MCAssembler.cpp
@@ -404,7 +404,7 @@ const MCSymbol *MCAssembler::getAtom(const MCSymbol &S) const {
return &S;
// Absolute and undefined symbols have no defining atom.
- if (!S.getFragment())
+ if (!S.isInSection())
return nullptr;
// Non-linker visible symbols in sections which can't be atomized have no
diff --git a/lib/MC/MCExpr.cpp b/lib/MC/MCExpr.cpp
index a30ceec..4d76b7d 100644
--- a/lib/MC/MCExpr.cpp
+++ b/lib/MC/MCExpr.cpp
@@ -477,7 +477,8 @@ static void AttemptToFoldSymbolOffsetDifference(
if (!Asm->getWriter().isSymbolRefDifferenceFullyResolved(*Asm, A, B, InSet))
return;
- if (SA.getFragment() == SB.getFragment()) {
+ if (SA.getFragment() == SB.getFragment() && !SA.isVariable() &&
+ !SB.isVariable()) {
Addend += (SA.getOffset() - SB.getOffset());
// Pointers to Thumb symbols need to have their low-bit set to allow
@@ -616,9 +617,7 @@ static bool canExpand(const MCSymbol &Sym, const MCAssembler *Asm, bool InSet) {
if (InSet)
return true;
- if (!Asm)
- return false;
- return !Asm->getWriter().isWeak(Sym);
+ return !Sym.isInSection();
}
bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
@@ -765,45 +764,41 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
llvm_unreachable("Invalid assembly expression kind!");
}
-MCSection *MCExpr::findAssociatedSection() const {
+MCFragment *MCExpr::findAssociatedFragment() const {
switch (getKind()) {
case Target:
// We never look through target specific expressions.
- return cast<MCTargetExpr>(this)->findAssociatedSection();
+ return cast<MCTargetExpr>(this)->findAssociatedFragment();
case Constant:
- return MCSymbol::AbsolutePseudoSection;
+ return MCSymbol::AbsolutePseudoFragment;
case SymbolRef: {
const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(this);
const MCSymbol &Sym = SRE->getSymbol();
-
- if (Sym.isDefined())
- return &Sym.getSection();
-
- return nullptr;
+ return Sym.getFragment();
}
case Unary:
- return cast<MCUnaryExpr>(this)->getSubExpr()->findAssociatedSection();
+ return cast<MCUnaryExpr>(this)->getSubExpr()->findAssociatedFragment();
case Binary: {
const MCBinaryExpr *BE = cast<MCBinaryExpr>(this);
- MCSection *LHS_S = BE->getLHS()->findAssociatedSection();
- MCSection *RHS_S = BE->getRHS()->findAssociatedSection();
+ MCFragment *LHS_F = BE->getLHS()->findAssociatedFragment();
+ MCFragment *RHS_F = BE->getRHS()->findAssociatedFragment();
- // If either section is absolute, return the other.
- if (LHS_S == MCSymbol::AbsolutePseudoSection)
- return RHS_S;
- if (RHS_S == MCSymbol::AbsolutePseudoSection)
- return LHS_S;
+ // If either is absolute, return the other.
+ if (LHS_F == MCSymbol::AbsolutePseudoFragment)
+ return RHS_F;
+ if (RHS_F == MCSymbol::AbsolutePseudoFragment)
+ return LHS_F;
// Not always correct, but probably the best we can do without more context.
if (BE->getOpcode() == MCBinaryExpr::Sub)
- return MCSymbol::AbsolutePseudoSection;
+ return MCSymbol::AbsolutePseudoFragment;
// Otherwise, return the first non-null section.
- return LHS_S ? LHS_S : RHS_S;
+ return LHS_F ? LHS_F : RHS_F;
}
}
diff --git a/lib/MC/MCMachOStreamer.cpp b/lib/MC/MCMachOStreamer.cpp
index 33045d0..6643531 100644
--- a/lib/MC/MCMachOStreamer.cpp
+++ b/lib/MC/MCMachOStreamer.cpp
@@ -462,7 +462,8 @@ void MCMachOStreamer::FinishImpl() {
// defining symbols.
DenseMap<const MCFragment *, const MCSymbol *> DefiningSymbolMap;
for (const MCSymbol &Symbol : getAssembler().symbols()) {
- if (getAssembler().isSymbolLinkerVisible(Symbol) && Symbol.getFragment()) {
+ if (getAssembler().isSymbolLinkerVisible(Symbol) && Symbol.isInSection() &&
+ !Symbol.isVariable()) {
// An atom defining symbol should never be internal to a fragment.
assert(Symbol.getOffset() == 0 &&
"Invalid offset in atom defining symbol!");
diff --git a/lib/MC/MCObjectWriter.cpp b/lib/MC/MCObjectWriter.cpp
index 3479034..e84f74a 100644
--- a/lib/MC/MCObjectWriter.cpp
+++ b/lib/MC/MCObjectWriter.cpp
@@ -33,8 +33,14 @@ bool MCObjectWriter::isSymbolRefDifferenceFullyResolved(
if (!SA.getFragment() || !SB.getFragment())
return false;
- return isSymbolRefDifferenceFullyResolvedImpl(Asm, SA, *SB.getFragment(),
- InSet, false);
+ return isSymbolRefDifferenceFullyResolvedImpl(Asm, SA, SB, InSet);
+}
+
+bool MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
+ const MCAssembler &Asm, const MCSymbol &A, const MCSymbol &B,
+ bool InSet) const {
+ return isSymbolRefDifferenceFullyResolvedImpl(Asm, A, *B.getFragment(), InSet,
+ false);
}
bool MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
diff --git a/lib/MC/MCSymbol.cpp b/lib/MC/MCSymbol.cpp
index 125380a..d9e58ce 100644
--- a/lib/MC/MCSymbol.cpp
+++ b/lib/MC/MCSymbol.cpp
@@ -17,7 +17,8 @@
using namespace llvm;
// Sentinel value for the absolute pseudo section.
-MCSection *MCSymbol::AbsolutePseudoSection = reinterpret_cast<MCSection *>(1);
+MCFragment *MCSymbol::AbsolutePseudoFragment =
+ reinterpret_cast<MCFragment *>(4);
void *MCSymbol::operator new(size_t s, const StringMapEntry<bool> *Name,
MCContext &Ctx) {
diff --git a/lib/MC/MachObjectWriter.cpp b/lib/MC/MachObjectWriter.cpp
index 8ce6127..d09d8b1 100644
--- a/lib/MC/MachObjectWriter.cpp
+++ b/lib/MC/MachObjectWriter.cpp
@@ -628,6 +628,18 @@ void MachObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
}
bool MachObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
+ const MCAssembler &Asm, const MCSymbol &A, const MCSymbol &B,
+ bool InSet) const {
+ // FIXME: We don't handle things like
+ // foo = .
+ // creating atoms.
+ if (A.isVariable() || B.isVariable())
+ return false;
+ return MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(Asm, A, B,
+ InSet);
+}
+
+bool MachObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(
const MCAssembler &Asm, const MCSymbol &SymA, const MCFragment &FB,
bool InSet, bool IsPCRel) const {
if (InSet)
diff --git a/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp b/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp
index 2870341..3a51fa7 100644
--- a/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp
+++ b/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp
@@ -85,7 +85,7 @@ void AArch64MCExpr::visitUsedExpr(MCStreamer &Streamer) const {
Streamer.visitUsedExpr(*getSubExpr());
}
-MCSection *AArch64MCExpr::findAssociatedSection() const {
+MCFragment *AArch64MCExpr::findAssociatedFragment() const {
llvm_unreachable("FIXME: what goes here?");
}
diff --git a/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.h b/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.h
index 1165314..ef6e16c 100644
--- a/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.h
+++ b/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.h
@@ -149,7 +149,7 @@ public:
void visitUsedExpr(MCStreamer &Streamer) const override;
- MCSection *findAssociatedSection() const override;
+ MCFragment *findAssociatedFragment() const override;
bool evaluateAsRelocatableImpl(MCValue &Res,
const MCAsmLayout *Layout,
diff --git a/lib/Target/ARM/MCTargetDesc/ARMMCExpr.h b/lib/Target/ARM/MCTargetDesc/ARMMCExpr.h
index 9146d4d..75dde80 100644
--- a/lib/Target/ARM/MCTargetDesc/ARMMCExpr.h
+++ b/lib/Target/ARM/MCTargetDesc/ARMMCExpr.h
@@ -63,8 +63,8 @@ public:
return false;
}
void visitUsedExpr(MCStreamer &Streamer) const override;
- MCSection *findAssociatedSection() const override {
- return getSubExpr()->findAssociatedSection();
+ MCFragment *findAssociatedFragment() const override {
+ return getSubExpr()->findAssociatedFragment();
}
// There are no TLS ARMMCExprs at the moment.
diff --git a/lib/Target/Mips/MCTargetDesc/MipsMCExpr.h b/lib/Target/Mips/MCTargetDesc/MipsMCExpr.h
index fd2ed17..e889972 100644
--- a/lib/Target/Mips/MCTargetDesc/MipsMCExpr.h
+++ b/lib/Target/Mips/MCTargetDesc/MipsMCExpr.h
@@ -51,8 +51,8 @@ public:
const MCAsmLayout *Layout,
const MCFixup *Fixup) const override;
void visitUsedExpr(MCStreamer &Streamer) const override;
- MCSection *findAssociatedSection() const override {
- return getSubExpr()->findAssociatedSection();
+ MCFragment *findAssociatedFragment() const override {
+ return getSubExpr()->findAssociatedFragment();
}
// There are no TLS MipsMCExprs at the moment.
diff --git a/lib/Target/NVPTX/NVPTXMCExpr.h b/lib/Target/NVPTX/NVPTXMCExpr.h
index 46b4b33..81a606d 100644
--- a/lib/Target/NVPTX/NVPTXMCExpr.h
+++ b/lib/Target/NVPTX/NVPTXMCExpr.h
@@ -68,7 +68,7 @@ public:
return false;
}
void visitUsedExpr(MCStreamer &Streamer) const override {};
- MCSection *findAssociatedSection() const override { return nullptr; }
+ MCFragment *findAssociatedFragment() const override { return nullptr; }
// There are no TLS NVPTXMCExprs at the moment.
void fixELFSymbolsInTLSFixups(MCAssembler &Asm) const override {}
@@ -110,7 +110,7 @@ public:
return false;
}
void visitUsedExpr(MCStreamer &Streamer) const override {};
- MCSection *findAssociatedSection() const override { return nullptr; }
+ MCFragment *findAssociatedFragment() const override { return nullptr; }
// There are no TLS NVPTXMCExprs at the moment.
void fixELFSymbolsInTLSFixups(MCAssembler &Asm) const override {}
diff --git a/lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.h b/lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.h
index a641780..d42a111 100644
--- a/lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.h
+++ b/lib/Target/PowerPC/MCTargetDesc/PPCMCExpr.h
@@ -82,8 +82,8 @@ public:
const MCAsmLayout *Layout,
const MCFixup *Fixup) const override;
void visitUsedExpr(MCStreamer &Streamer) const override;
- MCSection *findAssociatedSection() const override {
- return getSubExpr()->findAssociatedSection();
+ MCFragment *findAssociatedFragment() const override {
+ return getSubExpr()->findAssociatedFragment();
}
// There are no TLS PPCMCExprs at the moment.
diff --git a/lib/Target/Sparc/MCTargetDesc/SparcMCExpr.h b/lib/Target/Sparc/MCTargetDesc/SparcMCExpr.h
index d08ad86..13f0819 100644
--- a/lib/Target/Sparc/MCTargetDesc/SparcMCExpr.h
+++ b/lib/Target/Sparc/MCTargetDesc/SparcMCExpr.h
@@ -90,8 +90,8 @@ public:
const MCAsmLayout *Layout,
const MCFixup *Fixup) const override;
void visitUsedExpr(MCStreamer &Streamer) const override;
- MCSection *findAssociatedSection() const override {
- return getSubExpr()->findAssociatedSection();
+ MCFragment *findAssociatedFragment() const override {
+ return getSubExpr()->findAssociatedFragment();
}
void fixELFSymbolsInTLSFixups(MCAssembler &Asm) const override;
diff --git a/test/MC/COFF/alias.s b/test/MC/COFF/alias.s
index 2293d43..369bbe8 100644
--- a/test/MC/COFF/alias.s
+++ b/test/MC/COFF/alias.s
@@ -21,9 +21,9 @@ weak_aliased_to_external = external2
.long weak_aliased_to_external
// CHECK: Relocations [
-// CHECK: 0x0 IMAGE_REL_I386_DIR32 local1
+// CHECK: 0x0 IMAGE_REL_I386_DIR32 external_aliased_to_local
// CHECK: 0x4 IMAGE_REL_I386_DIR32 external1
-// CHECK: 0x8 IMAGE_REL_I386_DIR32 local2
+// CHECK: 0x8 IMAGE_REL_I386_DIR32 global_aliased_to_local
// CHECK: 0xC IMAGE_REL_I386_DIR32 external2
// CHECK: ]
// CHECK: Symbols [
diff --git a/test/MC/ELF/relocation.s b/test/MC/ELF/relocation.s
index 34f1a40..0fec767 100644
--- a/test/MC/ELF/relocation.s
+++ b/test/MC/ELF/relocation.s
@@ -55,6 +55,11 @@ bar:
.quad pr23272_2 - pr23272
.quad pr23272_3 - pr23272
+ .global pr24486
+pr24486:
+ pr24486_alias = pr24486
+ .long pr24486_alias
+
.code16
call pr23771
@@ -94,6 +99,7 @@ bar:
// CHECK-NEXT: 0xD4 R_X86_64_SIZE32 blah 0xFFFFFFFFFFFFFFE0
// CHECK-NEXT: 0xD8 R_X86_64_GOTPCREL foo 0x0
// CHECK-NEXT: 0xDC R_X86_64_PLT32 foo 0x0
-// CHECK-NEXT: 0xF1 R_X86_64_PC16 pr23771 0xFFFFFFFFFFFFFFFE
+// CHECK-NEXT: 0xF0 R_X86_64_32 .text 0xF0
+// CHECK-NEXT: 0xF5 R_X86_64_PC16 pr23771 0xFFFFFFFFFFFFFFFE
// CHECK-NEXT: ]
// CHECK-NEXT: }
More information about the llvm-commits
mailing list