[PATCH] review for MC ELF header e_flags setting
Jim Grosbach
grosbach at apple.com
Tue Jan 29 17:42:21 PST 2013
LGTM!
-Jim
On Jan 28, 2013, at 3:59 PM, "Carter, Jack" <jcarter at mips.com> wrote:
> Jim,
>
> I split up the patch into 3 pieces: general, ARM and Mips. These were the only targets setting and testing the flags. I had to turn off an ARM test for the general patch and turn it back on with the ARM patch.
>
> Regards,
>
> Jack
>
>
> From: Jim Grosbach [grosbach at apple.com]
> Sent: Thursday, January 24, 2013 5: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
>
>
> 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>
>
> <general_eflags.patch><arm_eflags.patch><mips_eflags.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130129/7e3e692d/attachment.html>
More information about the llvm-commits
mailing list