<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"><base href="x-msg://2955/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">LGTM!<div><br></div><div>-Jim</div><div><br><div><div>On Jan 28, 2013, at 3:59 PM, "Carter, Jack" <<a href="mailto:jcarter@mips.com">jcarter@mips.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div ocsi="0" fpstyle="1" 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-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; "><div style="direction: ltr; font-family: Tahoma; 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'; font-size: 16px; "><hr tabindex="-1"><div id="divRpF818314" 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">grosbach@apple.com</a>]<br><b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, January 24, 2013 5: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">llvm-commits@cs.uiuc.edu</a>; Eli Bendersky;<span class="Apple-converted-space"> </span><a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>;<span class="Apple-converted-space"> </span><a href="mailto:bruno.cardoso@gmail.com">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><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;<span class="Apple-converted-space"> </span><a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>;<span class="Apple-converted-space"> </span><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><span><general_eflags.patch></span><span><arm_eflags.patch></span><span><mips_eflags.patch></span></div></blockquote></div><br></div></body></html>