[PATCH] review for MC ELF header e_flags setting

Carter, Jack jcarter at mips.com
Mon Jan 28 15:59:15 PST 2013


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/20130128/cc9ab6f0/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/20130128/cc9ab6f0/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/20130128/cc9ab6f0/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/20130128/cc9ab6f0/attachment-0002.bin>


More information about the llvm-commits mailing list