Fix test for debug dir presence

[alfonsosanchezbeato]

Summary

If the number of directories was 6 (equal to the DEBUG_DIRECTORY
index), patchDebugDirectory() was run even though the debug directory
is actually the 7th entry. Use <= in the comparison to fix that.

This fixes https://llvm.org/PR51243

Author: Alfonso Sánchez-Beato <[e-mail]>

[jhenderson]

Could you add a test case, please, which demonstrates the failure before this fix, and that the fix is correct.

[alfonsosanchezbeato]

I have tried to generate a binary for the test with

--- !COFF
OptionalHeader:
  AddressOfEntryPoint: 4096
  ImageBase:       0
  SectionAlignment: 4096
  FileAlignment:   512
  MajorOperatingSystemVersion: 0
  MinorOperatingSystemVersion: 0
  MajorImageVersion: 0
  MinorImageVersion: 0
  MajorSubsystemVersion: 0
  MinorSubsystemVersion: 0
  Subsystem:       IMAGE_SUBSYSTEM_EFI_APPLICATION
  DLLCharacteristics: [  ]
  SizeOfStackReserve: 0
  SizeOfStackCommit: 0
  SizeOfHeapReserve: 0
  SizeOfHeapCommit: 0
  ExportTable:
    RelativeVirtualAddress: 0
    Size:            0
  ImportTable:
    RelativeVirtualAddress: 0
    Size:            0
  ResourceTable:
    RelativeVirtualAddress: 0
    Size:            0
  ExceptionTable:
    RelativeVirtualAddress: 0
    Size:            0
  CertificateTable:
    RelativeVirtualAddress: 0
    Size:            0
  BaseRelocationTable:
    RelativeVirtualAddress: 0
    Size:            0
header:
  Machine:         IMAGE_FILE_MACHINE_AMD64
  Characteristics: [ ]
sections:
  - Name:            .foo
    Characteristics: [ ]
    Alignment:       4
symbols:
...

However, that generates a binary where all directory entries are filled and NumberOfRvaAndSize is 16, and I need it to be 6 in the binary. Apparently that is fixed in the yaml2obj code, and it is not possible to set NumberOfRvaAndSize in the yaml either. Any suggestions on how can I proceed to obtain a binary for the test?

[jhenderson]

In D106940#2918217, @alfonsosanchezbeato wrote:

However, that generates a binary where all directory entries are filled and NumberOfRvaAndSize is 16, and I need it to be 6 in the binary. Apparently that is fixed in the yaml2obj code, and it is not possible to set NumberOfRvaAndSize in the yaml either. Any suggestions on how can I proceed to obtain a binary for the test?

Probably it requries a yaml2obj change - of the formats supported by yaml2obj, I think COFF has had the least attention, and therefore there are likely features that are missing (in ELF, for example, we can specify overrides for pretty much any value you can find in the object). I suggest reaching out to people who have worked on COFF parts of yaml2obj to see if they'd be able to make a change (or make it yourself, if you're comfortable doing so).

[alfonsosanchezbeato]

I have hand-crafted a binary that exposed the bug. Something to note here is that without the fix there is a very clear access to uninitialized memory when Obj.DataDirectories.size()==DEBUG_DIRECTORY, as we access after the check to Obj.DataDirectories[DEBUG_DIRECTORY].

This implies that the bug is not always reproducible, and also that we might get random crashes, depending on the memory content.

[jhenderson]

The pre-merge checks are saying that this new test fails...

I'm not too familiar with yaml2obj's COFF version, but it might be good for you to take a quick look at what would be involved to add support for the new field you're proposing, rather than checking in a pre-canned binary. If it's not too complex, that would be the preferred approach.

llvm/test/tools/llvm-objcopy/COFF/check-debug-dir-present.test

4–6 ↗

(On Diff #365387)

CHECK is the default.

8 ↗

(On Diff #365387)

[alfonsosanchezbeato]

In D106940#2946447, @jhenderson wrote:

The pre-merge checks are saying that this new test fails...

I think that the problem is that the binary has not been properly imported, as the error is "The file was not recognized as a valid object file". I generated the diff with git show HEAD --binary -U999999, maybe the diff needs to be generated in a different way so phabricator generates a good binary?

I'm not too familiar with yaml2obj's COFF version, but it might be good for you to take a quick look at what would be involved to add support for the new field you're proposing, rather than checking in a pre-canned binary. If it's not too complex, that would be the preferred approach.

I am not familiar with yaml2obj either, I've taken a look and I'm not sure how easy would it be to add something like that.

[jhenderson]

I've skimmed the yaml2obj code and identified what I think will allow you to make a small change to support an arbitrary value for NumberOfRvaAndSize. The value is set in initializeOptionalHeader, in COFFEmitter.cpp to be COFF::NUM_DATA_DIRECTORIES + 1. It would be trivial to set this to a different value, derived from the input header. You'd need to provide a new Optional member in the PEHeader struct in COFFYAML.h, which would be populated at YAML parse time in MappingTraits<COFFYAML::PEHeader>::mapping and provide an override for the default value set in initializeOptionalHeader, if specified.

Take a look and see if you can get that to work. I think it'll be fairly straightforward.

[alfonsosanchezbeato]

I've created a new diff for the yaml2obj/obj2yaml changes: https://reviews.llvm.org/D108825

[alfonsosanchezbeato]

Updated, now that https://reviews.llvm.org/D108825 has been merged.

[jhenderson]

LGTM. Do you plan on making more LLVM contributions in the future? If so, given you've now got a couple of commits landed in your name successfully (I'm assuming the yaml2obj one did land successfully), I think you can request commit access for yourself. Otherwise, let me know if you'd like me to commit on your behalf.

llvm/test/tools/llvm-objcopy/COFF/check-debug-dir-present.test

5 ↗

(On Diff #371590)

As said already, CHECK is the default check prefix and so doesn't need specifying (as long as no other --check-prefix/--check-prefixes commands are used in that invocation).

[alfonsosanchezbeato]

Remove the unneeded --check-prefix option.

[alfonsosanchezbeato]

In D106940#2993657, @jhenderson wrote:

LGTM. Do you plan on making more LLVM contributions in the future? If so, given you've now got a couple of commits landed in your name successfully (I'm assuming the yaml2obj one did land successfully), I think you can request commit access for yourself. Otherwise, let me know if you'd like me to commit on your behalf.

The patches were for fixing a concrete problem we had while manipulating PE/COFF binaries, so I do not expect to make many more contributions in the near future. Please go ahead and commit on my behalf. Thank you!

[jhenderson]

Okay, I've pushed this. As before, please keep an eye out for any failure notifications.

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

Expected<uint32_t> COFFWriter::virtualAddressToFileAddress(uint32_t RVA) {
   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)
+  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) {