[llvm-objcopy] [COFF] Implement --add-gnu-debuglink

[jhenderson]

test/tools/llvm-objcopy/COFF/add-gnu-debuglink.test

4–5

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.

24

Just to confirm, this new section is supposed to have a virtual address, i.e. it is loaded at runtime?

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

38

Are sections required to be in address order?

52

Rather than modifying in place via a reference, I think it would be better if this returned the section data.

54

This is probably an instance of too much auto.

170

I just want to confirm that GNU objcopy doesn't add a gnu debug link section if it is not PE?

tools/llvm-objcopy/COFF/Object.h

46

I'd probably rename this to ModifiedContents or similar. It isn't clear to me what "Local" means in this context.

[mstorsjo]

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

170

I'll have a look.

tools/llvm-objcopy/COFF/Object.h

46

Ok - or maybe rename the old one OrigContents instead?

[jhenderson]

tools/llvm-objcopy/COFF/Object.h

46

Either works for me.

[mstorsjo]

test/tools/llvm-objcopy/COFF/add-gnu-debuglink.test

4–5

Ok, will add. (The same goes for a lot of the other tests I've added here as well.)

24

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.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

38

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.

52

Sure

54

Ok, will write out the types. (The ELF dir uses auto similarly though.)

170

GNU objcopy doesn't make a difference, so I'll remove that condition.

[mstorsjo]

Addressed the review comments.

[jhenderson]

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

54

I know, and I've disagreed with it there in some places too!

57

This one CAN be auto, because its type is clear from the type of LinkTargetOrErr.

tools/llvm-objcopy/COFF/Object.h

46

Thinking about this a bit more, I wonder if it would be a good idea for there to be a getContents function that decides which to fetch. This feels like a potential risk point for bugs, if people try to get Contents when it is owned by OrigContents. You'd want to make them both private as a result, and therefore add some setters too.

[mstorsjo]

tools/llvm-objcopy/COFF/Object.h

46

Right, that makes the code much clearer.

[mstorsjo]

Addressed review comments.

[jhenderson]

LGTM.

[jhenderson]

llvm/trunk/tools/llvm-objcopy/COFF/Writer.cpp

292–293 ↗

(On Diff #182877)

I didn't spot this earlier, but why are you padding assuming it is x86?

[mstorsjo]

llvm/trunk/tools/llvm-objcopy/COFF/Writer.cpp

292–293 ↗

(On Diff #182877)

Yeah, ideally we'd use a different padding pattern for other arches. So far, LLD/COFF always uses this padding pattern regardless of architecture (while LLD/ELF uses arch specific paddings), and I'm matching that behaviour for now here. I'd like to bring it up for discussion in LLD first, and once some other paddings are settled on there, I'd implement the same over here.

test/tools/llvm-objcopy/COFF/add-gnu-debuglink.test

+RUN: yaml2obj %p/Inputs/x86_64-exe.yaml > %t.in123.exe
+
+# Using a debuglink filename with a length that is a multiple of 4, to
+# showcase padding in CONTENTS below.
+
+RUN: llvm-objcopy --add-gnu-debuglink=%t.in123.exe %t.in123.exe %t.out.exe
+RUN: llvm-readobj -sections %t.out.exe | FileCheck %s --check-prefix=SECTIONS
+RUN: llvm-objdump -s %t.out.exe | FileCheck %s --check-prefix=CONTENTS
+
+# Show the last of the preexisting sections, which is used for choosing
+# a virtual address for the generated one.
+
+SECTIONS:        Section {
+SECTIONS:          Number: 4
+SECTIONS-NEXT:     Name: .pdata
+SECTIONS-NEXT:     VirtualSize: 0x18
+SECTIONS-NEXT:     VirtualAddress: 0x4000
+SECTIONS-NEXT:     RawDataSize: 512
+SECTIONS:        Section {
+SECTIONS-NEXT:     Number: 5
+SECTIONS-NEXT:     Name: .gnu_debuglink
+SECTIONS-NEXT:     VirtualSize: 0x2C
+SECTIONS-NEXT:     VirtualAddress: 0x5000
+SECTIONS-NEXT:     RawDataSize: 512
+SECTIONS-NEXT:     PointerToRawData:
+SECTIONS-NEXT:     PointerToRelocations:
+SECTIONS-NEXT:     PointerToLineNumbers:
+SECTIONS-NEXT:     RelocationCount:
+SECTIONS-NEXT:     LineNumberCount:
+SECTIONS-NEXT:     Characteristics [ (0x42000040)
+SECTIONS-NEXT:       IMAGE_SCN_CNT_INITIALIZED_DATA (0x40)
+SECTIONS-NEXT:       IMAGE_SCN_MEM_DISCARDABLE (0x2000000)
+SECTIONS-NEXT:       IMAGE_SCN_MEM_READ (0x40000000)
+SECTIONS-NEXT:     ]
+
+# Note: The last 4 bytes here are the crc of the referenced file - if the
+# yaml2obj generated file changes, this crc changes.
+
+CONTENTS: Contents of section .gnu_debuglink:
+CONTENTS:  40005000 6164642d 676e752d 64656275 676c696e  add-gnu-debuglin
+CONTENTS:  40005010 6b2e7465 73742e74 6d702e69 6e313233  k.test.tmp.in123
+CONTENTS:  40005020 2e657865 00000000 7929adc3           .exe

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 #include "Object.h"
 #include "Reader.h"
 #include "Writer.h"
 #include "llvm-objcopy.h"
 
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/JamCRC.h"
+#include "llvm/Support/Path.h"
 #include <cassert>
 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 using namespace object;
 using namespace COFF;
 
 static bool isDebugSection(const Section &Sec) {
   return Sec.Name.startswith(".debug");
 }
 
+static uint64_t getNextRVA(const Object &Obj) {
+  if (Obj.getSections().empty())
+    return 0;
+  const Section &Last = Obj.getSections().back();
+  return alignTo(Last.Header.VirtualAddress + Last.Header.VirtualSize,
+                 Obj.PeHeader.SectionAlignment);
+}
+
+static uint32_t getCRC32(StringRef Data) {
+  JamCRC CRC;
+  CRC.update(ArrayRef<char>(Data.data(), Data.size()));
+  // The CRC32 value needs to be complemented because the JamCRC dosn't
+  // finalize the CRC32 value. It also dosn't negate the initial CRC32 value
+  // but it starts by default at 0xFFFFFFFF which is the complement of zero.
+  return ~CRC.getCRC();
+}
+
+static std::vector<uint8_t> createGnuDebugLinkSectionContents(StringRef File) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> LinkTargetOrErr =
+      MemoryBuffer::getFile(File);
+  if (!LinkTargetOrErr)
+    error("'" + File + "': " + LinkTargetOrErr.getError().message());
+  std::unique_ptr<MemoryBuffer> LinkTarget = std::move(*LinkTargetOrErr);
+  uint32_t CRC32 = getCRC32(LinkTarget->getBuffer());
+
+  StringRef FileName = sys::path::filename(File);
+  size_t CRCPos = alignTo(FileName.size() + 1, 4);
+  std::vector<uint8_t> Data(CRCPos + 4);
+  memcpy(Data.data(), FileName.data(), FileName.size());
+  support::endian::write32le(Data.data() + CRCPos, CRC32);
+  return Data;
+}
+
+static void addGnuDebugLink(Object &Obj, StringRef DebugLinkFile) {
+  uint32_t StartRVA = getNextRVA(Obj);
+
+  std::vector<Section> Sections;
+  Section Sec;
+  Sec.Contents = createGnuDebugLinkSectionContents(DebugLinkFile);
+  Sec.Name = ".gnu_debuglink";
+  Sec.Header.VirtualSize = Sec.Contents.size();
+  Sec.Header.VirtualAddress = StartRVA;
+  Sec.Header.SizeOfRawData =
+      alignTo(Sec.Header.VirtualSize, Obj.PeHeader.FileAlignment);
+  // Sec.Header.PointerToRawData is filled in by the writer.
+  Sec.Header.PointerToRelocations = 0;
+  Sec.Header.PointerToLinenumbers = 0;
+  // Sec.Header.NumberOfRelocations is filled in by the writer.
+  Sec.Header.NumberOfLinenumbers = 0;
+  Sec.Header.Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
+                               IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_DISCARDABLE;
+  Sections.push_back(Sec);
+  Obj.addSections(Sections);
+}
+
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   // Perform the actual section removals.
   Obj.removeSections([&Config](const Section &Sec) {
     // Contrary to --only-keep-debug, --only-section fully removes sections that
     // aren't mentioned.
     if (!Config.OnlySection.empty() &&
         !is_contained(Config.OnlySection, Sec.Name))
       return true;
if (!Sym.Referenced) {
       // but undefined local symbols are kept when --discard-all is set.
       if (Config.DiscardAll && Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC &&
           Sym.Sym.SectionNumber != 0)
         return true;
     }
 
     return false;
   });
+
+  if (!Config.AddGnuDebugLink.empty())
+    addGnuDebugLink(Obj, Config.AddGnuDebugLink);
+
   return Error::success();
 }
 
 void executeObjcopyOnBinary(const CopyConfig &Config,
                             COFFObjectFile &In, Buffer &Out) {
   COFFReader Reader(In);
   Expected<std::unique_ptr<Object>> ObjOrErr = Reader.create();
   if (!ObjOrErr)

tools/llvm-objcopy/COFF/Object.h

struct Relocation {
 
   object::coff_relocation Reloc;
   size_t Target;
   StringRef TargetName; // Used for diagnostics only
 };
 
 struct Section {
   object::coff_section Header;
-  ArrayRef<uint8_t> Contents;
+  ArrayRef<uint8_t> OrigContents;
   std::vector<Relocation> Relocs;
   StringRef Name;
   ssize_t UniqueId;
   size_t Index;
+
+  // A container which actually owns the data. If non-empty, this is used
+  // instead of OrigContents above.
+  std::vector<uint8_t> Contents;
 };
 
 struct Symbol {
   object::coff_symbol32 Sym;
   StringRef Name;
   std::vector<uint8_t> AuxData;
   ssize_t TargetSectionId;
   ssize_t AssociativeComdatTargetSectionId = 0;

tools/llvm-objcopy/COFF/Object.cpp

void Object::removeSections(function_ref<bool(const Section &)> ToRemove) {
   } while (!AssociatedSections.empty());
   updateSections();
   updateSymbols();
 }
 
 void Object::truncateSections(function_ref<bool(const Section &)> ToTruncate) {
   for (Section &Sec : Sections) {
     if (ToTruncate(Sec)) {
-      Sec.Contents = ArrayRef<uint8_t>();
+      Sec.OrigContents = ArrayRef<uint8_t>();
       Sec.Relocs.clear();
       Sec.Header.SizeOfRawData = 0;
+      Sec.Contents.clear();
     }
   }
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

tools/llvm-objcopy/COFF/Reader.cpp

Error COFFReader::readSections(Object &Obj) const {
   // Section indexing starts from 1.
   for (size_t I = 1, E = COFFObj.getNumberOfSections(); I <= E; I++) {
     const coff_section *Sec;
     if (auto EC = COFFObj.getSection(I, Sec))
       return errorCodeToError(EC);
     Sections.push_back(Section());
     Section &S = Sections.back();
     S.Header = *Sec;
-    if (auto EC = COFFObj.getSectionContents(Sec, S.Contents))
+    if (auto EC = COFFObj.getSectionContents(Sec, S.OrigContents))
       return errorCodeToError(EC);
     ArrayRef<coff_relocation> Relocs = COFFObj.getRelocations(Sec);
     for (const coff_relocation &R : Relocs)
       S.Relocs.push_back(R);
     if (auto EC = COFFObj.getSectionName(Sec, S.Name))
       return errorCodeToError(EC);
     if (Sec->hasExtendedRelocations())
       return createStringError(object_error::parse_failed,

tools/llvm-objcopy/COFF/Writer.cpp

for (const auto &S : Obj.getSections()) {
     memcpy(Ptr, &S.Header, sizeof(S.Header));
     Ptr += sizeof(S.Header);
   }
 }
 
 void COFFWriter::writeSections() {
   for (const auto &S : Obj.getSections()) {
     uint8_t *Ptr = Buf.getBufferStart() + S.Header.PointerToRawData;
-    std::copy(S.Contents.begin(), S.Contents.end(), Ptr);
+    ArrayRef<uint8_t> Contents = S.OrigContents;
+    if (!S.Contents.empty())
+      Contents = S.Contents;
+    std::copy(Contents.begin(), Contents.end(), Ptr);
 
     // For executable sections, pad the remainder of the raw data size with
     // 0xcc, which is int3 on x86.
     if ((S.Header.Characteristics & IMAGE_SCN_CNT_CODE) &&
-        S.Header.SizeOfRawData > S.Contents.size())
-      memset(Ptr + S.Contents.size(), 0xcc,
-             S.Header.SizeOfRawData - S.Contents.size());
+        S.Header.SizeOfRawData > Contents.size())
+      memset(Ptr + Contents.size(), 0xcc,
+             S.Header.SizeOfRawData - Contents.size());
 
     Ptr += S.Header.SizeOfRawData;
     for (const auto &R : S.Relocs) {
       memcpy(Ptr, &R.Reloc, sizeof(R.Reloc));
       Ptr += sizeof(R.Reloc);
     }
   }
 }