[llvm-commits] [PATCH] review for MC ELF header e_flags setting
Jim Grosbach
grosbach at apple.com
Thu Jan 24 17:29:22 PST 2013
On Jan 24, 2013, at 5:27 PM, "Carter, Jack" <jcarter at mips.com> wrote:
> Jim,
>
> I agree with all the changes, but are you comfortable with having ARM set its one flag from a call from ARMsAsmPrinter::EmitEndOfAsmFile() for the ARM code generator?
>
> That is why I left in the <target>ELFObjectWriter::getEFlags().
>
> If you are fine with it, I will remove getEFlags().
>
Seems reasonable to me. It's more consistent.
-Jim
> Jack
>
>
>
> From: Jim Grosbach [grosbach at apple.com]
> Sent: Thursday, January 24, 2013 3:29 PM
> To: Carter, Jack
> Cc: llvm-commits at cs.uiuc.edu; Eli Bendersky; rafael.espindola at gmail.com; bruno.cardoso at gmail.com
> Subject: Re: [PATCH] review for MC ELF header e_flags setting
>
> 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/4d69fc14/attachment.html>
More information about the llvm-commits
mailing list