[PATCH] D32609: Update llvm-readobj -coff-resources to display tree structure.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/include/llvm/Object/COFF.h:655
+    support::ulittle32_t ID;
+    StringRef getIDTypeName() const {
+      switch (ID) {
----------------
zturner wrote:
> It's a little jarring to see methods in unions.  I think part of the reason is that when you're looking at a union, it's nice for it to be as compact as possible so that you can get a picture of the entire layout of the union in one screen.
> 
> Can you move all the methods out of the union and into the top level struct?
> 
> With that said, we don't normally put constant <-> string translation functions in low level headers like this.  Take a look at `COFFDumper.cpp`, you will see a lot of things like this:
> 
> ```
> static const EnumEntry<COFF::DLLCharacteristics> PEDLLCharacteristics[] = {
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA      ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE         ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY      ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT            ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_NO_ISOLATION         ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_NO_SEH               ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_NO_BIND              ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_APPCONTAINER         ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_WDM_DRIVER           ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_GUARD_CF             ),
>   LLVM_READOBJ_ENUM_ENT(COFF, IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE),
> };
> ```
> 
> I think we should do something similar for the `ResourceID` enum.
> 
> Also, unless you're using a strongly typed enum (e.g. `enum class`), you shouldn't explicitly scope it by saying `ResourceID::`.  So you have to pick one of the following two patterns:
> 
> ```
> enum class ResourceID {
>   Cursor,   // Names are not RID_ prefixed because you have to scope it with ResourceID::
>   Bitmap,
>   ...
> };
> 
> enum ResourceID {
>   RID_Cursor,   // Names are RID_ prefixed because you refer to them unqualified.
>   RID_Bitmap,
>   ...
> };
> ```
I opted to follow the pattern of other enums in COFFDumper.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1597-1602
+ErrorOr<const coff_resource_dir_table *>
+ResourceSectionRef::getTableAtOffset(uint32_t Offset) {
+  auto Table = reinterpret_cast<const coff_resource_dir_table *>(
+      reinterpret_cast<const uint8_t *>(Ref.data()) + Offset);
+  return Table;
+}
----------------
zturner wrote:
> Is this function ever called?
ah you're right I replaced this function and forgot to remove it.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1614
+  StringRef RawDirString;
+  Reader.setOffset(Offset + 2);
+  // Strings are stored as 2-byte aligned unicode characters but readFixedString
----------------
zturner wrote:
> What's the +2 for?
ah woops I didn't realize the reader updated its own offset as it reads.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1549-1550
+
+void COFFDumper::printResourceDirectoryTable(
+    ResourceSectionRef &RSF, const coff_resource_dir_table *Table,
+    StringRef Level) {
----------------
zturner wrote:
> Should `RSF` be passed by value?  It's immutable in principle anyway since all it contains is a `StringRef`, and it's thin as well.  Usually when classes are suffixed with `Ref`, LLVM convention is that we should pass them by value since they're "as good as references"
yeah that makes sense


https://reviews.llvm.org/D32609