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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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.
+
----------------
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.


================
Comment at: test/tools/llvm-objcopy/COFF/add-gnu-debuglink.test:23
+SECTIONS-NEXT:     VirtualSize: 0x2C
+SECTIONS-NEXT:     VirtualAddress: 0x5000
+SECTIONS-NEXT:     RawDataSize: 512
----------------
Just to confirm, this new section is supposed to have a virtual address, i.e. it is loaded at runtime?


================
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,
----------------
Are sections required to be in address order?


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


================
Comment at: tools/llvm-objcopy/COFF/COFFObjcopy.cpp:54
+                                              StringRef File) {
+  auto LinkTargetOrErr = MemoryBuffer::getFile(File);
+  if (!LinkTargetOrErr)
----------------
This is probably an instance of too much `auto`.


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


================
Comment at: tools/llvm-objcopy/COFF/Object.h:46
+  // is used instead of Contents above.
+  std::vector<uint8_t> LocalContents;
 };
----------------
I'd probably rename this to ModifiedContents or similar. It isn't clear to me what "Local" means in this context.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57007