From 1c5f439f417f7d60e399886e769fa59283a0913b Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Tue, 29 Apr 2014 12:46:50 +0000 Subject: [PATCH 1/1] Centralize the handling of the thumb bit. This patch centralizes the handling of the thumb bit around MCStreamer::isThumbFunc and makes isThumbFunc handle aliases. This fixes a corner case, but the main advantage is having just one way to check if a MCSymbol is thumb or not. This should still be refactored to be ARM only, but at least now it is just one predicate that has to be refactored instead of 3 (isThumbFunc, ELF_Other_ThumbFunc, and SF_ThumbFunc). git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@207522 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/MC/MCAssembler.h | 6 ++--- include/llvm/MC/MCELFSymbolFlags.h | 8 ++---- lib/MC/ELFObjectWriter.cpp | 17 +++++++------ lib/MC/MCAssembler.cpp | 25 +++++++++++++++++++ lib/MC/MCMachOStreamer.cpp | 4 --- lib/MC/MachObjectWriter.cpp | 3 +++ .../ARM/MCTargetDesc/ARMELFStreamer.cpp | 6 +---- .../ARM/MCTargetDesc/ARMMachObjectWriter.cpp | 4 +-- test/MC/ARM/thumb_set.s | 15 +++++++++++ 9 files changed, 59 insertions(+), 29 deletions(-) diff --git a/include/llvm/MC/MCAssembler.h b/include/llvm/MC/MCAssembler.h index e71775a8c1b..be13b3681ee 100644 --- a/include/llvm/MC/MCAssembler.h +++ b/include/llvm/MC/MCAssembler.h @@ -902,7 +902,7 @@ private: // here. Maybe when the relocation stuff moves to target specific, // this can go with it? The streamer would need some target specific // refactoring too. - SmallPtrSet ThumbFuncs; + mutable SmallPtrSet ThumbFuncs; /// \brief The bundle alignment size currently set in the assembler. /// @@ -995,9 +995,7 @@ public: const MCAsmLayout &Layout) const; /// Check whether a given symbol has been flagged with .thumb_func. - bool isThumbFunc(const MCSymbol *Func) const { - return ThumbFuncs.count(Func); - } + bool isThumbFunc(const MCSymbol *Func) const; /// Flag a function symbol as the target of a .thumb_func directive. void setIsThumbFunc(const MCSymbol *Func) { ThumbFuncs.insert(Func); } diff --git a/include/llvm/MC/MCELFSymbolFlags.h b/include/llvm/MC/MCELFSymbolFlags.h index 5b82a58f400..2f1f5612212 100644 --- a/include/llvm/MC/MCELFSymbolFlags.h +++ b/include/llvm/MC/MCELFSymbolFlags.h @@ -24,9 +24,7 @@ namespace llvm { ELF_STT_Shift = 0, // Shift value for STT_* flags. ELF_STB_Shift = 4, // Shift value for STB_* flags. ELF_STV_Shift = 8, // Shift value for STV_* flags. - ELF_STO_Shift = 10, // Shift value for STO_* flags. - ELF_Other_Shift = 16 // Shift value for llvm local flags, - // not part of the final object file + ELF_STO_Shift = 10 // Shift value for STO_* flags. }; enum ELFSymbolFlags { @@ -49,9 +47,7 @@ namespace llvm { ELF_STV_Default = (ELF::STV_DEFAULT << ELF_STV_Shift), ELF_STV_Internal = (ELF::STV_INTERNAL << ELF_STV_Shift), ELF_STV_Hidden = (ELF::STV_HIDDEN << ELF_STV_Shift), - ELF_STV_Protected = (ELF::STV_PROTECTED << ELF_STV_Shift), - - ELF_Other_ThumbFunc = (1 << ELF_Other_Shift) + ELF_STV_Protected = (ELF::STV_PROTECTED << ELF_STV_Shift) }; } // end namespace llvm diff --git a/lib/MC/ELFObjectWriter.cpp b/lib/MC/ELFObjectWriter.cpp index 1b7ac642589..2e07e22dc33 100644 --- a/lib/MC/ELFObjectWriter.cpp +++ b/lib/MC/ELFObjectWriter.cpp @@ -215,7 +215,8 @@ class ELFObjectWriter : public MCObjectWriter { const MCAsmLayout &Layout, SectionIndexMapTy &SectionIndexMap); - bool shouldRelocateWithSymbol(const MCSymbolRefExpr *RefA, + bool shouldRelocateWithSymbol(const MCAssembler &Asm, + const MCSymbolRefExpr *RefA, const MCSymbolData *SD, uint64_t C, unsigned Type) const; @@ -486,6 +487,7 @@ void ELFObjectWriter::WriteHeader(const MCAssembler &Asm, uint64_t ELFObjectWriter::SymbolValue(MCSymbolData &OrigData, const MCAsmLayout &Layout) { + const MCSymbol &OrigSymbol = OrigData.getSymbol(); MCSymbolData *Data = &OrigData; if (Data->isCommon() && Data->isExternal()) return Data->getCommonAlignment(); @@ -512,8 +514,8 @@ uint64_t ELFObjectWriter::SymbolValue(MCSymbolData &OrigData, } } - if ((Data && Data->getFlags() & ELF_Other_ThumbFunc) || - OrigData.getFlags() & ELF_Other_ThumbFunc) + const MCAssembler &Asm = Layout.getAssembler(); + if (Asm.isThumbFunc(&OrigSymbol)) Res |= 1; if (!Symbol || !Symbol->isInSection()) @@ -641,8 +643,6 @@ void ELFObjectWriter::WriteSymbol(SymbolTableWriter &Writer, ELFSymbolData &MSD, BaseSD = &Layout.getAssembler().getSymbolData(*Base); Type = mergeTypeForSet(Type, MCELF::GetType(*BaseSD)); } - if (OrigData.getFlags() & ELF_Other_ThumbFunc) - Type = ELF::STT_FUNC; uint8_t Info = (Binding << ELF_STB_Shift) | (Type << ELF_STT_Shift); // Other and Visibility share the same byte with Visibility using the lower @@ -737,7 +737,8 @@ void ELFObjectWriter::WriteSymbolTable(MCDataFragment *SymtabF, // It is always valid to create a relocation with a symbol. It is preferable // to use a relocation with a section if that is possible. Using the section // allows us to omit some local symbols from the symbol table. -bool ELFObjectWriter::shouldRelocateWithSymbol(const MCSymbolRefExpr *RefA, +bool ELFObjectWriter::shouldRelocateWithSymbol(const MCAssembler &Asm, + const MCSymbolRefExpr *RefA, const MCSymbolData *SD, uint64_t C, unsigned Type) const { @@ -825,7 +826,7 @@ bool ELFObjectWriter::shouldRelocateWithSymbol(const MCSymbolRefExpr *RefA, // bit. With a symbol that is done by just having the symbol have that bit // set, so we would lose the bit if we relocated with the section. // FIXME: We could use the section but add the bit to the relocation value. - if (SD->getFlags() & ELF_Other_ThumbFunc) + if (Asm.isThumbFunc(&Sym)) return true; if (TargetObjectWriter->needsRelocateWithSymbol(Type)) @@ -887,7 +888,7 @@ void ELFObjectWriter::RecordRelocation(const MCAssembler &Asm, const MCSymbolData *SymAD = SymA ? &Asm.getSymbolData(*SymA) : nullptr; unsigned Type = GetRelocType(Target, Fixup, IsPCRel); - bool RelocateWithSymbol = shouldRelocateWithSymbol(RefA, SymAD, C, Type); + bool RelocateWithSymbol = shouldRelocateWithSymbol(Asm, RefA, SymAD, C, Type); if (!RelocateWithSymbol && SymA && !SymA->isUndefined()) C += Layout.getSymbolOffset(SymAD); diff --git a/lib/MC/MCAssembler.cpp b/lib/MC/MCAssembler.cpp index b3b2bb3d31b..08d73214382 100644 --- a/lib/MC/MCAssembler.cpp +++ b/lib/MC/MCAssembler.cpp @@ -323,6 +323,31 @@ void MCAssembler::reset() { getLOHContainer().reset(); } +bool MCAssembler::isThumbFunc(const MCSymbol *Symbol) const { + if (ThumbFuncs.count(Symbol)) + return true; + + if (!Symbol->isVariable()) + return false; + + // FIXME: It looks like gas support some cases of the form "foo + 2". It + // is not clear if that is a bug or a feature. + const MCExpr *Expr = Symbol->getVariableValue(); + const MCSymbolRefExpr *Ref = dyn_cast(Expr); + if (!Ref) + return false; + + if (Ref->getKind() != MCSymbolRefExpr::VK_None) + return false; + + const MCSymbol &Sym = Ref->getSymbol(); + if (!isThumbFunc(&Sym)) + return false; + + ThumbFuncs.insert(Symbol); // Cache it. + return true; +} + bool MCAssembler::isSymbolLinkerVisible(const MCSymbol &Symbol) const { // Non-temporary labels should always be visible to the linker. if (!Symbol.isTemporary()) diff --git a/lib/MC/MCMachOStreamer.cpp b/lib/MC/MCMachOStreamer.cpp index 23d28b62ca5..c103e41e3a8 100644 --- a/lib/MC/MCMachOStreamer.cpp +++ b/lib/MC/MCMachOStreamer.cpp @@ -237,10 +237,6 @@ void MCMachOStreamer::EmitThumbFunc(MCSymbol *Symbol) { // Remember that the function is a thumb function. Fixup and relocation // values will need adjusted. getAssembler().setIsThumbFunc(Symbol); - - // Mark the thumb bit on the symbol. - MCSymbolData &SD = getAssembler().getOrCreateSymbolData(*Symbol); - SD.setFlags(SD.getFlags() | SF_ThumbFunc); } bool MCMachOStreamer::EmitSymbolAttribute(MCSymbol *Symbol, diff --git a/lib/MC/MachObjectWriter.cpp b/lib/MC/MachObjectWriter.cpp index b4e53be7d52..cbaf0b87d6e 100644 --- a/lib/MC/MachObjectWriter.cpp +++ b/lib/MC/MachObjectWriter.cpp @@ -351,6 +351,9 @@ void MachObjectWriter::WriteNlist(MachSymbolData &MSD, } } + if (Layout.getAssembler().isThumbFunc(&Symbol)) + Flags |= SF_ThumbFunc; + // struct nlist (12 bytes) Write32(MSD.StringIndex); diff --git a/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp b/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp index 6d5fd6a2931..e55aa667134 100644 --- a/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp +++ b/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp @@ -609,12 +609,8 @@ private: } void EmitThumbFunc(MCSymbol *Func) override { - // FIXME: Anything needed here to flag the function as thumb? - getAssembler().setIsThumbFunc(Func); - - MCSymbolData &SD = getAssembler().getOrCreateSymbolData(*Func); - SD.setFlags(SD.getFlags() | ELF_Other_ThumbFunc); + EmitSymbolAttribute(Func, MCSA_ELF_TypeFunction); } // Helper functions for ARM exception handling directives diff --git a/lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp b/lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp index 38ed5632e46..ecfa4e54b26 100644 --- a/lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp +++ b/lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp @@ -206,11 +206,11 @@ RecordARMScatteredHalfRelocation(MachObjectWriter *Writer, // The thumb bit shouldn't be set in the 'other-half' bit of the // relocation, but it will be set in FixedValue if the base symbol // is a thumb function. Clear it out here. - if (A_SD->getFlags() & SF_ThumbFunc) + if (Asm.isThumbFunc(A)) FixedValue &= 0xfffffffe; break; case ARM::fixup_t2_movt_hi16: - if (A_SD->getFlags() & SF_ThumbFunc) + if (Asm.isThumbFunc(A)) FixedValue &= 0xfffffffe; MovtBit = 1; // Fallthrough diff --git a/test/MC/ARM/thumb_set.s b/test/MC/ARM/thumb_set.s index 650e1e5f40e..ac4bd328895 100644 --- a/test/MC/ARM/thumb_set.s +++ b/test/MC/ARM/thumb_set.s @@ -14,6 +14,9 @@ arm_func: .thumb_set alias_arm_func, arm_func + alias_arm_func2 = alias_arm_func + alias_arm_func3 = alias_arm_func2 + @ ASM: .thumb_set alias_arm_func, arm_func .thumb @@ -64,6 +67,18 @@ beta: @ CHECK: Type: Function @ CHECK: } +@ CHECK: Symbol { +@ CHECK: Name: alias_arm_func2 +@ CHECK: Value: 0x1 +@ CHECK: Type: Function +@ CHECK: } + +@ CHECK: Symbol { +@ CHECK: Name: alias_arm_func3 +@ CHECK: Value: 0x1 +@ CHECK: Type: Function +@ CHECK: } + @ CHECK: Symbol { @ CHECK: Name: alias_defined_data @ CHECK: Value: 0x5 -- 2.34.1