[PATCH] [ELFYAML] Support mips64 relocation record format in yaml2obj/obj2yaml

Sean Silva chisophugis at gmail.com
Sat Jan 24 07:41:56 PST 2015


================
Comment at: lib/Object/ELFYAML.cpp:583-595
@@ -547,3 +582,15 @@
+
+  if (Object->Header.Machine == ELFYAML::ELF_EM(ELF::EM_MIPS) &&
+      Object->Header.Class == ELFYAML::ELF_ELFCLASS(ELF::ELFCLASS64)) {
+    MappingNormalization<NormalizedMips64RelType, ELFYAML::ELF_REL> Key(
+        IO, Rel.Type);
+    IO.mapRequired("Type", Key->Type);
+    IO.mapOptional("Type2", Key->Type2, ELFYAML::ELF_REL(ELF::R_MIPS_NONE));
+    IO.mapOptional("Type3", Key->Type3, ELFYAML::ELF_REL(ELF::R_MIPS_NONE));
+    IO.mapOptional("SpecSym", Key->SpecSym, ELFYAML::ELF_RSS(ELF::RSS_UNDEF));
+  } else
+    IO.mapRequired("Type", Rel.Type);
+
   IO.mapOptional("Addend", Rel.Addend);
 }
 
----------------
Rather than changing the fields of ELFYAML::Relocation dynamically, I would prefer if we just treated this MIPS64 special case as just syntax sugar for filling in the bits of the usual `Type` field. E.g. make the `Type` field a union of either a single relocation value, as it "usually" is, or a sub-object with `Type`, `Type2`, `Type3`, `SpecSym` fields in the MIPS64 case.

So really the only changes to the code would end up being that we would optionally accept a subobject for the `Type` field with the MIPS64 special fields, and we would dump in that format when applicable.

================
Comment at: lib/Object/ELFYAML.cpp:583-595
@@ -547,3 +582,15 @@
+
+  if (Object->Header.Machine == ELFYAML::ELF_EM(ELF::EM_MIPS) &&
+      Object->Header.Class == ELFYAML::ELF_ELFCLASS(ELF::ELFCLASS64)) {
+    MappingNormalization<NormalizedMips64RelType, ELFYAML::ELF_REL> Key(
+        IO, Rel.Type);
+    IO.mapRequired("Type", Key->Type);
+    IO.mapOptional("Type2", Key->Type2, ELFYAML::ELF_REL(ELF::R_MIPS_NONE));
+    IO.mapOptional("Type3", Key->Type3, ELFYAML::ELF_REL(ELF::R_MIPS_NONE));
+    IO.mapOptional("SpecSym", Key->SpecSym, ELFYAML::ELF_RSS(ELF::RSS_UNDEF));
+  } else
+    IO.mapRequired("Type", Rel.Type);
+
   IO.mapOptional("Addend", Rel.Addend);
 }
 
----------------
silvas wrote:
> Rather than changing the fields of ELFYAML::Relocation dynamically, I would prefer if we just treated this MIPS64 special case as just syntax sugar for filling in the bits of the usual `Type` field. E.g. make the `Type` field a union of either a single relocation value, as it "usually" is, or a sub-object with `Type`, `Type2`, `Type3`, `SpecSym` fields in the MIPS64 case.
> 
> So really the only changes to the code would end up being that we would optionally accept a subobject for the `Type` field with the MIPS64 special fields, and we would dump in that format when applicable.
Ignore this comment. Phab seems broken and doesn't let me delete it.

================
Comment at: tools/yaml2obj/yaml2elf.cpp:93-100
@@ -92,2 +92,10 @@
 
+static bool is64Bit(const ELFYAML::Object &Doc) {
+  return Doc.Header.Class == ELFYAML::ELF_ELFCLASS(ELF::ELFCLASS64);
+}
+
+static bool isLittleEndian(const ELFYAML::Object &Doc) {
+  return Doc.Header.Data == ELFYAML::ELF_ELFDATA(ELF::ELFDATA2LSB);
+}
+
 namespace {
----------------
Please don't reuse these functions for just a single use. They are just local helpers (pre-lambda days) for main to make it less gross; it defeats the purpose to have them far away from main like this.

================
Comment at: tools/yaml2obj/yaml2elf.cpp:343-344
@@ -334,2 +342,4 @@
 
+  bool IsMips64EL = Doc.Header.Machine == ELFYAML::ELF_EM(llvm::ELF::EM_MIPS) &&
+                    isLittleEndian(Doc) && is64Bit(Doc);
   bool IsRela = Section.Type == llvm::ELF::SHT_RELA;
----------------
Rather than reusing the other two predicates here, I would prefer if you open-coded all the checks into an isMips64EL helper function. That makes it immediately obvious to a reader what is being checked.

http://reviews.llvm.org/D7136

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list