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

Wed Apr 29 01:02:14 PDT 2020

mstorsjo marked 3 inline comments as done.
mstorsjo added inline comments.

================
Comment at: llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test:1
+# RUN: yaml2obj %s -o %t.in.exe
+
----------------
jhenderson wrote:
> 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?
Sure, will do.

================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:386

+Expected<uint32_t> COFFWriter::virtualAddressToFileAddress(uint32_t Rva) {
+  for (const auto &S : Obj.getSections()) {
----------------
jhenderson wrote:
> Are COFF addresses limited to 32-bits? Or should this really be a 64-bit value?
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.

================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.h:48
   Error patchDebugDirectory();
+  Expected<uint32_t> virtualAddressToFileAddress(uint32_t Rva);

----------------
jhenderson wrote:
> I think `RVA` is more correct for acronyms. Same applies in the .cpp.
Right, will change.

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

https://reviews.llvm.org/D78921