This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mstorsjo on Apr 27 2020, 5:54 AM.

Details

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).

Diff Detail

Event Timeline

mstorsjo created this revision.Apr 27 2020, 5:54 AM
This revision is now accepted and ready to land.Apr 28 2020, 2:00 PM
llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test
3

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 marked an inline comment as done.Apr 28 2020, 2:28 PM
mstorsjo added inline comments.
llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test
3

Fair enough, I'll change that before pushing.

jhenderson added inline comments.Apr 29 2020, 12:13 AM
llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test
1

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 marked 3 inline comments as done.Apr 29 2020, 12:45 AM
mstorsjo added inline comments.
llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test
1

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 updated this revision to Diff 260856.Apr 29 2020, 12:57 AM
mstorsjo marked 3 inline comments as done.

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

jhenderson accepted this revision.Apr 29 2020, 1:24 AM

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 marked 3 inline comments as done.Apr 29 2020, 1:27 AM
mstorsjo added inline comments.
llvm/test/tools/llvm-objcopy/COFF/debug-dir-unmapped.test
2–4

Thanks, will fix before pushing.

This revision was automatically updated to reflect the committed changes.
mstorsjo marked an inline comment as done.