<html dir="ltr">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style id="owaParaStyle" type="text/css">P {margin-top:0;margin-bottom:0;}</style>
</head>
<body ocsi="0" fpstyle="1" style="word-wrap:break-word">
<div style="direction: ltr;font-family: Tahoma;color: #000000;font-size: 10pt;">Jim,<br>
<br>
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.<br>
<br>
Regards,<br>
<br>
Jack<br>
<br>
<br>
<div style="font-family: Times New Roman; color: #000000; font-size: 16px">
<hr tabindex="-1">
<div style="direction: ltr;" id="divRpF818314"><font face="Tahoma" color="#000000" size="2"><b>From:</b> Jim Grosbach [grosbach@apple.com]<br>
<b>Sent:</b> Thursday, January 24, 2013 5:29 PM<br>
<b>To:</b> Carter, Jack<br>
<b>Cc:</b> llvm-commits@cs.uiuc.edu; Eli Bendersky; rafael.espindola@gmail.com; bruno.cardoso@gmail.com<br>
<b>Subject:</b> Re: [PATCH] review for MC ELF header e_flags setting<br>
</font><br>
</div>
<div></div>
<div><br>
<div>
<div>On Jan 24, 2013, at 5:27 PM, "Carter, Jack" <<a href="mailto:jcarter@mips.com" target="_blank">jcarter@mips.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="font-family:Helvetica; font-size:medium; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:2; text-indent:0px; text-transform:none; white-space:normal; widows:2; word-spacing:0px; word-wrap:break-word">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">Jim,<br>
<br>
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?<br>
<br>
That is why I left in the <target>ELFObjectWriter::getEFlags().<br>
<br>
If you are fine with it, I will remove getEFlags().<br>
<br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Seems reasonable to me. It's more consistent.</div>
<div><br>
</div>
<div>-Jim</div>
<br>
<blockquote type="cite">
<div style="font-family:Helvetica; font-size:medium; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:2; text-indent:0px; text-transform:none; white-space:normal; widows:2; word-spacing:0px; word-wrap:break-word">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">Jack<br>
<br>
<br>
<br>
<div style="font-family:'Times New Roman'; font-size:16px">
<hr tabindex="-1">
<div id="divRpF389605" style="direction:ltr"><font face="Tahoma" size="2"><b>From:</b><span class="Apple-converted-space"> </span>Jim Grosbach [<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>]<br>
<b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, January 24, 2013 3:29 PM<br>
<b>To:</b><span class="Apple-converted-space"> </span>Carter, Jack<br>
<b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>; Eli Bendersky;
<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>;
<a href="mailto:bruno.cardoso@gmail.com" target="_blank">bruno.cardoso@gmail.com</a><br>
<b>Subject:</b><span class="Apple-converted-space"> </span>Re: [PATCH] review for MC ELF header e_flags setting<br>
</font><br>
</div>
<div></div>
<div>Hi Jack,
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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?</div>
<div><br>
</div>
<div>A few comments inline.</div>
<div><br>
</div>
<div>Regards,</div>
<div> Jim</div>
<div><br>
</div>
<div><br>
</div>
<div>
<blockquote type="cite">
<div><br>
</div>
<div>---</div>
<div> include/llvm/MC/MCAssembler.h | 6 ++++</div>
<div> lib/MC/ELFObjectWriter.cpp | 12 +++++--</div>
<div> lib/MC/MCAssembler.cpp | 5 ++-</div>
<div> .../Mips/MCTargetDesc/MipsELFObjectWriter.cpp | 12 +------</div>
<div> lib/Target/Mips/MipsAsmPrinter.cpp | 34 ++++++++++++++++++++</div>
<div> lib/Target/Mips/MipsSubtarget.cpp | 4 +-</div>
<div> lib/Target/Mips/MipsSubtarget.h | 6 ++++</div>
<div> 7 files changed, 61 insertions(+), 18 deletions(-)</div>
<div><br>
</div>
<div>diff --git a/include/llvm/MC/MCAssembler.h b/include/llvm/MC/MCAssembler.h</div>
<div>index 2edd956..f194bcc 100644</div>
<div>--- a/include/llvm/MC/MCAssembler.h</div>
<div>+++ b/include/llvm/MC/MCAssembler.h</div>
<div>@@ -871,6 +871,8 @@ private:</div>
<div> unsigned NoExecStack : 1;</div>
<div> unsigned SubsectionsViaSymbols : 1;</div>
<div> </div>
<div>+ /// ELF specific e_header flags</div>
<div>+ unsigned AsmEFlags;</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
<br>
<blockquote type="cite"></blockquote>
<blockquote type="cite">
<div> private:</div>
<div> /// Evaluate a fixup to a relocatable expression and the value which should be</div>
<div> /// placed into the fixup.</div>
<div>@@ -948,6 +950,10 @@ public:</div>
<div> /// Flag a function symbol as the target of a .thumb_func directive.</div>
<div> void setIsThumbFunc(const MCSymbol *Func) { ThumbFuncs.insert(Func); }</div>
<div> </div>
<div>+ /// ELF e_header flags</div>
</blockquote>
<blockquote type="cite">
<div>+ unsigned getAsmEFlags() const {return AsmEFlags;}</div>
<div>+ void setAsmEFlags(unsigned Flags) { AsmEFlags = Flags;}</div>
<div>+</div>
<div> public:</div>
<div> /// Construct a new assembler instance.</div>
<div> ///</div>
<div>diff --git a/lib/MC/ELFObjectWriter.cpp b/lib/MC/ELFObjectWriter.cpp</div>
<div>index 3b12d3a..3e96ad3 100644</div>
<div>--- a/lib/MC/ELFObjectWriter.cpp</div>
<div>+++ b/lib/MC/ELFObjectWriter.cpp</div>
<div>@@ -233,7 +233,8 @@ class ELFObjectWriter : public MCObjectWriter {</div>
<div> F.getContents().append(&buf[0], &buf[8]);</div>
<div> }</div>
<div> </div>
<div>- void WriteHeader(uint64_t SectionDataSize,</div>
<div>+ void WriteHeader(const MCAssembler &Asm,</div>
<div>+ uint64_t SectionDataSize,</div>
<div> unsigned NumberOfSections);</div>
<div> </div>
<div> void WriteSymbolEntry(MCDataFragment *SymtabF,</div>
<div>@@ -373,7 +374,8 @@ ELFObjectWriter::~ELFObjectWriter()</div>
<div> {}</div>
<div> </div>
<div> // Emit the ELF header.</div>
<div>-void ELFObjectWriter::WriteHeader(uint64_t SectionDataSize,</div>
<div>+void ELFObjectWriter::WriteHeader(const MCAssembler &Asm,</div>
<div>+ uint64_t SectionDataSize,</div>
<div> unsigned NumberOfSections) {</div>
<div> // ELF Header</div>
<div> // ----------</div>
<div>@@ -411,7 +413,9 @@ void ELFObjectWriter::WriteHeader(uint64_t SectionDataSize,</div>
<div> sizeof(ELF::Elf32_Ehdr))); // e_shoff = sec hdr table off in bytes</div>
<div> </div>
<div> // e_flags = whatever the target wants</div>
<div>- Write32(getEFlags());</div>
<div>+ unsigned Flags = getEFlags();</div>
<div>+ Flags |= Asm.getAsmEFlags();</div>
<div>+ Write32(Flags);</div>
<div> </div>
</blockquote>
<div><br>
</div>
<div>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.</div>
<br>
<blockquote type="cite">
<div> // e_ehsize = ELF header size</div>
<div> Write16(is64Bit() ? sizeof(ELF::Elf64_Ehdr) : sizeof(ELF::Elf32_Ehdr));</div>
<div>@@ -1534,7 +1538,7 @@ void ELFObjectWriter::WriteObject(MCAssembler &Asm,</div>
<div> }</div>
<div> </div>
<div> // Write out the ELF header ...</div>
<div>- WriteHeader(SectionHeaderOffset, NumSections + 1);</div>
<div>+ WriteHeader(Asm, SectionHeaderOffset, NumSections + 1);</div>
<div> </div>
<div> // ... then the regular sections ...</div>
<div> // + because of .shstrtab</div>
<div>diff --git a/lib/MC/MCAssembler.cpp b/lib/MC/MCAssembler.cpp</div>
<div>index c51ddc8..1fdb25c 100644</div>
<div>--- a/lib/MC/MCAssembler.cpp</div>
<div>+++ b/lib/MC/MCAssembler.cpp</div>
<div>@@ -263,8 +263,8 @@ MCAssembler::MCAssembler(MCContext &Context_, MCAsmBackend &Backend_,</div>
<div> MCCodeEmitter &Emitter_, MCObjectWriter &Writer_,</div>
<div> raw_ostream &OS_)</div>
<div> : Context(Context_), Backend(Backend_), Emitter(Emitter_), Writer(Writer_),</div>
<div>- OS(OS_), BundleAlignSize(0), RelaxAll(false), NoExecStack(false),</div>
<div>- SubsectionsViaSymbols(false) {</div>
<div>+ OS(OS_), BundleAlignSize(0), RelaxAll(false),</div>
<div>+ NoExecStack(false), SubsectionsViaSymbols(false), AsmEFlags(0) {</div>
<div> }</div>
<div> </div>
<div> MCAssembler::~MCAssembler() {</div>
<div>@@ -281,6 +281,7 @@ void MCAssembler::reset() {</div>
<div> RelaxAll = false;</div>
<div> NoExecStack = false;</div>
<div> SubsectionsViaSymbols = false;</div>
<div>+ AsmEFlags = 0;</div>
<div> </div>
<div> // reset objects owned by us</div>
<div> getBackend().reset();</div>
<div>diff --git a/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp b/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp</div>
<div>index 7afb77e..2c54480 100644</div>
<div>--- a/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp</div>
<div>+++ b/lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp</div>
<div>@@ -61,17 +61,9 @@ MipsELFObjectWriter::MipsELFObjectWriter(bool _is64Bit, uint8_t OSABI,</div>
<div> </div>
<div> MipsELFObjectWriter::~MipsELFObjectWriter() {}</div>
<div> </div>
<div>-// FIXME: get the real EABI Version from the Subtarget class.</div>
<div>+// ELF Header flags we will always produce</div>
<div> unsigned MipsELFObjectWriter::getEFlags() const {</div>
<div>-</div>
<div>- // FIXME: We can't tell if we are PIC (dynamic) or CPIC (static)</div>
<div>- unsigned Flag = ELF::EF_MIPS_NOREORDER;</div>
<div>-</div>
<div>- if (is64Bit())</div>
<div>- Flag |= ELF::EF_MIPS_ARCH_64R2;</div>
<div>- else</div>
<div>- Flag |= ELF::EF_MIPS_ARCH_32R2;</div>
<div>- return Flag;</div>
<div>+ return ELF::EF_MIPS_NOREORDER;</div>
<div> }</div>
<div> </div>
<div> const MCSymbol *MipsELFObjectWriter::ExplicitRelSym(const MCAssembler &Asm,</div>
<div>diff --git a/lib/Target/Mips/MipsAsmPrinter.cpp b/lib/Target/Mips/MipsAsmPrinter.cpp</div>
<div>index e3c3429..d334eac 100644</div>
<div>--- a/lib/Target/Mips/MipsAsmPrinter.cpp</div>
<div>+++ b/lib/Target/Mips/MipsAsmPrinter.cpp</div>
<div>@@ -32,9 +32,12 @@</div>
<div> #include "llvm/IR/InlineAsm.h"</div>
<div> #include "llvm/IR/Instructions.h"</div>
<div> #include "llvm/MC/MCAsmInfo.h"</div>
<div>+#include "llvm/MC/MCAssembler.h"</div>
<div> #include "llvm/MC/MCInst.h"</div>
<div>+#include "llvm/MC/MCObjectStreamer.h"</div>
<div> #include "llvm/MC/MCStreamer.h"</div>
<div> #include "llvm/MC/MCSymbol.h"</div>
<div>+#include "llvm/Support/ELF.h"</div>
<div> #include "llvm/Support/TargetRegistry.h"</div>
<div> #include "llvm/Support/raw_ostream.h"</div>
<div> #include "llvm/Target/Mangler.h"</div>
<div>@@ -545,9 +548,40 @@ void MipsAsmPrinter::EmitStartOfAsmFile(Module &M) {</div>
<div> </div>
<div> void MipsAsmPrinter::EmitEndOfAsmFile(Module &M) {</div>
<div> </div>
<div>+ if (OutStreamer.hasRawTextSupport()) return;</div>
<div>+</div>
<div> // Emit Mips ELF register info</div>
<div> Subtarget->getMReginfo().emitMipsReginfoSectionCG(</div>
<div> OutStreamer, getObjFileLowering(), *Subtarget);</div>
<div>+</div>
<div>+ // Update e_header flags</div>
<div>+ // This should and will be a single call to a routine living somewhere else,</div>
<div>+ // but for discussion purposes I will just make the individual settings here.</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
<br>
<blockquote type="cite">
<div>+ MCObjectStreamer& MCOS = static_cast< MCObjectStreamer & >(OutStreamer);</div>
<div>+ MCAssembler& MCA = MCOS.getAssembler();</div>
<div>+ unsigned EFlags = MCA.getAsmEFlags();</div>
<div>+</div>
<div>+ // Architecture</div>
<div>+ if (Subtarget->hasMips64r2())</div>
<div>+ EFlags |= ELF::EF_MIPS_ARCH_64R2;</div>
<div>+ else if (Subtarget->hasMips64())</div>
<div>+ EFlags |= ELF::EF_MIPS_ARCH_64;</div>
<div>+ else if (Subtarget->hasMips32r2())</div>
<div>+ EFlags |= ELF::EF_MIPS_ARCH_32R2;</div>
<div>+ else</div>
<div>+ EFlags |= ELF::EF_MIPS_ARCH_32;</div>
<div>+</div>
<div>+ // Relocation Model</div>
<div>+ Reloc::Model RM = Subtarget->getRelocationModel();</div>
<div>+ if (RM == Reloc::PIC_ || RM == Reloc::Default)</div>
<div>+ EFlags |= ELF::EF_MIPS_PIC;</div>
<div>+ else if (RM == Reloc::Static)</div>
<div>+ ; // Do nothing for Reloc::Static</div>
<div>+ else</div>
<div>+ llvm_unreachable("Unsupported relocation model for e_flags");</div>
<div>+</div>
<div>+ MCA.setAsmEFlags(EFlags);</div>
<div>+</div>
<div> }</div>
<div> </div>
<div> MachineLocation</div>
<div>diff --git a/lib/Target/Mips/MipsSubtarget.cpp b/lib/Target/Mips/MipsSubtarget.cpp</div>
<div>index 30d377a..6ad97db 100644</div>
<div>--- a/lib/Target/Mips/MipsSubtarget.cpp</div>
<div>+++ b/lib/Target/Mips/MipsSubtarget.cpp</div>
<div>@@ -26,13 +26,13 @@ void MipsSubtarget::anchor() { }</div>
<div> </div>
<div> MipsSubtarget::MipsSubtarget(const std::string &TT, const std::string &CPU,</div>
<div> const std::string &FS, bool little,</div>
<div>- Reloc::Model RM) :</div>
<div>+ Reloc::Model _RM) :</div>
<div> MipsGenSubtargetInfo(TT, CPU, FS),</div>
<div> MipsArchVersion(Mips32), MipsABI(UnknownABI), IsLittle(little),</div>
<div> IsSingleFloat(false), IsFP64bit(false), IsGP64bit(false), HasVFPU(false),</div>
<div> IsLinux(true), HasSEInReg(false), HasCondMov(false), HasSwap(false),</div>
<div> HasBitCount(false), HasFPIdx(false),</div>
<div>- InMips16Mode(false), HasDSP(false), HasDSPR2(false), IsAndroid(false)</div>
<div>+ InMips16Mode(false), HasDSP(false), HasDSPR2(false), IsAndroid(false), RM(_RM)</div>
<div> {</div>
<div> std::string CPUName = CPU;</div>
<div> if (CPUName.empty())</div>
<div>diff --git a/lib/Target/Mips/MipsSubtarget.h b/lib/Target/Mips/MipsSubtarget.h</div>
<div>index 001d8d1..5538615 100644</div>
<div>--- a/lib/Target/Mips/MipsSubtarget.h</div>
<div>+++ b/lib/Target/Mips/MipsSubtarget.h</div>
<div>@@ -100,6 +100,9 @@ protected:</div>
<div> // The instance to the register info section object</div>
<div> MipsReginfo MRI;</div>
<div> </div>
<div>+ // Relocation Model</div>
<div>+ Reloc::Model RM;</div>
<div>+</div>
<div> public:</div>
<div> virtual bool enablePostRAScheduler(CodeGenOpt::Level OptLevel,</div>
<div> AntiDepBreakMode& Mode,</div>
<div>@@ -152,6 +155,9 @@ public:</div>
<div> </div>
<div> // Grab MipsRegInfo object</div>
<div> const MipsReginfo &getMReginfo() const { return MRI; }</div>
<div>+</div>
<div>+ // Grab relocation model</div>
<div>+ Reloc::Model getRelocationModel () const {return RM;}</div>
<div> };</div>
<div> } // End llvm namespace</div>
<div> </div>
</blockquote>
</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
<div>
<div>On Jan 24, 2013, at 1:41 PM, "Carter, Jack" <<a href="mailto:jcarter@mips.com" target="_blank">jcarter@mips.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="font-family:Helvetica; font-size:medium; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:2; text-indent:0px; text-transform:none; white-space:normal; widows:2; word-spacing:0px">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">This is not a rehash of an old patch.<br>
<br>
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.<br>
<br>
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().<br>
<br>
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.<br>
<br>
I have looked at the ARMELFStreamer files and I do not see how they apply to the e_flag bit field data storage.<span class="Apple-converted-space"> </span><br>
<br>
Please review and comment.<span class="Apple-converted-space"> </span><br>
<br>
Regards,<br>
<br>
Jack<br>
</div>
<span><eheader4.patch></span></div>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</body>
</html>