Update llvm-readobj -coff-resources to display tree structure.

[ecbeckmann]

Summary

Continue making updates to llvm-readobj to display resource sections. This is necessary for testing the up and coming cvtres tool.

[zturner]

It looks like your diff includes all the changes from the previous patch which was already LGTM'ed. So that we don't have to re-review code which was already reviewed, can you create that includes only those changes which are new and re-upload?

[ecbeckmann]

Rebase to build off of boilerplate patch.

[ruiu]

In general you want to avoid checking in a binary file. Can you create that object file using llvm-mc? If so, please check in an assembly file or a .c file and generate an object file when the test is executed.

llvm/include/llvm/Object/COFF.h

641–644 ↗

(On Diff #96988)

You are not using this class, are you? If you plan to use it later, please introduce this later.

1068–1071 ↗

(On Diff #96988)

I think it is better to use ErrorOr<T>.

1076 ↗

(On Diff #96988)

StringRef has a (non-explicit) constructor that takes char*, so you don't need to construct StringRef explicitly. You can just return a string literal.

[zturner]

In addition to ruiu's comments about not checking in binary files, I think we also don't want to use outside projects as the source of our input files. It's pretty clear this object file came from a chrome binary. We should be coming making all of our own stuff from scratch, Chromium doesn't really have any kind of connection to LLVM so it doesn't make sense to use their bianries for our tests when we can just generate our own.

[ecbeckmann]

Added a new test using my own rc file, also fixed minor bugs in COFFDumper.

[ruiu]

The 3MB .bmp image file seems too large. Could you use a smaller image? A 1x1 image should suffice.

[ecbeckmann]

Use tiny bmp instead of large ones, for efficiency.

[ecbeckmann]

Sorry, this is not yet ready for review, I have some intermediate changes to complete.

llvm/include/llvm/Object/COFF.h

641–644 ↗

(On Diff #96988)

It is used in COFFDumper when we display each entry in a table.

[ruiu]

llvm/include/llvm/Object/COFF.h

641–644 ↗

(On Diff #96988)

So I do not see why you need this. Even if you need this, do you really need this many fields? They are actually pointing to the same memory location that is at offset 0 of a struct, so this is equivalent to

union coff_resource_dir_entry {
  support::ulittle32_t NameOffset;
  support::ulittle32_t ID;
  support::ulittle32_t DataEntryOffset
  support::ulittle32_t SubdirOffset;
};

It seems to me that you don't need to add this struct. You can just handle resource directory entry as a ulittle32_t, can't you?

1076 ↗

(On Diff #96988)

What about this?

1072 ↗

(On Diff #97479)

Does it compile? (Look at the stray ,).

1074 ↗

(On Diff #97479)

As a local convention of this directory, it seems functions that returns a name of something is named getSomething. So this should be named getIDName. (But what ID is this?)

[zturner]

llvm/include/llvm/Object/COFF.h

1072 ↗

(On Diff #97479)

Why is there a comma here?

llvm/lib/Object/COFFObjectFile.cpp

1604–1610 ↗

(On Diff #97479)

This code has endianness problems. See later for suggested fix.

llvm/tools/llvm-readobj/COFFDumper.cpp

1537 ↗

(On Diff #97479)

Can you use llvm::BinaryStreamReader here? This could would be written:

BinaryByteStream Stream(Ref, support::little);
BinaryStreamReader Reader(Stream);
const coff_resource_dir_table *TypeTable;
error(Reader.readObject(TypeTable));
// ...
for (int i=0; i < TypeTable->NumberOfNameEntries; ++i) {
  const coff_resource_dir_entry *Entry;
  error(Reader.readObject(Entry));
  uint32_t OldOffset = Reader.getOffset();

  Reader.setOffset(Entry->NameOffset);
  uint16_t Length;
  StringRef TypeName;
  error(Reader.readInteger(Length));
  error(Reader.readFixedString(TypeName, Length));
  ListScope ResourceType(W, "Type: " + TypeName.str());

  Reader.setOffset(OldOffset);
}

for (int i=0; i < TypeTable->NumberOfIDEntries; ++i) {
  const coff_resource_dir_entry *Entry;
  error(Reader.readObject(Entry));

  StringRef TypeName = RSF.convertIDToType(Entry->Identifier.ID);
  ListScope ResourceType(W, "Type: " + TypeName.str());
}

Note that your original code has a bug because it is using uint16_t to read an integer, instead of ulittle16_t. The code here fixes that since you specify the endianness in the stream.

[zturner]

llvm/include/llvm/Object/COFF.h

1076 ↗

(On Diff #97479)

Can we have an enumeration for this in COFF.h?

enum class ResourceID : uint32_t {
  RID_Cursor = 1,
  RID_Bitmap = 2,
  ...
};

[ecbeckmann]

Recursively display entire tree.

[ecbeckmann]

llvm/include/llvm/Object/COFF.h

641–644 ↗

(On Diff #96988)

Hmm I'm not quite sure what you mean. A resource directory entry is composed of two separate 32-bit fields, 64 bits in total. The first field can be either an offset to a string name or an ID number. The second field is either an offset to the resource data or the offset to the next directory level down the tree. We must access both of these fields to read an entry, we might be able to use a ulittle32_t to read the first field and then move up 4 bytes to read the second field, but it seems much clearer to be able to access either of them by name.

1074 ↗

(On Diff #97479)

This should actually be a function that is part of the coff_resource_dir_entry structure.

[zturner]

llvm/include/llvm/Object/COFF.h

641–644 ↗

(On Diff #96988)

I also agree that it's not an equivalent struct. For @ruiu's proposed definition, sizeof(coff_resource_dir_entry) == 4, but for the one in the patch, sizeof(coff_resource_dir_entry) == 8.

627 ↗

(On Diff #97560)

I think this enum should go in llvm/Support/COFF.h. It's confusing that there are two files named COFF.h, but this file is for object file layout stuff, and the other file is more semantic information and enum / constant definitions.

655 ↗

(On Diff #97560)

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,
  ...
};

703 ↗

(On Diff #97560)

IIUC you are are taking the lower 31 bits of NameOffset? Can you write this as:

return maskTrailingOnes<uint32_t>(31) & NameOffset;

This function is defined in MathExtras.h

710 ↗

(On Diff #97560)

Same here.

723–725 ↗

(On Diff #97560)

I don't think we should put this logic here. Although in practice the two structures appear immediately after each other, in theory they should be independent of each other and the parsing code should be responsible for knowing this.

1146 ↗

(On Diff #97560)

No need for the argument to be const. Also can you mark this constructor explicit?

1155 ↗

(On Diff #97560)

Instead of storing a StringRef, store a BinaryByteStream. This way you don't have to reconstruct it every time you want to read from it. And it's just as good as a StringRef

llvm/lib/Object/COFFObjectFile.cpp

1597–1602 ↗

(On Diff #97560)

Is this function ever called?

1614 ↗

(On Diff #97560)

What's the +2 for?

llvm/tools/llvm-readobj/COFFDumper.cpp

1549–1550 ↗

(On Diff #97560)

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"

[ecbeckmann]

Refactored parsing funcationality into more logical places.

[ecbeckmann]

llvm/include/llvm/Object/COFF.h

655 ↗

(On Diff #97560)

I opted to follow the pattern of other enums in COFFDumper.

llvm/lib/Object/COFFObjectFile.cpp

1597–1602 ↗

(On Diff #97560)

ah you're right I replaced this function and forgot to remove it.

1614 ↗

(On Diff #97560)

ah woops I didn't realize the reader updated its own offset as it reads.

llvm/tools/llvm-readobj/COFFDumper.cpp

1549–1550 ↗

(On Diff #97560)

yeah that makes sense

[ruiu]

llvm/lib/Object/COFFObjectFile.cpp

1600–1601 ↗

(On Diff #97750)

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.

1611 ↗

(On Diff #97750)

This seems odd. Why can't you do RawDirString[i]?

1623 ↗

(On Diff #97750)

You can directly return it without the assignment.

[ecbeckmann]

Cleaned up error code checks.

[zturner]

Getting much closer. Just a few more things.

llvm/include/llvm/Object/COFF.h

1076 ↗

(On Diff #97750)

You can write this line:

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

llvm/lib/Object/COFFObjectFile.cpp

1600 ↗

(On Diff #97750)

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));

1609–1612 ↗

(On Diff #97750)

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.

1623–1626 ↗

(On Diff #97750)

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.

1630 ↗

(On Diff #97750)

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.

llvm/tools/llvm-readobj/COFFDumper.cpp

121 ↗

(On Diff #97750)

Can this be a const coff_resouce_dir_table & instead of a pointer?

143–145 ↗

(On Diff #97750)

References instead of pointers.

502–504 ↗

(On Diff #97750)

This is strange. This somehow got touched by clang-format.

1586–1590 ↗

(On Diff #97750)

Does this work?

auto &Entry = error(EO);

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

1599 ↗

(On Diff #97750)

This is wrong, because RawName will get destroyed at the end of this scope, and the StringRef will point to bogus memory.

1618 ↗

(On Diff #97750)

Does NextLevel = "Language" not work?

1621 ↗

(On Diff #97750)

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));

1643–1645 ↗

(On Diff #97750)

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.

[ecbeckmann]

Some more cleanup.

[ecbeckmann]

llvm/tools/llvm-readobj/COFFDumper.cpp

1586–1590 ↗

(On Diff #97750)

Unfortunately error() does not accept type ErrorOr.

[ecbeckmann]

Fixed formatting issues.

[ecbeckmann]

Use the unwrapOrError function for clarity.

[ecbeckmann]

llvm/tools/llvm-readobj/COFFDumper.cpp

1586–1590 ↗

(On Diff #97750)

I found a function unwrapOrError that does exactly what we want.

[ruiu]

llvm/tools/llvm-readobj/COFFDumper.cpp

1628–1631 ↗

(On Diff #97877)

This is a comment about the style, but for operators who's precedences are "obvious", we don't use that many parentheses.

Everyone knows that + has lower precedence than >=.

*(reinterpret_cast<foo>(bar)) is the same as *reinterpret_cast<foo>(bar).

(&Table + 1) + Index is the same as &Table + 1 + Index.

[zturner]

llvm/tools/llvm-readobj/COFFDumper.cpp

1628–1631 ↗

(On Diff #97877)

Although both are ugly IMO :) How about

auto TablePtr = reinterpret_cast<const coff_resource_dir_entry *>(&Table);
return TablePtr[1 + Index];

[zturner]

llvm/tools/llvm-readobj/COFFDumper.cpp

1628–1631 ↗

(On Diff #97877)

Correction:

auto TablePtr = reinterpret_cast<const coff_resource_dir_entry *>(&Table + 1);
return TablePtr[Index];

[ecbeckmann]

Cleaned up some style.

[zturner]

lgtm, thanks for being so patient!

[ecbeckmann]

Rename the file from .o to .obj.coff, since it seems git ignores .o files

[ecbeckmann]

Remove the .obj so this will be done in another patch

[ecbeckmann]

• Quick fix to D32609, it seems .o files are not transferred in all cases.

[chapuni]

This is failing on some builders, includes valgrind, possibly sanitizers.

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/9118
http://bb.pgr.jp/builders/ninja-clang-i686-msc19-R/builds/13960

llvm/lib/Object/COFFObjectFile.cpp

1610 ↗

(On Diff #98117)

I can suggest that converter may be moved to caller, then this method may return StringRef.

1612 ↗

(On Diff #98117)

Don't return local std::string as StringRef.

llvm/tools/llvm-readobj/COFFDumper.cpp

1571 ↗

(On Diff #98117)

It writes back part of SmallString via StringRef.
Could you rewrite not to modify (SmallString)IDStr?

[zturner]

Since some bots are red and we're dealing with memory corruption / sanitizer failures which may require a closer investigation to fully resolve, let's revert this temporarily while you investigate?

[ecbeckmann]

• Quick fix to D32609, it seems .o files are not transferred in all cases.
• Hopefully one last commit to fix this patch, addresses string reference

llvm/tools/llvm-readobj/COFFDumper.cpp

1571 ↗

(On Diff #98117)

Is it necessarily incorrect to modify IDStr in this case? The StringRef points to something we own within this scope.

[chapuni]

llvm/tools/llvm-readobj/COFFDumper.cpp

1571 ↗

(On Diff #98117)

It is doing overlapping memcpy. I think it'd not be dangerous since memcpy is done toward upper address.

That said, please be careful to write back string via StringRef.

[ecbeckmann]

In D32609#748320, @zturner wrote:

Since some bots are red and we're dealing with memory corruption / sanitizer failures which may require a closer investigation to fully resolve, let's revert this temporarily while you investigate?

done in D32958

[ecbeckmann]

• Quick fix to D32609, it seems .o files are not transferred in all cases.
• Hopefully one last commit to fix this patch, addresses string reference
• Also fixed compiler warning of signed to unsigned conversion.

[ecbeckmann]

In D32609#748342, @ecbeckmann wrote:

In D32609#748320, @zturner wrote:

Since some bots are red and we're dealing with memory corruption / sanitizer failures which may require a closer investigation to fully resolve, let's revert this temporarily while you investigate?

done in D32958

actually it seems that the stringref fix resolves the failures, so we can probably go ahead with the merge.

[uweigand]

It looks like this broke SystemZ, and probably all big-endian platforms:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/8010/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Aresources.test

From the symptoms, it appears that the little-endian UTF16 halfwords from the COFF binary never get translated to big-endian host byte order.

llvm/trunk/include/llvm/Object/COFF.h

 #define LLVM_OBJECT_COFF_H
 
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/DebugInfo/CodeView/CVDebugRecord.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Object/ObjectFile.h"
+#include "llvm/Support/BinaryByteStream.h"
 #include "llvm/Support/COFF.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <system_error>
 
 namespace llvm {
 
 template <typename T> class ArrayRef;
 
 namespace object {
 
 class BaseRelocRef;
 class DelayImportDirectoryEntryRef;
 class ExportDirectoryEntryRef;
 class ImportDirectoryEntryRef;
 class ImportedSymbolRef;
+class ResourceSectionRef;
 
 using import_directory_iterator = content_iterator<ImportDirectoryEntryRef>;
 using delay_import_directory_iterator =
     content_iterator<DelayImportDirectoryEntryRef>;
 using export_directory_iterator = content_iterator<ExportDirectoryEntryRef>;
 using imported_symbol_iterator = content_iterator<ImportedSymbolRef>;
 using base_reloc_iterator = content_iterator<BaseRelocRef>;
 

 
 struct coff_base_reloc_block_entry {
   support::ulittle16_t Data;
 
   int getType() const { return Data >> 12; }
   int getOffset() const { return Data & ((1 << 12) - 1); }
 };
 
+struct coff_resource_dir_entry {
+  union {
+    support::ulittle32_t NameOffset;
+    support::ulittle32_t ID;
+    uint32_t getNameOffset() const {
+      return maskTrailingOnes<uint32_t>(31) & NameOffset;
+    }
+  } Identifier;
+  union {
+    support::ulittle32_t DataEntryOffset;
+    support::ulittle32_t SubdirOffset;
+
+    bool isSubDir() const { return SubdirOffset >> 31; }
+    uint32_t value() const {
+      return maskTrailingOnes<uint32_t>(31) & SubdirOffset;
+    }
+
+  } Offset;
+};
+
 struct coff_resource_dir_table {
   support::ulittle32_t Characteristics;
   support::ulittle32_t TimeDateStamp;
   support::ulittle16_t MajorVersion;
   support::ulittle16_t MinorVersion;
   support::ulittle16_t NumberOfNameEntries;
   support::ulittle16_t NumberOfIDEntries;
 };
public:
   std::error_code getRVA(uint32_t &Result) const;
 
 private:
   const coff_base_reloc_block_header *Header;
   uint32_t Index;
   const COFFObjectFile *OwningObject = nullptr;
 };
 
+class ResourceSectionRef {
+public:
+  ResourceSectionRef() = default;
+  explicit ResourceSectionRef(StringRef Ref) : BBS(Ref, support::little) {}
+
+  ErrorOr<StringRef> getEntryNameString(const coff_resource_dir_entry &Entry);
+  ErrorOr<const coff_resource_dir_table &>
+  getEntrySubDir(const coff_resource_dir_entry &Entry);
+  ErrorOr<const coff_resource_dir_table &> getBaseTable();
+
+private:
+  BinaryByteStream BBS;
+
+  ErrorOr<const coff_resource_dir_table &> getTableAtOffset(uint32_t Offset);
+  ErrorOr<StringRef> getDirStringAtOffset(uint32_t Offset);
+};
+
 // Corresponds to `_FPO_DATA` structure in the PE/COFF spec.
 struct FpoData {
   support::ulittle32_t Offset; // ulOffStart: Offset 1st byte of function code
   support::ulittle32_t Size;   // cbProcSize: # bytes in function
   support::ulittle32_t NumLocals; // cdwLocals: # bytes in locals/4
   support::ulittle16_t NumParams; // cdwParams: # bytes in params/4
   support::ulittle16_t Attributes;
 

llvm/trunk/include/llvm/Support/COFF.h

enum Characteristics {
     IMAGE_FILE_DLL                     = 0x2000,
     /// This file should only be run on a uniprocessor machine.
     IMAGE_FILE_UP_SYSTEM_ONLY          = 0x4000,
     /// Big endian: the MSB precedes the LSB in memory. This is deprecated
     /// and should be 0.
     IMAGE_FILE_BYTES_REVERSED_HI       = 0x8000
   };
 
+  enum ResourceTypeID {
+    RID_Cursor = 1,
+    RID_Bitmap = 2,
+    RID_Icon = 3,
+    RID_Menu = 4,
+    RID_Dialog = 5,
+    RID_String = 6,
+    RID_FontDir = 7,
+    RID_Font = 8,
+    RID_Accelerator = 9,
+    RID_RCData = 10,
+    RID_MessageTable = 11,
+    RID_Group_Cursor = 12,
+    RID_Group_Icon = 14,
+    RID_Version = 16,
+    RID_DLGInclude = 17,
+    RID_PlugPlay = 19,
+    RID_VXD = 20,
+    RID_AniCursor = 21,
+    RID_AniIcon = 22,
+    RID_HTML = 23,
+    RID_Manifest = 24,
+  };
+
   struct symbol {
     char     Name[NameSize];
     uint32_t Value;
     int32_t  SectionNumber;
     uint16_t Type;
     uint8_t  StorageClass;
     uint8_t  NumberOfAuxSymbols;
   };

llvm/trunk/lib/Object/COFFObjectFile.cpp

 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Object/ObjectFile.h"
+#include "llvm/Support/BinaryStreamReader.h"
 #include "llvm/Support/COFF.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <algorithm>
 #include <cassert>
 #include <cstddef>
void COFFObjectFile::moveSymbolNext(DataRefImpl &Ref) const {
   } else {
     llvm_unreachable("no symbol table pointer!");
   }
 }
 
 Expected<StringRef> COFFObjectFile::getSymbolName(DataRefImpl Ref) const {
   COFFSymbolRef Symb = getCOFFSymbol(Ref);
   StringRef Result;
-  std::error_code EC = getSymbolName(Symb, Result);
-  if (EC)
+  if (std::error_code EC = getSymbolName(Symb, Result))
     return errorCodeToError(EC);
   return Result;
 }
 
 uint64_t COFFObjectFile::getSymbolValueImpl(DataRefImpl Ref) const {
   return getCOFFSymbol(Ref).getValue();
 }
 
std::error_code BaseRelocRef::getType(uint8_t &Type) const {
   return std::error_code();
 }
 
 std::error_code BaseRelocRef::getRVA(uint32_t &Result) const {
   auto *Entry = reinterpret_cast<const coff_base_reloc_block_entry *>(Header + 1);
   Result = Header->PageRVA + Entry[Index].getOffset();
   return std::error_code();
 }
+
+#define RETURN_IF_ERROR(X)                                                     \
+  if (auto EC = errorToErrorCode(X))                                           \
+    return EC;
+
+ErrorOr<StringRef> ResourceSectionRef::getDirStringAtOffset(uint32_t Offset) {
+  BinaryStreamReader Reader = BinaryStreamReader(BBS);
+  Reader.setOffset(Offset);
+  uint16_t Length;
+  RETURN_IF_ERROR(Reader.readInteger(Length));
+  ArrayRef<UTF16> RawDirString;
+  // Strings are stored as 2-byte aligned unicode characters but readFixedString
+  // assumes byte string, so we double length.
+  RETURN_IF_ERROR(Reader.readArray(RawDirString, Length));
+  std::string DirString;
+  if (!llvm::convertUTF16ToUTF8String(RawDirString, DirString))
+    return object_error::parse_failed;
+  return DirString;
+}
+
+ErrorOr<StringRef>
+ResourceSectionRef::getEntryNameString(const coff_resource_dir_entry &Entry) {
+  return getDirStringAtOffset(Entry.Identifier.getNameOffset());
+}
+
+ErrorOr<const coff_resource_dir_table &>
+ResourceSectionRef::getTableAtOffset(uint32_t 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;
+}
+
+ErrorOr<const coff_resource_dir_table &>
+ResourceSectionRef::getEntrySubDir(const coff_resource_dir_entry &Entry) {
+  return getTableAtOffset(Entry.Offset.value());
+}
+
+ErrorOr<const coff_resource_dir_table &> ResourceSectionRef::getBaseTable() {
+  return getTableAtOffset(0);
+}

llvm/trunk/test/tools/llvm-readobj/Inputs/resources/test_resource.rc

+#include "windows.h"
+
+LANGUAGE LANG_ENGLISH, SUBLANG_ENGLISH_US
+
+myaccelerators ACCELERATORS
+{
+  "^C", 999, VIRTKEY, ALT
+  "D", 1100, VIRTKEY, CONTROL, SHIFT
+  "^R", 444, ASCII, NOINVERT
+}
+
+cursor BITMAP "cursor_small.bmp"
+okay BITMAP "okay_small.bmp"
+
+14432 MENU
+LANGUAGE LANG_CHINESE, SUBLANG_CHINESE_SIMPLIFIED
+{
+  MENUITEM "yu", 100
+  MENUITEM "shala", 101
+  MENUITEM "kaoya", 102
+}
+
+testdialog DIALOG 10, 10, 200, 300
+STYLE WS_POPUP | WS_BORDER
+CAPTION "Test"
+{
+  CTEXT "Continue:", 1, 10, 10, 230, 14
+  PUSHBUTTON "&OK", 2, 66, 134, 161, 13
+}
+
+12 ACCELERATORS
+{
+  "X", 164, VIRTKEY, ALT
+  "H", 5678, VIRTKEY, CONTROL, SHIFT
+  "^R", 444, ASCII, NOINVERT
+}
+
+"eat" MENU
+LANGUAGE LANG_ENGLISH, SUBLANG_ENGLISH_AUS
+{
+  MENUITEM "fish", 100
+  MENUITEM "salad", 101
+  MENUITEM "duck", 102
+}

llvm/trunk/test/tools/llvm-readobj/resources.test

-RUN: llvm-readobj -coff-resources %p/Inputs/zero-string-table.obj.coff-i386 \
-RUN:   | FileCheck %s -check-prefix RESOURCE
+// Check dumping of the .rsrc section(s)
+// The input was generated with the following commands, using the original Windows
+// rc.exe and cvtres.exe:
+// > rc /fo test_resource.res /nologo test_resource.rc
+// > cvtres /machine:X86 /readonly /nologo /out:test_resource.o test_resource.res
 
-RESOURCE:      Resources [
-RESOURCE-NEXT:   Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
-RESOURCE-NEXT:   .rsrc$01 Data (
-RESOURCE-NEXT:     0000: 00000000 00000000 00000000 00000100  |................|
-RESOURCE-NEXT:     0010: 06000000 18000080 00000000 00000000  |................|
-RESOURCE-NEXT:     0020: 00000000 00000100 01000000 30000080  |............0...|
-RESOURCE-NEXT:     0030: 00000000 00000000 00000000 00000100  |................|
-RESOURCE-NEXT:     0040: 09040000 48000000 00000000 2A000000  |....H.......*...|
-RESOURCE-NEXT:     0050: 00000000 00000000                    |........|
-RESOURCE-NEXT:   )
-RESOURCE-NEXT:   .rsrc$02 Data (
-RESOURCE-NEXT:     0000: 00000500 48006500 6C006C00 6F000000  |....H.e.l.l.o...|
-RESOURCE-NEXT:     0010: 00000000 00000000 00000000 00000000  |................|
-RESOURCE-NEXT:     0020: 00000000 00000000 00000000 00000000  |................|
-RESOURCE-NEXT:   )
-RESOURCE-NEXT: ]
+RUN: llvm-readobj -coff-resources -section-data %p/Inputs/zero-string-table.obj.coff-i386 \
+RUN:   | FileCheck %s -check-prefix ZERO
+RUN: llvm-readobj -coff-resources %p/Inputs/resources/test_resource.o \
+RUN:   | FileCheck %s -check-prefix TEST_RES
+
+ZERO:      Resources [
+ZERO-NEXT:   String Name Entries: 0
+ZERO-NEXT:   ID Entries: 1
+ZERO-NEXT:   Type: kRT_STRING (ID 6) [
+ZERO-NEXT:     String Name Entries: 0
+ZERO-NEXT:     ID Entries: 1
+ZERO-NEXT:     Name: (ID 1) [
+ZERO-NEXT:       String Name Entries: 0
+ZERO-NEXT:       ID Entries: 1
+ZERO-NEXT:       Language: (ID 1033) [
+ZERO-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+ZERO-NEXT:         Major Version: 0
+ZERO-NEXT:         Minor Version: 0
+ZERO-NEXT:       ]
+ZERO-NEXT:     ]
+ZERO-NEXT:   ]
+
+
+TEST_RES:      Resources [
+TEST_RES-NEXT:   String Name Entries: 0
+TEST_RES-NEXT:   ID Entries: 4
+TEST_RES-NEXT:   Type: kRT_BITMAP (ID 2) [
+TEST_RES-NEXT:     String Name Entries: 2
+TEST_RES-NEXT:     ID Entries: 0
+TEST_RES-NEXT:     Name: CURSOR [
+TEST_RES-NEXT:       String Name Entries: 0
+TEST_RES-NEXT:       ID Entries: 1
+TEST_RES-NEXT:       Language: (ID 1033) [
+TEST_RES-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+TEST_RES-NEXT:         Major Version: 0
+TEST_RES-NEXT:         Minor Version: 0
+TEST_RES-NEXT:       ]
+TEST_RES-NEXT:     ]
+TEST_RES-NEXT:     Name: OKAY [
+TEST_RES-NEXT:       String Name Entries: 0
+TEST_RES-NEXT:       ID Entries: 1
+TEST_RES-NEXT:       Language: (ID 1033) [
+TEST_RES-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+TEST_RES-NEXT:         Major Version: 0
+TEST_RES-NEXT:         Minor Version: 0
+TEST_RES-NEXT:       ]
+TEST_RES-NEXT:     ]
+TEST_RES-NEXT:   ]
+TEST_RES-NEXT:   Type: kRT_MENU (ID 4) [
+TEST_RES-NEXT:     String Name Entries: 1
+TEST_RES-NEXT:     ID Entries: 1
+TEST_RES-NEXT:     Name: "EAT" [
+TEST_RES-NEXT:       String Name Entries: 0
+TEST_RES-NEXT:       ID Entries: 1
+TEST_RES-NEXT:       Language: (ID 3081) [
+TEST_RES-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+TEST_RES-NEXT:         Major Version: 0
+TEST_RES-NEXT:         Minor Version: 0
+TEST_RES-NEXT:       ]
+TEST_RES-NEXT:     ]
+TEST_RES-NEXT:     Name: (ID 14432) [
+TEST_RES-NEXT:       String Name Entries: 0
+TEST_RES-NEXT:       ID Entries: 1
+TEST_RES-NEXT:       Language: (ID 2052) [
+TEST_RES-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+TEST_RES-NEXT:         Major Version: 0
+TEST_RES-NEXT:         Minor Version: 0
+TEST_RES-NEXT:       ]
+TEST_RES-NEXT:     ]
+TEST_RES-NEXT:   ]
+TEST_RES-NEXT:   Type: kRT_DIALOG (ID 5) [
+TEST_RES-NEXT:     String Name Entries: 1
+TEST_RES-NEXT:     ID Entries: 0
+TEST_RES-NEXT:     Name: TESTDIALOG [
+TEST_RES-NEXT:       String Name Entries: 0
+TEST_RES-NEXT:       ID Entries: 1
+TEST_RES-NEXT:       Language: (ID 1033) [
+TEST_RES-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+TEST_RES-NEXT:         Major Version: 0
+TEST_RES-NEXT:         Minor Version: 0
+TEST_RES-NEXT:       ]
+TEST_RES-NEXT:     ]
+TEST_RES-NEXT:   ]
+TEST_RES-NEXT:   Type: kRT_ACCELERATOR (ID 9) [
+TEST_RES-NEXT:     String Name Entries: 1
+TEST_RES-NEXT:     ID Entries: 1
+TEST_RES-NEXT:     Name: MYACCELERATORS [
+TEST_RES-NEXT:       String Name Entries: 0
+TEST_RES-NEXT:       ID Entries: 1
+TEST_RES-NEXT:       Language: (ID 1033) [
+TEST_RES-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+TEST_RES-NEXT:         Major Version: 0
+TEST_RES-NEXT:         Minor Version: 0
+TEST_RES-NEXT:       ]
+TEST_RES-NEXT:     ]
+TEST_RES-NEXT:     Name: (ID 12) [
+TEST_RES-NEXT:       String Name Entries: 0
+TEST_RES-NEXT:       ID Entries: 1
+TEST_RES-NEXT:       Language: (ID 1033) [
+TEST_RES-NEXT:         Time/Date Stamp: 1970-01-01 00:00:00 (0x0)
+TEST_RES-NEXT:         Major Version: 0
+TEST_RES-NEXT:         Minor Version: 0
+TEST_RES-NEXT:       ]
+TEST_RES-NEXT:     ]
+TEST_RES-NEXT:   ]

llvm/trunk/tools/llvm-readobj/COFFDumper.cpp

private:
   void printCodeViewFileChecksums(StringRef Subsection);
 
   void printCodeViewInlineeLines(StringRef Subsection);
 
   void printRelocatedField(StringRef Label, const coff_section *Sec,
                            uint32_t RelocOffset, uint32_t Offset,
                            StringRef *RelocSym = nullptr);
 
+  void printResourceDirectoryTable(ResourceSectionRef RSF,
+                                   const coff_resource_dir_table &Table,
+                                   StringRef Level);
+
   void printBinaryBlockWithRelocs(StringRef Label, const SectionRef &Sec,
                                   StringRef SectionContents, StringRef Block);
 
   /// Given a .debug$S section, find the string table and file checksum table.
   void initializeFileAndStringTables(BinaryStreamReader &Reader);
 
   void cacheRelocations();
 
   std::error_code resolveSymbol(const coff_section *Section, uint64_t Offset,
                                 SymbolRef &Sym);
   std::error_code resolveSymbolName(const coff_section *Section,
                                     uint64_t Offset, StringRef &Name);
   std::error_code resolveSymbolName(const coff_section *Section,
                                     StringRef SectionContents,
                                     const void *RelocPtr, StringRef &Name);
   void printImportedSymbols(iterator_range<imported_symbol_iterator> Range);
   void printDelayImportedSymbols(
       const DelayImportDirectoryEntryRef &I,
       iterator_range<imported_symbol_iterator> Range);
+  ErrorOr<const coff_resource_dir_entry &>
+  getResourceDirectoryTableEntry(const coff_resource_dir_table &Table,
+                                 uint32_t Index);
 
   typedef DenseMap<const coff_section*, std::vector<RelocationRef> > RelocMapTy;
 
   const llvm::object::COFFObjectFile *Obj;
   bool RelocCached = false;
   RelocMapTy RelocMap;
 
   BinaryByteStream ChecksumContents;

 
 static const EnumEntry<uint8_t> FileChecksumKindNames[] = {
   LLVM_READOBJ_ENUM_CLASS_ENT(FileChecksumKind, None),
   LLVM_READOBJ_ENUM_CLASS_ENT(FileChecksumKind, MD5),
   LLVM_READOBJ_ENUM_CLASS_ENT(FileChecksumKind, SHA1),
   LLVM_READOBJ_ENUM_CLASS_ENT(FileChecksumKind, SHA256),
 };
 
+static const EnumEntry<COFF::ResourceTypeID> ResourceTypeNames[]{
+    {"kRT_CURSOR (ID 1)", COFF::RID_Cursor},
+    {"kRT_BITMAP (ID 2)", COFF::RID_Bitmap},
+    {"kRT_ICON (ID 3)", COFF::RID_Icon},
+    {"kRT_MENU (ID 4)", COFF::RID_Menu},
+    {"kRT_DIALOG (ID 5)", COFF::RID_Dialog},
+    {"kRT_STRING (ID 6)", COFF::RID_String},
+    {"kRT_FONTDIR (ID 7)", COFF::RID_FontDir},
+    {"kRT_FONT (ID 8)", COFF::RID_Font},
+    {"kRT_ACCELERATOR (ID 9)", COFF::RID_Accelerator},
+    {"kRT_RCDATA (ID 10)", COFF::RID_RCData},
+    {"kRT_MESSAGETABLE (ID 11)", COFF::RID_MessageTable},
+    {"kRT_GROUP_CURSOR (ID 12)", COFF::RID_Group_Cursor},
+    {"kRT_GROUP_ICON (ID 14)", COFF::RID_Group_Icon},
+    {"kRT_VERSION (ID 16)", COFF::RID_Version},
+    {"kRT_DLGINCLUDE (ID 17)", COFF::RID_DLGInclude},
+    {"kRT_PLUGPLAY (ID 19)", COFF::RID_PlugPlay},
+    {"kRT_VXD (ID 20)", COFF::RID_VXD},
+    {"kRT_ANICURSOR (ID 21)", COFF::RID_AniCursor},
+    {"kRT_ANIICON (ID 22)", COFF::RID_AniIcon},
+    {"kRT_HTML (ID 23)", COFF::RID_HTML},
+    {"kRT_MANIFEST (ID 24)", COFF::RID_Manifest}};
+
 template <typename T>
 static std::error_code getSymbolAuxData(const COFFObjectFile *Obj,
                                         COFFSymbolRef Symbol,
                                         uint8_t AuxSymbolIdx, const T *&Aux) {
   ArrayRef<uint8_t> AuxData = Obj->getSymbolAuxData(Symbol);
   AuxData = AuxData.slice(AuxSymbolIdx * Obj->getSymbolTableEntrySize());
   Aux = reinterpret_cast<const T*>(AuxData.data());
   return readobj_error::success;
for (const SectionRef &S : Obj->sections()) {
     error(S.getName(Name));
     if (!Name.startswith(".rsrc"))
       continue;
 
     StringRef Ref;
     error(S.getContents(Ref));
 
     if ((Name == ".rsrc") || (Name == ".rsrc$01")) {
-      auto Table =
-          reinterpret_cast<const coff_resource_dir_table *>(Ref.data());
-      char FormattedTime[20];
-      time_t TDS = time_t(Table->TimeDateStamp);
-      strftime(FormattedTime, sizeof(FormattedTime), "%Y-%m-%d %H:%M:%S",
-               gmtime(&TDS));
-      W.printHex("Time/Date Stamp", FormattedTime, Table->TimeDateStamp);
+      ResourceSectionRef RSF(Ref);
+      auto &BaseTable = unwrapOrError(RSF.getBaseTable());
+      printResourceDirectoryTable(RSF, BaseTable, "Type");
     }
+    if (opts::SectionData)
       W.printBinaryBlock(Name.str() + " Data", Ref);
   }
 }
 
+void COFFDumper::printResourceDirectoryTable(
+    ResourceSectionRef RSF, const coff_resource_dir_table &Table,
+    StringRef Level) {
+  W.printNumber("String Name Entries", Table.NumberOfNameEntries);
+  W.printNumber("ID Entries", Table.NumberOfIDEntries);
+
+  char FormattedTime[20] = {};
+  time_t TDS = time_t(Table.TimeDateStamp);
+  strftime(FormattedTime, 20, "%Y-%m-%d %H:%M:%S", gmtime(&TDS));
+
+  // Iterate through level in resource directory tree.
+  for (int i = 0; i < Table.NumberOfNameEntries + Table.NumberOfIDEntries;
+       i++) {
+    auto Entry = unwrapOrError(getResourceDirectoryTableEntry(Table, i));
+    StringRef Name;
+    SmallString<20> IDStr;
+    raw_svector_ostream OS(IDStr);
+    if (i < Table.NumberOfNameEntries) {
+      StringRef EntryNameString = unwrapOrError(RSF.getEntryNameString(Entry));
+      OS << ": ";
+      OS << EntryNameString.str();
+    } else {
+      if (Level == "Type") {
+        ScopedPrinter Printer(OS);
+        Printer.printEnum("", Entry.Identifier.ID,
+                          makeArrayRef(ResourceTypeNames));
+        IDStr = IDStr.slice(0, IDStr.find_first_of(")", 0) + 1);
+      } else {
+        OS << ": (ID " << Entry.Identifier.ID << ")";
+      }
+    }
+    Name = StringRef(IDStr);
+    ListScope ResourceType(W, Level.str() + Name.str());
+    if (Entry.Offset.isSubDir()) {
+      StringRef NextLevel;
+      if (Level == "Name")
+        NextLevel = "Language";
+      else
+        NextLevel = "Name";
+      auto &NextTable = unwrapOrError(RSF.getEntrySubDir(Entry));
+      printResourceDirectoryTable(RSF, NextTable, NextLevel);
+    } else {
+      W.printHex("Time/Date Stamp", FormattedTime, Table.TimeDateStamp);
+      W.printNumber("Major Version", Table.MajorVersion);
+      W.printNumber("Minor Version", Table.MinorVersion);
+    }
+  }
+}
+
+ErrorOr<const coff_resource_dir_entry &>
+COFFDumper::getResourceDirectoryTableEntry(const coff_resource_dir_table &Table,
+                                           uint32_t Index) {
+  if (Index >= Table.NumberOfNameEntries + Table.NumberOfIDEntries)
+    return object_error::parse_failed;
+  auto TablePtr = reinterpret_cast<const coff_resource_dir_entry *>(&Table + 1);
+  return TablePtr[Index];
+}
+
 void COFFDumper::printStackMap() const {
   object::SectionRef StackMapSection;
   for (auto Sec : Obj->sections()) {
     StringRef Name;
     Sec.getName(Name);
     if (Name == ".llvm_stackmaps") {
       StackMapSection = Sec;
       break;