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

Tue Aug 3 14:08:10 PDT 2021

mstorsjo added inline comments.

================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:432
+            Debug->Type != COFF::IMAGE_DEBUG_TYPE_REPRO) {
+          if (!Debug->AddressOfRawData)
+            return createStringError(object_error::parse_failed,
----------------
pirama wrote:
> >>! 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107324/new/

https://reviews.llvm.org/D107324