[PATCH] COFFDumper: Dump data directory entries.

Richard legalize at xmission.com
Mon Jul 15 21:44:50 PDT 2013



================
Comment at: lib/Object/COFFObjectFile.cpp:433-438
@@ -432,7 +432,8 @@
   : ObjectFile(Binary::ID_COFF, Object)
   , COFFHeader(0)
   , PE32Header(0)
+  , DataDirectory(0)
   , SectionTable(0)
   , SymbolTable(0)
   , StringTable(0)
   , StringTableSize(0) {
----------------
Is it uncommon to initialize raw pointers with nullptr in clang?

================
Comment at: lib/Object/COFFObjectFile.cpp:465-467
@@ -463,5 +464,5 @@
 
   COFFHeader = reinterpret_cast<const coff_file_header *>(base() + CurPtr);
   if (!checkAddr(Data, ec, uintptr_t(COFFHeader), sizeof(coff_file_header)))
     return;
   CurPtr += sizeof(coff_file_header);
----------------
We're repeating this expression at least 4 times in this function:

  T* foo = reinterpret_cast<T *>(base() + CurrPtr + offset);
  if (!checkAddr(Data, ec, uintptr_t(foo), sizeof(T)*count))
    return;

Can we extract a method to get rid of the duplication and the casting noise?

================
Comment at: lib/Object/COFFObjectFile.cpp:479-486
@@ -477,4 +478,10 @@
       PE32Header = 0;
-    // There may be optional data directory after PE header. Skip them.
+    } else if (PE32Header->NumberOfRvaAndSize > 0) {
+      DataDirectory = reinterpret_cast<const data_directory *>(
+          base() + CurPtr + sizeof(pe32_header));
+      if (!checkAddr(Data, ec, uintptr_t(DataDirectory),
+                     sizeof(data_directory) * PE32Header->NumberOfRvaAndSize))
+        return;
+    }
     CurPtr += COFFHeader->SizeOfOptionalHeader;
   }
----------------
Does SizeOfOptionalHeader include the rvas?

Do we need to also advance CurPtr by sizeof(data_directory)*PE32Header->NumberOfRvaAndSize?

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:565
@@ -562,1 +564,3 @@
 
+void COFFDumper::printDataDirectory(uint32_t Index, std::string FieldName) {
+  const data_directory *Data;
----------------
pass strings by const reference

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:637
@@ +636,3 @@
+      DictScope D(W, "DataDirectory");
+      const char *directory[] = {
+        "ExportTable", "ImportTable", "ResourceTable", "ExceptionTable",
----------------
static const char *const directory[] = ...;

1) makes it clear this data is purely a static initializer and only computed once
2) makes it clear that not only are we not changing the characters, we aren't changing the pointers into the characters

Normally I wouldn't be so picky about const, but with raw pointers, always better safe than sorry.


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



More information about the llvm-commits mailing list