[llvm-commits] [PATCH] review for MC ELF header e_flags setting
Jim Grosbach
grosbach at apple.com
Thu Jan 24 15:29:43 PST 2013
Hi Jack,
This looks like a much better approach. Theoretically, I'd really prefer to have things like this be attributes on the Targets' AsmBackend implementations, but those interfaces aren't factored in a way to make that practical at the moment. Thus the need to do semi-hacky things to the MCAssembler data.
A few general things first. Please split this into two patches that a) implement the target-independent bits and b) use those bits in the MIPS backend to do what you're really after. Second, this really needs tests. Third, it feels a bit weird to have both this and the TargetObjectWriter getEFlags() interface. Perhaps the latter can/should be removed and migrated to this new bit?
A few comments inline.
Regards,
Jim
>
> ---
> include/llvm/MC/MCAssembler.h | 6 ++++
> lib/MC/ELFObjectWriter.cpp | 12 +++++--
> lib/MC/MCAssembler.cpp | 5 ++-
> .../Mips/MCTargetDesc/MipsELFObjectWriter.cpp | 12 +------
> lib/Target/Mips/MipsAsmPrinter.cpp | 34 ++++++++++++++++++++
> lib/Target/Mips/MipsSubtarget.cpp | 4 +-
> lib/Target/Mips/MipsSubtarget.h | 6 ++++
> 7 files changed, 61 insertions(+), 18 deletions(-)
>
> diff --git a/include/llvm/MC/MCAssembler.h b/include/llvm/MC/MCAssembler.h
> index 2edd956..f194bcc 100644
> --- a/include/llvm/MC/MCAssembler.h
> +++ b/include/llvm/MC/MCAssembler.h
> @@ -871,6 +871,8 @@ private:
> unsigned NoExecStack : 1;
> unsigned SubsectionsViaSymbols : 1;
>
> + /// ELF specific e_header flags
> + unsigned AsmEFlags;
A bit more commentary on what these are and why they're stored here would be helpful. The variable name seems a bit odd. These aren't just for assembly code, so why the "Asm" name? It would seem something like ELFHeaderEFlags might be more descriptive.
> private:
> /// Evaluate a fixup to a relocatable expression and the value which should be
> /// placed into the fixup.
> @@ -948,6 +950,10 @@ public:
> /// Flag a function symbol as the target of a .thumb_func directive.
> void setIsThumbFunc(const MCSymbol *Func) { ThumbFuncs.insert(Func); }
>
> + /// ELF e_header flags
> + unsigned getAsmEFlags() const {return AsmEFlags;}
> + void setAsmEFlags(unsigned Flags) { AsmEFlags = Flags;}
> +
> public:
> /// Construct a new assembler instance.
> ///
> diff --git a/lib/MC/ELFObjectWriter.cpp b/lib/MC/ELFObjectWriter.cpp
> index 3b12d3a..3e96ad3 100644
> --- a/lib/MC/ELFObjectWriter.cpp
> +++ b/lib/MC/ELFObjectWriter.cpp
> @@ -233,7 +233,8 @@ class ELFObjectWriter : public MCObjectWriter {
> F.getContents().append(&buf[0], &buf[8]);
> }
>
> - void WriteHeader(uint64_t SectionDataSize,
> + void WriteHeader(const MCAssembler &Asm,
> + uint64_t SectionDataSize,
> unsigned NumberOfSections);
>
> void WriteSymbolEntry(MCDataFragment *SymtabF,
> @@ -373,7 +374,8 @@ ELFObjectWriter::~ELFObjectWriter()
> {}
>
> // Emit the ELF header.
> -void ELFObjectWriter::WriteHeader(uint64_t SectionDataSize,
> +void ELFObjectWriter::WriteHeader(const MCAssembler &Asm,
> + uint64_t SectionDataSize,
> unsigned NumberOfSections) {
> // ELF Header
> // ----------
> @@ -411,7 +413,9 @@ void ELFObjectWriter::WriteHeader(uint64_t SectionDataSize,
> sizeof(ELF::Elf32_Ehdr))); // e_shoff = sec hdr table off in bytes
>
> // e_flags = whatever the target wants
> - Write32(getEFlags());
> + unsigned Flags = getEFlags();
> + Flags |= Asm.getAsmEFlags();
> + Write32(Flags);
>
This is part of what feels really awkward about having both the object writer EFLags storage and the MCAssembler storage. We should have one place for the data.
> // e_ehsize = ELF header size
> Write16(is64Bit() ? sizeof(ELF::Elf64_Ehdr) : sizeof(ELF::Elf32_Ehdr));
> @@ -1534,7 +1538,7 @@ void ELFObjectWriter::WriteObject(MCAssembler &Asm,
> }
>
> // Write out the ELF header ...
> - WriteHeader(SectionHeaderOffset, NumSections + 1);
> + WriteHeader(Asm, SectionHeaderOffset, NumSections + 1);
>
> // ... then the regular sections ...
> // + because of .shstrtab
> diff --git a/lib/MC/MCAssembler.cpp b/lib/MC/MCAssembler.cpp
> index c51ddc8..1fdb25c 100644
> --- a/lib/MC/MCAssembler.cpp
> +++ b/lib/MC/MCAssembler.cpp
> @@ -263,8 +263,8 @@ MCAssembler::MCAssembler(MCContext &Context_, MCAsmBackend &Backend_,
> MCCodeEmitter &Emitter_, MCObjectWriter &Writer_,
> raw_ostream &OS_)
> : Context(Context_), Backend(Backend_), Emitter(Emitter_), Writer(Writer_),
> - OS(OS_), BundleAlignSize(0), RelaxAll(false), NoExecStack(false),
> - SubsectionsViaSymbols(false) {
> + OS(OS_), BundleAlignSize(0), RelaxAll(false),
> + NoExecStack(false), SubsectionsViaSymbols(false), AsmEFlags(0) {
> }
>
> MCAssembler::~MCAssembler() {
> @@ -281,6 +281,7 @@ void MCAssembler::reset() {
> RelaxAll = false;
> NoExecStack = false;
> SubsectionsViaSymbols = false;
> + AsmEFlags = 0;
>
> // reset objects owned by us
> getBackend().reset();
> diff --git a/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp b/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp
> index 7afb77e..2c54480 100644
> --- a/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp
> +++ b/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp
> @@ -61,17 +61,9 @@ MipsELFObjectWriter::MipsELFObjectWriter(bool _is64Bit, uint8_t OSABI,
>
> MipsELFObjectWriter::~MipsELFObjectWriter() {}
>
> -// FIXME: get the real EABI Version from the Subtarget class.
> +// ELF Header flags we will always produce
> unsigned MipsELFObjectWriter::getEFlags() const {
> -
> - // FIXME: We can't tell if we are PIC (dynamic) or CPIC (static)
> - unsigned Flag = ELF::EF_MIPS_NOREORDER;
> -
> - if (is64Bit())
> - Flag |= ELF::EF_MIPS_ARCH_64R2;
> - else
> - Flag |= ELF::EF_MIPS_ARCH_32R2;
> - return Flag;
> + return ELF::EF_MIPS_NOREORDER;
> }
>
> const MCSymbol *MipsELFObjectWriter::ExplicitRelSym(const MCAssembler &Asm,
> diff --git a/lib/Target/Mips/MipsAsmPrinter.cpp b/lib/Target/Mips/MipsAsmPrinter.cpp
> index e3c3429..d334eac 100644
> --- a/lib/Target/Mips/MipsAsmPrinter.cpp
> +++ b/lib/Target/Mips/MipsAsmPrinter.cpp
> @@ -32,9 +32,12 @@
> #include "llvm/IR/InlineAsm.h"
> #include "llvm/IR/Instructions.h"
> #include "llvm/MC/MCAsmInfo.h"
> +#include "llvm/MC/MCAssembler.h"
> #include "llvm/MC/MCInst.h"
> +#include "llvm/MC/MCObjectStreamer.h"
> #include "llvm/MC/MCStreamer.h"
> #include "llvm/MC/MCSymbol.h"
> +#include "llvm/Support/ELF.h"
> #include "llvm/Support/TargetRegistry.h"
> #include "llvm/Support/raw_ostream.h"
> #include "llvm/Target/Mangler.h"
> @@ -545,9 +548,40 @@ void MipsAsmPrinter::EmitStartOfAsmFile(Module &M) {
>
> void MipsAsmPrinter::EmitEndOfAsmFile(Module &M) {
>
> + if (OutStreamer.hasRawTextSupport()) return;
> +
> // Emit Mips ELF register info
> Subtarget->getMReginfo().emitMipsReginfoSectionCG(
> OutStreamer, getObjFileLowering(), *Subtarget);
> +
> + // Update e_header flags
> + // This should and will be a single call to a routine living somewhere else,
> + // but for discussion purposes I will just make the individual settings here.
I suggest moving it out and cleaning it up a bit as part of splitting the patch apart. Just a suggestion, though. I don't care about ordering too much so long as the cleanup happens, though, so whatever works for you.
> + MCObjectStreamer& MCOS = static_cast< MCObjectStreamer & >(OutStreamer);
> + MCAssembler& MCA = MCOS.getAssembler();
> + unsigned EFlags = MCA.getAsmEFlags();
> +
> + // Architecture
> + if (Subtarget->hasMips64r2())
> + EFlags |= ELF::EF_MIPS_ARCH_64R2;
> + else if (Subtarget->hasMips64())
> + EFlags |= ELF::EF_MIPS_ARCH_64;
> + else if (Subtarget->hasMips32r2())
> + EFlags |= ELF::EF_MIPS_ARCH_32R2;
> + else
> + EFlags |= ELF::EF_MIPS_ARCH_32;
> +
> + // Relocation Model
> + Reloc::Model RM = Subtarget->getRelocationModel();
> + if (RM == Reloc::PIC_ || RM == Reloc::Default)
> + EFlags |= ELF::EF_MIPS_PIC;
> + else if (RM == Reloc::Static)
> + ; // Do nothing for Reloc::Static
> + else
> + llvm_unreachable("Unsupported relocation model for e_flags");
> +
> + MCA.setAsmEFlags(EFlags);
> +
> }
>
> MachineLocation
> diff --git a/lib/Target/Mips/MipsSubtarget.cpp b/lib/Target/Mips/MipsSubtarget.cpp
> index 30d377a..6ad97db 100644
> --- a/lib/Target/Mips/MipsSubtarget.cpp
> +++ b/lib/Target/Mips/MipsSubtarget.cpp
> @@ -26,13 +26,13 @@ void MipsSubtarget::anchor() { }
>
> MipsSubtarget::MipsSubtarget(const std::string &TT, const std::string &CPU,
> const std::string &FS, bool little,
> - Reloc::Model RM) :
> + Reloc::Model _RM) :
> MipsGenSubtargetInfo(TT, CPU, FS),
> MipsArchVersion(Mips32), MipsABI(UnknownABI), IsLittle(little),
> IsSingleFloat(false), IsFP64bit(false), IsGP64bit(false), HasVFPU(false),
> IsLinux(true), HasSEInReg(false), HasCondMov(false), HasSwap(false),
> HasBitCount(false), HasFPIdx(false),
> - InMips16Mode(false), HasDSP(false), HasDSPR2(false), IsAndroid(false)
> + InMips16Mode(false), HasDSP(false), HasDSPR2(false), IsAndroid(false), RM(_RM)
> {
> std::string CPUName = CPU;
> if (CPUName.empty())
> diff --git a/lib/Target/Mips/MipsSubtarget.h b/lib/Target/Mips/MipsSubtarget.h
> index 001d8d1..5538615 100644
> --- a/lib/Target/Mips/MipsSubtarget.h
> +++ b/lib/Target/Mips/MipsSubtarget.h
> @@ -100,6 +100,9 @@ protected:
> // The instance to the register info section object
> MipsReginfo MRI;
>
> + // Relocation Model
> + Reloc::Model RM;
> +
> public:
> virtual bool enablePostRAScheduler(CodeGenOpt::Level OptLevel,
> AntiDepBreakMode& Mode,
> @@ -152,6 +155,9 @@ public:
>
> // Grab MipsRegInfo object
> const MipsReginfo &getMReginfo() const { return MRI; }
> +
> + // Grab relocation model
> + Reloc::Model getRelocationModel () const {return RM;}
> };
> } // End llvm namespace
>
On Jan 24, 2013, at 1:41 PM, "Carter, Jack" <jcarter at mips.com> wrote:
> This is not a rehash of an old patch.
>
> Currently gathering information such as symbol, section and data is done by collecting it in an MCAssembler object. From MCAssembler and MCAsmLayout objects ELFObjectWriter::WriteObject() forms and streams out the ELF object file.
>
> This patch just adds a few members to the MCAssember class to store and access the e_flag settings. It allows for runtime additions to the e_flag by assembler directives. The standalone assembler can get to MCAssembler from getParser().getStreamer().getAssembler().
>
> The downside is that I am putting an ELF specific data member in MCAssembler (right next to an existing MACH specific member). We probably should have an MCELFAssembler object down the road.
>
> I have looked at the ARMELFStreamer files and I do not see how they apply to the e_flag bit field data storage.
>
> Please review and comment.
>
> Regards,
>
> Jack
> <eheader4.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130124/a35aa84e/attachment.html>
More information about the llvm-commits
mailing list