[PATCH] COFFDumper: Dump data directory entries.

[Rui Ueyama]

================
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) {
----------------
Richard wrote:
> Is it uncommon to initialize raw pointers with nullptr in clang?
In LLVM we use 0 as the literal for the null pointer.

================
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);
----------------
Richard wrote:
> 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?
Done.

================
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;
   }
----------------
Richard wrote:
> Does SizeOfOptionalHeader include the rvas?
> 
> Do we need to also advance CurPtr by sizeof(data_directory)*PE32Header->NumberOfRvaAndSize?
SizeOfOptionalHeader includes the data directory. Section table follows the data directory, and llvm-readobj can dump section table with this patch, so I think I'm sure about that.

================
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;
----------------
Richard wrote:
> pass strings by const reference
Done.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:637
@@ +636,3 @@
+      DictScope D(W, "DataDirectory");
+      const char *directory[] = {
+        "ExportTable", "ImportTable", "ResourceTable", "ExceptionTable",
----------------
Richard wrote:
> 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.
Good point. Fixed.


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