[llvm-objcopy] [COFF] Fix a misconception about debug directory payloads

[mstorsjo]

Summary

The debug directory payload is not located directly after the debug directory entry itself, but can essentially be located anywhere in the binary (even outside of mapped sections, although we don't handle that case).

[alexander-shaposhnikov]

LG

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test

4

nit:

1. --remove-section (I think other tests generally prefer long names)
2. since there are no other prefixes in this file - it looks like one doesn't actually need a custom prefix (ERROR) at all,

i'd use the default one (CHECK)

[mstorsjo]

llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test

4

Fair enough, I'll change that before pushing.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test

2

Could you add a top-level comment to this and the other test saying exactly what this is testing, and how it is testing it (if not obvious from the "what"), please?

llvm/tools/llvm-objcopy/COFF/Writer.cpp

386

Are COFF addresses limited to 32-bits? Or should this really be a 64-bit value?

llvm/tools/llvm-objcopy/COFF/Writer.h

48

I think RVA is more correct for acronyms. Same applies in the .cpp.

[mstorsjo]

llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test

2

Sure, will do.

llvm/tools/llvm-objcopy/COFF/Writer.cpp

386

PECOFF images are essentially limited to 32 bit in size, yes. And the PointerToRawData field that this is going to be stored in in the end is support::ulittle32_t anyway.

llvm/tools/llvm-objcopy/COFF/Writer.h

48

Right, will change.

[mstorsjo]

Added comments to the testcases, changed -R into --remove-section in the tests, changed the variable name Rva into RVA.

[jhenderson]

LGTM, with one request.

llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test

2–4

'##' to distinguish from lit and FileCheck directives, please.

llvm/test/tools/llvm-objcopy/COFF/patch-debug-dir2.test

2–3

Ditto.

[mstorsjo]

llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test

2–4

Thanks, will fix before pushing.

llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test

+## Check that we error out when trying to patch up debug directories that
+## point to data outside of the runtime mapped sections (as we don't try to
+## locate and copy such payloads from the padding areas of the input file).
+
+# RUN: yaml2obj %s -o %t.in.exe
+
+# RUN: not llvm-objcopy --remove-section .rdata %t.in.exe %t.out.exe 2>&1 | FileCheck %s
+
+# CHECK: error: '{{.*}}{{/|\\}}debug-dir-unmapped.test.tmp.out.exe': debug directory payload outside of mapped sections not supported
+
+--- !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: 12288
+    Size:            28
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    VirtualAddress:  4096
+    VirtualSize:     16
+    SectionData:     C3909090909090909090909090909090
+  - Name:            .rdata
+    Characteristics: [  ]
+    VirtualAddress:  8192
+    VirtualSize:     32
+    SectionData:     FFFFFFFF00000000FFFFFFFF00000000
+  - Name:            .buildid
+    Characteristics: [  ]
+    VirtualAddress:  12288
+    VirtualSize:     28
+    SectionData:     0000000042EE405C00000000020000001900000000000000E4070000
+symbols:
+...

llvm/test/tools/llvm-objcopy/COFF/patch-debug-dir2.test

+## Check that we successfully patch the PointerToRawData field in more than
+## one debug directory entry.
+
+# RUN: yaml2obj %s -o %t.in.exe
+
+# RUN: llvm-readobj --coff-debug-directory %t.in.exe | FileCheck %s --check-prefixes=DEBUG-DIRS,DEBUG-DIRS-PRE
+# RUN: llvm-readobj --sections %t.in.exe | FileCheck %s --check-prefixes=SECTIONS,SECTIONS-PRE
+# RUN: llvm-objcopy --remove-section .rdata %t.in.exe %t.out.exe
+# RUN: llvm-readobj --coff-debug-directory %t.out.exe | FileCheck %s --check-prefixes=DEBUG-DIRS,DEBUG-DIRS-POST
+# RUN: llvm-readobj --sections %t.out.exe | FileCheck %s --check-prefixes=SECTIONS,SECTIONS-POST
+
+# DEBUG-DIRS:               AddressOfRawData: 0x3038
+# DEBUG-DIRS-PRE-NEXT:      PointerToRawData: 0x638
+# DEBUG-DIRS-POST-NEXT:     PointerToRawData: 0x438
+
+# DEBUG-DIRS:               AddressOfRawData: 0x3051
+# DEBUG-DIRS-PRE-NEXT:      PointerToRawData: 0x651
+# DEBUG-DIRS-POST-NEXT:     PointerToRawData: 0x451
+
+# SECTIONS:           Name: .buildid
+# SECTIONS-NEXT:      VirtualSize:
+# SECTIONS-NEXT:      VirtualAddress:
+# SECTIONS-NEXT:      RawDataSize:
+# SECTIONS-PRE-NEXT:  PointerToRawData: 0x600
+# SECTIONS-POST-NEXT: PointerToRawData: 0x400
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:       5368709120
+  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: 12288
+    Size:            56
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    VirtualAddress:  4096
+    VirtualSize:     1
+    SectionData:     C3
+  - Name:            .rdata
+    Characteristics: [  ]
+    VirtualAddress:  8192
+    VirtualSize:     32
+    SectionData:     FFFFFFFFFFFFFFFF0000000000000000FFFFFFFFFFFFFFFF0000000000000000
+  - Name:            .buildid
+    Characteristics: [  ]
+    VirtualAddress:  12288
+    VirtualSize:     85
+    SectionData:     0000000046C7A65E00000000020000001900000038300000380600000000000046C7A65E000000001400000004000000513000005106000052534453B3411F5F27A80D2A4C4C44205044422E010000000001000000
+symbols:
+...

llvm/tools/llvm-objcopy/COFF/Writer.h

class COFFWriter {
 
   void writeHeaders(bool IsBigObj);
   void writeSections();
   template <class SymbolTy> void writeSymbolStringTables();
 
   Error write(bool IsBigObj);
 
   Error patchDebugDirectory();
+  Expected<uint32_t> virtualAddressToFileAddress(uint32_t RVA);
 
 public:
   virtual ~COFFWriter() {}
   Error write();
 
   COFFWriter(Object &Obj, Buffer &Buf)
       : Obj(Obj), Buf(Buf), StrTabBuilder(StringTableBuilder::WinCOFF) {}
 };
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_COFF_WRITER_H

llvm/tools/llvm-objcopy/COFF/Writer.cpp

Error COFFWriter::write(bool IsBigObj) {
 
   if (Obj.IsPE)
     if (Error E = patchDebugDirectory())
       return E;
 
   return Buf.commit();
 }
 
+Expected<uint32_t> COFFWriter::virtualAddressToFileAddress(uint32_t RVA) {
+  for (const auto &S : Obj.getSections()) {
+    if (RVA >= S.Header.VirtualAddress &&
+        RVA < S.Header.VirtualAddress + S.Header.SizeOfRawData)
+      return S.Header.PointerToRawData + RVA - S.Header.VirtualAddress;
+  }
+  return createStringError(object_error::parse_failed,
+                           "debug directory payload not found");
+}
+
 // Locate which sections contain the debug directories, iterate over all
 // the debug_directory structs in there, and set the PointerToRawData field
 // in all of them, according to their new physical location in the file.
 Error COFFWriter::patchDebugDirectory() {
   if (Obj.DataDirectories.size() < DEBUG_DIRECTORY)
     return Error::success();
   const data_directory *Dir = &Obj.DataDirectories[DEBUG_DIRECTORY];
   if (Dir->Size <= 0)
     return Error::success();
   for (const auto &S : Obj.getSections()) {
     if (Dir->RelativeVirtualAddress >= S.Header.VirtualAddress &&
         Dir->RelativeVirtualAddress <
             S.Header.VirtualAddress + S.Header.SizeOfRawData) {
       if (Dir->RelativeVirtualAddress + Dir->Size >
           S.Header.VirtualAddress + S.Header.SizeOfRawData)
         return createStringError(object_error::parse_failed,
                                  "debug directory extends past end of section");
 
       size_t Offset = Dir->RelativeVirtualAddress - S.Header.VirtualAddress;
       uint8_t *Ptr = Buf.getBufferStart() + S.Header.PointerToRawData + Offset;
       uint8_t *End = Ptr + Dir->Size;
       while (Ptr < End) {
         debug_directory *Debug = reinterpret_cast<debug_directory *>(Ptr);
-        Debug->PointerToRawData =
-            S.Header.PointerToRawData + Offset + sizeof(debug_directory);
-        Ptr += sizeof(debug_directory) + Debug->SizeOfData;
-        Offset += sizeof(debug_directory) + Debug->SizeOfData;
+        if (!Debug->AddressOfRawData)
+          return createStringError(object_error::parse_failed,
+                                   "debug directory payload outside of "
+                                   "mapped sections not supported");
+        if (Expected<uint32_t> FilePosOrErr =
+                virtualAddressToFileAddress(Debug->AddressOfRawData))
+          Debug->PointerToRawData = *FilePosOrErr;
+        else
+          return FilePosOrErr.takeError();
+        Ptr += sizeof(debug_directory);
+        Offset += sizeof(debug_directory);
       }
       // Debug directory found and patched, all done.
       return Error::success();
     }
   }
   return createStringError(object_error::parse_failed,
                            "debug directory not found");
 }