[ping] RE: [PATCH] review for MC ELF header e_flags setting
Carter, Jack
jcarter at mips.com
Tue Jan 29 13:43:40 PST 2013
Am I to assume that the patches were redone as requested and that I can commit?
Cheers,
Jack
________________________________
From: Carter, Jack
Sent: Monday, January 28, 2013 3:59 PM
To: Jim Grosbach
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
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<mailto: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<mailto:grosbach at apple.com>]
Sent: Thursday, January 24, 2013 3:29 PM
To: Carter, Jack
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>; Eli Bendersky; rafael.espindola at gmail.com<mailto:rafael.espindola at gmail.com>; bruno.cardoso at gmail.com<mailto: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<mailto: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/20130129/2cd01a67/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: general_eflags.patch
Type: text/x-patch
Size: 9182 bytes
Desc: general_eflags.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130129/2cd01a67/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm_eflags.patch
Type: text/x-patch
Size: 4830 bytes
Desc: arm_eflags.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130129/2cd01a67/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mips_eflags.patch
Type: text/x-patch
Size: 10677 bytes
Desc: mips_eflags.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130129/2cd01a67/attachment-0002.bin>
More information about the llvm-commits
mailing list