[llvm-objcopy] [COFF] Do not patch debug entries with Repro type

[pirama]

Summary

Fix an edge case missed by https://reviews.llvm.org/D78921. The Repro
debug entry (generated with the /Brepro linker flag) does not have a
debug-directory payload. Do not attempt to patch Debug entries of this
type.

[mstorsjo]

LGTM, thanks!

Do you happen to know if the same is the case for any other debug directory types, or is this the only one?

[pirama]

In D107324#2921407, @mstorsjo wrote:

Do you happen to know if the same is the case for any other debug directory types, or is this the only one?

I'm not sure if this is also the case for other debug directory types. Maybe someone familiar with Windows debug info can help? (+thakis, rnk)

[pirama]

In D107324#2921410, @pirama wrote:

In D107324#2921407, @mstorsjo wrote:

Do you happen to know if the same is the case for any other debug directory types, or is this the only one?

I'm not sure if this is also the case for other debug directory types. Maybe someone familiar with Windows debug info can help? (+thakis, rnk)

Looking at https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#debug-type, we should also exclude IMAGE_DEBUG_TYPE_UNKNOWN. I think all other entries could have a payload associated with it.

[pirama]

Skip IMAGE_DEBUG_TYPE_UNKNOWN debug entries as well.

[mstorsjo]

LGTM, thanks!

I’m considering if it’d be good to change the existing code even further; if the field in the file is zero, regardless of type, we’d just leave it as is, and one error out of its nonzero but can’t be found. WDYT?

[pirama]

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

432

In D107324#2923059, @mstorsjo wrote:

LGTM, thanks!

I’m considering if it’d be good to change the existing code even further; if the field in the file is zero, regardless of type, we’d just leave it as is, and one error out of its nonzero but can’t be found. WDYT?

Are you suggesting we remove the error check here and call virtualAddressToFileAddress only if Debug->AddressOfRawData is not 0?

I considered it but thought I didn't quite understand the connection between the conditional and the error message. I can amend the patch if you think this is a better choice.

[mstorsjo]

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

432

Yes, exactly, I think that's a better/more robust choice - if there's a different directory entry which is payload-less (for one reason or another) we should just ignore it; it's only an error if there's a value which we can't reasonably rebase.

And maybe it'd be even better to check PointerToRawData instead of AddressOfRawData for the main check - if the input file didn't have a raw file address to the payload, then we keep it as is (the virtual address should remain unchanged), but if there's a nonzero PointerToRawData, we need to recalculate that based on the new file layout.

[pirama]

Simplify check and update expected error for one test.

[mstorsjo]

LGTM, thanks! And sorry for the iterative detours while I thought through the issue.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/strip-brepro.test

2 ↗

(On Diff #363881)

Why do you need both -S and -x? As I understand it, you only need one of them to strip the debug data.

Also, this comment needs enhancing to explain why this case is special.

5 ↗

(On Diff #363881)

Shouldn't there be some check after this line to show that it's been stripped?

[mstorsjo]

llvm/test/tools/llvm-objcopy/COFF/strip-brepro.test

5 ↗

(On Diff #363881)

I guess the intent of this test isn't to test stripping in itself, but that llvm-objcopy doesn't bail out while processing an executable with this content in it; the executable as such doesn't seem to contain anything that can be stripped out either. But making this clearer in the test comment would be good I guess.

[pirama]

Rename test, expand comments and remove '-S -x' from RUN line.

[pirama]

llvm/test/tools/llvm-objcopy/COFF/strip-brepro.test

2 ↗

(On Diff #363881)

Why do you need both -S and -x? As I understand it, you only need one of them to strip the debug data.

This is a carry-over from our local build. Neither -S nor -x is needed to reproduce the issue.

5 ↗

(On Diff #363881)

Remove -S, -x. The test is mainly for absence of error but I can add any relevant check on the output if needed.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/debug-entry-no-payload.test

3

"are handled correctly" usually implies that something needs to be checked, in my mind, namely that the debug directory entries look correct after the invocation of llvm-objcopy. It's this that should be checked. If on the other hand, you think it should be just checking for lack of an error, it's probably best to say just that. I'd check the debug entry though.

48

If I'm not mistaken, there are two entries here below. If so, please update this comment.

[pirama]

Update test with revew comments.

[mstorsjo]

Still LGTM, fwiw.

[jhenderson]

LGTM, with two inline nits.

Also super nit: in the llvm-objcopy world, it's one space after '.' :-)

llvm/test/tools/llvm-objcopy/COFF/debug-entry-no-payload.test

14

As there are no other check prefixes used in this file, you can omit the check-prefix argument and just use CHECK instead of DEBUG-DIRS, if you want.

53–54

[pirama]

Address review comments

[pirama]

Thanks for the reviews @mstorsjo @jhenderson.

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
+# CHECK: error: '{{.*}}{{/|\\}}debug-dir-unmapped.test.tmp.out.exe': debug directory payload not found
 
 --- !COFF
 OptionalHeader:
   AddressOfEntryPoint: 4096
   ImageBase:       1073741824
   SectionAlignment: 4096
   FileAlignment:   512
   MajorOperatingSystemVersion: 6

llvm/test/tools/llvm-objcopy/COFF/debug-entry-no-payload.test

+## Check that llvm-objcopy does not produce an error for debug directory
+## entries without a payload (e.g. IMAGE_DEBUG_TYPE_REPRO,
+## IMAGE_DEBUG_TYPE_UNKNOWN) when patching entries that may have moved during
+## a requested operation. Also check that the such debug entries are present
+## in the output.
+
+## The code to patch debug entries is executed even when no operation is
+## requested. Hence the RUN line below invokes llvm-objcopy without any
+## arguments.
+
+# RUN: yaml2obj %s -o %t.in.exe
+# RUN: llvm-objcopy %t.in.exe %t.out.exe
+# RUN: llvm-readobj --coff-debug-directory %t.out.exe | FileCheck %s
+
+# CHECK: Type: Repro (0x10)
+# CHECK: Type: Unknown (0x0)
+
+--- !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:     3
+    SectionData:     31C0C3
+  - Name:            .rdata
+    Characteristics: [  ]
+    VirtualAddress:  8192
+    VirtualSize:     56
+    ## '10' in SectionData below indicates the Repro type for the first debug
+    ## entry.  The second entry has a type of Unknown ('00').
+    SectionData:     0000000047F96B2000000000100000000000000000000000000000000000000047F96B200000000000000000000000000000000000000000
+symbols:         []
+...

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

if (Dir->RelativeVirtualAddress >= S.Header.VirtualAddress &&
                                  "debug directory extends past end of section");
 
       size_t Offset = Dir->RelativeVirtualAddress - S.Header.VirtualAddress;
       uint8_t *Ptr = reinterpret_cast<uint8_t *>(Buf->getBufferStart()) +
                      S.Header.PointerToRawData + Offset;
       uint8_t *End = Ptr + Dir->Size;
       while (Ptr < End) {
         debug_directory *Debug = reinterpret_cast<debug_directory *>(Ptr);
-        if (!Debug->AddressOfRawData)
-          return createStringError(object_error::parse_failed,
-                                   "debug directory payload outside of "
-                                   "mapped sections not supported");
+        if (Debug->PointerToRawData) {
           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,