[llvm-readobj] [COFF] Cope with debug directory payloads in unmapped areas

[mstorsjo]

Summary

According to the spec, the payload for debug directories can be in parts of the binary that aren't mapped at runtime - in these cases, AddressOfRawData is just set to zero.

Just skip trying to print the payload in these cases, for now. If we'd really want to print that, we'd need a binary test input file as yaml2obj can't easily be tricked into placing data in padding areas of the file.)

[jhenderson]

What happens currently without this change? I'm guessing it tries to read something bad?

FWIW, ELF's yaml2obj now has the ability to add data blocks to an ELF, without generating a section header. See the "Fill" section type. Perhaps that's something that could be implemented in COFF's version at some point?

Aside from that, this all looks okay to me, but I'd like another opinion, as I'm not too familiar with COFF.

llvm/test/tools/llvm-readobj/COFF/debug-directory-unmapped.test

2

It might be worth a comment at the start of this test explaining what exactly is being tested, as it's not particularly obvious to me just looking at the test.

[mstorsjo]

In D78920#2007188, @jhenderson wrote:

What happens currently without this change? I'm guessing it tries to read something bad?

Not really, the getDebugPDBInfo and getRvaAndSizeAsBytes methods do fail cleanly, but the llvm-readobj call errors out at that point without printing info for the rest of the file.

FWIW, ELF's yaml2obj now has the ability to add data blocks to an ELF, without generating a section header. See the "Fill" section type. Perhaps that's something that could be implemented in COFF's version at some point?

That sounds potentially useful - if someone wants to spend more time on this cornercase.

llvm/test/tools/llvm-readobj/COFF/debug-directory-unmapped.test

2

Good point, I can do that.

[mstorsjo]

Added a comment to the test file.

[jhenderson]

LGTM, with the suggestions.

llvm/test/tools/llvm-readobj/COFF/debug-directory-unmapped.test

2–5

Could you use '##' for the comment markers, please. This is a practice I'm encouraging in my reviews, as it helps comments stand out from the lit/FileCheck directives.

Also I'd change the bit in brackets to "Currently llvm-readobj only prints the entry itself and not the payload. Note that there isn't currently any meaningful data in this test input where it claims the debug entry payload is." and probably just get rid of the brackets.

[mstorsjo]

llvm/test/tools/llvm-readobj/COFF/debug-directory-unmapped.test

2–5

Sure, will fix before pushing. Thanks!

llvm/test/tools/llvm-readobj/COFF/debug-directory-unmapped.test

+## Test that printing debug directories that aren't part of the runtime
+## mapped sections doesn't fail. Currently llvm-readobj only prints the
+## entry itself and not the payload. Note that there isn't currently any
+## meaningful data in this test input where it claims the debug entry
+## payload is.
+
+# RUN: yaml2obj %s -o %t.exe
+
+# RUN: llvm-readobj --coff-debug-directory %t.exe | FileCheck %s
+
+# CHECK:      DebugDirectory [
+# CHECK-NEXT:   DebugEntry {
+# CHECK-NEXT:     Characteristics: 0x0
+# CHECK-NEXT:     TimeDateStamp: 2019-01-17 21:06:10 (0x5C40EE42)
+# CHECK-NEXT:     MajorVersion: 0x0
+# CHECK-NEXT:     MinorVersion: 0x0
+# CHECK-NEXT:     Type: CodeView (0x2)
+# CHECK-NEXT:     SizeOfData: 0x19
+# CHECK-NEXT:     AddressOfRawData: 0x0
+# CHECK-NEXT:     PointerToRawData: 0x3E4
+# CHECK-NEXT:   }
+# CHECK-NEXT:   DebugEntry {
+# CHECK-NEXT:     Characteristics: 0x0
+# CHECK-NEXT:     TimeDateStamp: 2019-01-17 21:06:10 (0x5C40EE42)
+# CHECK-NEXT:     MajorVersion: 0x0
+# CHECK-NEXT:     MinorVersion: 0x0
+# CHECK-NEXT:     Type: ExtendedDLLCharacteristics (0x14)
+# CHECK-NEXT:     SizeOfData: 0x4
+# CHECK-NEXT:     AddressOfRawData: 0x0
+# CHECK-NEXT:     PointerToRawData: 0x3E0
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:       1073741824
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [  ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  Debug:
+    RelativeVirtualAddress: 8192
+    Size:            56
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    VirtualAddress:  4096
+    VirtualSize:     16
+    SectionData:     C3909090909090909090909090909090
+  - Name:            .buildid
+    Characteristics: [  ]
+    VirtualAddress:  8192
+    VirtualSize:     56
+    SectionData:     0000000042EE405C00000000020000001900000000000000E40300000000000042EE405C00000000140000000400000000000000E0030000
+symbols:
+...

llvm/tools/llvm-readobj/COFFDumper.cpp

for (const debug_directory &D : Obj->debug_directories()) {
     W.printHex("Characteristics", D.Characteristics);
     W.printHex("TimeDateStamp", FormattedTime, D.TimeDateStamp);
     W.printHex("MajorVersion", D.MajorVersion);
     W.printHex("MinorVersion", D.MinorVersion);
     W.printEnum("Type", D.Type, makeArrayRef(ImageDebugType));
     W.printHex("SizeOfData", D.SizeOfData);
     W.printHex("AddressOfRawData", D.AddressOfRawData);
     W.printHex("PointerToRawData", D.PointerToRawData);
+    // Ideally, if D.AddressOfRawData == 0, we should try to load the payload
+    // using D.PointerToRawData instead.
+    if (D.AddressOfRawData == 0)
+      continue;
     if (D.Type == COFF::IMAGE_DEBUG_TYPE_CODEVIEW) {
       const codeview::DebugInfo *DebugInfo;
       StringRef PDBFileName;
       if (std::error_code EC = Obj->getDebugPDBInfo(&D, DebugInfo, PDBFileName))
         reportError(errorCodeToError(EC), Obj->getFileName());
 
       DictScope PDBScope(W, "PDBInfo");
       W.printHex("PDBSignature", DebugInfo->Signature.CVSignature);