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

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

Getting much closer.  Just a few more things.



================
Comment at: llvm/include/llvm/Object/COFF.h:1076
+  explicit ResourceSectionRef(StringRef Ref)
+      : BBS(BinaryByteStream(Ref, support::little)) {}
+
----------------
You can write this line:

```
  : BBS(Ref, support::little) {}
```


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1600
+  uint16_t Length;
+  std::error_code EC = errorToErrorCode(Reader.readInteger(Length));
+  if (EC)
----------------
ruiu wrote:
> You can define a variable inside an `if`.
> 
>   if (std::error_code EC = ...)
> 
> And this is preferred way if EC's scope is limited inside that `if` as narrower scope = better.
You can shorten all these by one line if you say:

```
if (auto EC = errorToErrorCode(Reader.readInteger(Length)))
  return EC;
```

You can shorten them all TO one line if you add a macro like:

```
#define RETURN_IF_ERROR(X) \
if (auto EC = errorToErrorCode(X)) \
  return EC;
```

And then say:

```
RETURN_IF_ERROR(Reader.readInteger(Length));
```


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1609-1612
+  std::string DirString;
+  for (int i = 0; i < 2 * Length; i += 2) {
+    DirString.push_back(RawDirString.data()[i]);
+  }
----------------
It looks like you're just truncating the second byte of each string?  You should probably do a proper UTF16 -> UTF8 conversion here.

```
#include "llvm/Support/ConvertUTF.h"
...
ArrayRef<UTF16> RawDirString;
std::string DirString;
RETURN_IF_ERROR(Reader.readArray(RawDirString, Length));
if (!llvm::convertUTF16ToUTF8String(RawDirString, DirString))
  return // some kind of error.
```


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1623-1626
+  auto Table = reinterpret_cast<const coff_resource_dir_table *>(
+      reinterpret_cast<const uint8_t *>(BBS.data().data()) +
+      Entry->Offset.value());
+  return Table;
----------------
ruiu wrote:
> You can directly return it without the assignment.
It would be nice to get rid of all the casting, in part because it makes the logic hard to follow, but also in part because there's no protection in case of corrupt data.  For example, this function can never return an error, even if `Entry` is bogus and makes you read off into nowhere.  So I would write this:

```
const coff_resource_dir_table *Table = nullptr;

BinaryStreamReader Reader(BBS);
Reader.setOffset(Entry->Offset.value());
RETURN_IF_ERROR(Reader.readObject(Table));
return Table;
```

this way if for some reason the offset is bogus, you'll get a useful error.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1630
+const coff_resource_dir_table *ResourceSectionRef::getBaseTable() {
+  return reinterpret_cast<const coff_resource_dir_table *>(BBS.data().data());
+}
----------------
How about adding a function:

```
ErrorOr<const coff_resource_dir_table&> getTableAtOffset(unsigned Offset) {
const coff_resource_dir_table *Table = nullptr;

BinaryStreamReader Reader(BBS);
Reader.setOffset(Offset);
RETURN_IF_ERROR(Reader.readObject(Table));
assert(Table != nullptr);
return *Table;
}
```

Then have this funciton and the one above it be:

```
ErrorOr<const coff_resource_dir_table&> getEntrySubDir(const coff_resource_dir_entry &Entry) {
  return getTableAtOffset(Entry->Offset.value());
}

ErrorOr<const coff_resource_dir_table&> getBaseTable() {
  return getTableAtOffset(0);
}
```

Note also that I'm using references now instead of pointers, since they can't be null.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:121
+  void printResourceDirectoryTable(ResourceSectionRef RSF,
+                                   const coff_resource_dir_table *Table,
+                                   StringRef Level);
----------------
Can this be a `const coff_resouce_dir_table &` instead of a pointer?


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:143-145
+  ErrorOr<const coff_resource_dir_entry *>
+  getResourceDirectoryTableEntry(const coff_resource_dir_table *Table,
+                                 uint32_t Index);
----------------
References instead of pointers.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:502-504
+        {"NoLibrary", COFF::IMAGE_WEAK_EXTERN_SEARCH_NOLIBRARY},
+        {"Library", COFF::IMAGE_WEAK_EXTERN_SEARCH_LIBRARY},
+        {"Alias", COFF::IMAGE_WEAK_EXTERN_SEARCH_ALIAS}};
----------------
This is strange.  This somehow got touched by clang-format.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1586-1590
+    const coff_resource_dir_entry *Entry;
+    if (EO)
+      Entry = *EO;
+    else
+      error(EO.getError());
----------------
Does this work?

```
auto &Entry = error(EO);
```

I know it does with `Expected<T>`, but not sure about `ErrorOr<T>`.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1599
+        RawName.insert(0, ": ");
+        Name = StringRef(RawName);
+      } else
----------------
This is wrong, because `RawName` will get destroyed at the end of this scope, and the `StringRef` will point to bogus memory.


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1618
+      if (Level == "Name")
+        NextLevel = StringRef("Language");
+      else
----------------
Does `NextLevel = "Language"` not work?


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1621
+        NextLevel = StringRef("Name");
+      ErrorOr<const coff_resource_dir_table *> EO = RSF.getEntrySubDir(Entry);
+      const coff_resource_dir_table *NextTable;
----------------
For these verbose names, you can use auto.  Although in this case the `auto &X = error(T)` pattern might work, so this whole block could just be simplified to:

```
auto &NextTable = error(RSF.getEntrySubDir(Entry));
```


================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1643-1645
+  if (Index >= (Table->NumberOfNameEntries + Table->NumberOfIDEntries))
+    return object_error::parse_failed;
+  return reinterpret_cast<const coff_resource_dir_entry *>(Table + 1) + Index;
----------------
I suppose this is fine now.  Ignore my comments earlier about `FixedStreamArray`.  It would be more idiomatic to use here if there was a class like

```
struct CoffResourceDirTable {
  const coff_resource_dir_table *Header;
  FixedStreamArray<coff_resource_dir_entry> Entries;
};
```

And then instead of having these functions return `coff_resource_dir_entry&` or `coff_resource_dir_table&`, have them return these structures instead.

In any case, don't worry about this for now.


https://reviews.llvm.org/D32609