[PATCH] Object: Provide a richer means of describing auxiliary symbols

Rui Ueyama ruiu at google.com
Mon Mar 17 12:42:23 PDT 2014



================
Comment at: include/llvm/Object/COFFYAML.h:39
@@ -37,1 +38,3 @@
 namespace COFFYAML {
+  LLVM_YAML_STRONG_TYPEDEF(uint8_t, COMDATType)
+  LLVM_YAML_STRONG_TYPEDEF(uint32_t, WeakExternalCharacteristics)
----------------
nit: the latest coding standard states that we shouldn't indent namespace. I don't think we should fix it in this patch but you might want to do in a followup patch.

================
Comment at: include/llvm/Support/COFF.h:383
@@ -373,1 +382,3 @@
+    uint32_t SymbolTableIndex;
+    char     unused2[12];
   };
----------------
If the definitions for Support/COFF.h are for in-memory representation of a COFF file, why do we need padding fields?

================
Comment at: include/llvm/Object/COFF.h:266
@@ +265,3 @@
+  support::ulittle8_t  Reserved;
+  support::ulittle32_t SymbolTableIndex;
+  char                 unused[12];
----------------
First I thought this field is not properly aligned, but as we discussed offline, ulittl32_t is a magic struct that can be located on any byte offset. Cool.

================
Comment at: include/llvm/Support/YAMLTraits.h:456
@@ +455,3 @@
+    } else {
+      if (UseDefault)
+        Val = DefaultValue;
----------------
nit: "else { if" -> "else if"

================
Comment at: lib/Object/COFFYAML.cpp:217
@@ +216,3 @@
+  NSectionSelectionType(IO &)
+      : SelectionType(COFFYAML::COMDATType(0)) {}
+  NSectionSelectionType(IO &, uint8_t C)
----------------
Does 0 here mean nullptr? Should we use nullptr? (as C++11 is now allowed.)

================
Comment at: test/Object/Inputs/COFF/i386.yaml:42
@@ -41,3 +41,3 @@
     StorageClass: IMAGE_SYM_CLASS_STATIC # (3)
-    NumberOfAuxSymbols: 1
-    AuxiliaryData:  !hex "240000000300000000000000010000000000" # |$.................|
+    SectionDefinition:
+      Length:          36
----------------
LLD also has COFF yaml files so they have to be converted too (in a different patch, of course). This really looks nice than hex-encoded value! :)

================
Comment at: tools/obj2yaml/coff2yaml.cpp:97
@@ +96,3 @@
+
+    if (Symbol->NumberOfAuxSymbols > 0) {
+      ArrayRef<uint8_t> AuxData = Obj.getSymbolAuxData(Symbol);
----------------
This piece of code looks a bit too large to insert into here. Can you make this a separate function?


http://llvm-reviews.chandlerc.com/D3092



More information about the llvm-commits mailing list