[PATCH] D57007: [llvm-objcopy] [COFF] Implement --add-gnu-debuglink

[Martin Storsjö via Phabricator via llvm-commits]

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


================
Comment at: test/tools/llvm-objcopy/COFF/add-gnu-debuglink.test:3-4
+
+Using a debuglink filename with a length that is a multiple of 4, to
+showcase padding in CONTENTS below.
+
----------------
jhenderson wrote:
> I know it's not strictly necessary, but it would be nice if comments like this had some sort of leading character, e.g. '#' to make them stand out better.
Ok, will add. (The same goes for a lot of the other tests I've added here as well.)


================
Comment at: test/tools/llvm-objcopy/COFF/add-gnu-debuglink.test:23
+SECTIONS-NEXT:     VirtualSize: 0x2C
+SECTIONS-NEXT:     VirtualAddress: 0x5000
+SECTIONS-NEXT:     RawDataSize: 512
----------------
jhenderson wrote:
> Just to confirm, this new section is supposed to have a virtual address, i.e. it is loaded at runtime?
This is at least how GNU objcopy does it. It's not loaded at runtime (it's got the IMAGE_SCN_MEM_DISCARDABLE flag set which means not loaded), but despite this all sections get a virtual address allocated. IIRC windows is pretty picky with this - all sections need to be consecutive without holes, even the ones that aren't loaded.



================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:38
+    return 0;
+  const Section &Last = Obj.getSections().back();
+  return alignTo(Last.Header.VirtualAddress + Last.Header.VirtualSize,
----------------
jhenderson wrote:
> Are sections required to be in address order?
I believe so, yes: https://docs.microsoft.com/en-us/windows/desktop/debug/pe-format#section-table-section-headers

> In an image file, the VAs for sections must be assigned by the linker so that they are in ascending order and adjacent, and they must be a multiple of the SectionAlignment value in the optional header.


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:52
+
+static void createGnuDebugLinkSectionContents(std::vector<uint8_t> &Data,
+                                              StringRef File) {
----------------
jhenderson wrote:
> Rather than modifying in place via a reference, I think it would be better if this returned the section data.
Sure


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:54
+                                              StringRef File) {
+  auto LinkTargetOrErr = MemoryBuffer::getFile(File);
+  if (!LinkTargetOrErr)
----------------
jhenderson wrote:
> This is probably an instance of too much `auto`.
Ok, will write out the types. (The ELF dir uses `auto` similarly though.)


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:169
+
+  if (!Config.AddGnuDebugLink.empty() && Obj.IsPE)
+    addGnuDebugLink(Obj, Config.AddGnuDebugLink);
----------------
mstorsjo wrote:
> jhenderson wrote:
> > I just want to confirm that GNU objcopy doesn't add a gnu debug link section if it is not PE?
> I'll have a look.
GNU objcopy doesn't make a difference, so I'll remove that condition.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57007