[llvm-objcopy] Add support for --add-section for COFF

[sdmitriev]

Summary

This patch enables support for --add-section=... option for COFF objects.

[jhenderson]

Please make sure to update the documentation at llvm/docs/CommandGuide/llvm-objcopy.rst to move this switch out of the ELF-specific directory.

Please add a test case for trying to add a section with contents from a non-existent file. Also a test case where the argument format is invalid, e.g. --add-section foo (i.e. no '=' sign).

llvm/test/tools/llvm-objcopy/COFF/add-section.test

7–13

Nit: It would be a bit easier to read if these had some extra spaces to make the whole lot line up with the block below. Also, the Characteristics block being indented further seems weird to me. I think the following would look better:

# CHECK:      Name: .test.section
# CHECK:      Characteristics [
# CHECK-NEXT:   IMAGE_SCN_ALIGN_1BYTES
# CHECK-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
# CHECK-NEXT: ]

17

Same comment applies here as above.

27–33

I suspect you can delete all of this. Let's keep the YAML to the minimum required for this test.

34–37

You can probably delete this and most other pre-existing sections.

46

I'm guessing you don't need these symbols for this test case.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

186

Rather than having a big block of code that is broadly similar to the addGnuDebugLink code, I wonder if some of the body of this loop and that function could be factored out into a function, which takes an Object, some section contents, and maybe one or two other parameters to do with addresses and adds a section to that Object.

[sdmitriev]

Addressed review comments.

[sdmitriev]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

186

I have refactored this code into an addSection function. Please review revised changes.

[jhenderson]

Thanks. Just a few minor things mostly in the test left.

llvm/test/tools/llvm-objcopy/COFF/add-section.test

4

A number of newer tests in the tools use '##' to distinguish comments from lit/FileCheck commands. Please update this and the other comments accordingly.

Also "objcopy" -> "llvm-objcopy", "adds section" -> "adds a section".

9

"if file" -> "if the file"

14

This isn't quite testing what it should be testing, since the command-line parser is interpreting %t2 as a positional argument. You should add just remove %t2 from your command-line here, and then you should get the error for the bad format of --add-section.

16

I find it easier to follow the test if the FileCheck CHECK lines are immediately following the test case they are used for, so it would look something like:

# RUN: ...

# CHECK1: ...
# CHECK1: ...

# RUN: ...

# CHECK2: ...
# CHECK2: ...

27

Can you use FileCheck's -D option here to check the file name in the error message?

RUN: ... FileCheck -DFile=%t2

CHECK: ... error: [[FILE]] ...

39

Does yaml2obj allow this line to be deleted?

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

95

You can delete the empty lines before and after this line. Also, what is the type of Contents here? LLVM style is to not use auto in this sort of situation, as the type is not obvious from context.

[sdmitriev]

Additional changes to address review comments.

[sdmitriev]

llvm/test/tools/llvm-objcopy/COFF/add-section.test

39

No, 'symbols' is a required keyword.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

95

Sure, I have deleted empty lines. 'Contents' has type std::vector<uint8_t> since it is a result of the createGnuDebugLinkSectionContents() function call which is defined several lines above, but, anyway, I have replaced auto with explicit type.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/add-section.test

30

Use the FileCheck -D option here too instead of {{.+}}

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

189

Does COFF allow for empty section names? If it does, you need to distinguish the case where there is no '=' at all in the parameter from it producing an empty string, i.e. "--add-sections==foo.txt" might be allowable if COFF allows empty section names (but not "--add-section=foo.txt".

[sdmitriev]

Another update to address review comments.

[sdmitriev]

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

189

I do not see anything in the COFF spec that would disallow sections with empty names. But, as it turned out, such sections were not handled correctly by the COFF/Writer, so I had to make a small change there to fix the problem. I have also added an additional test to the lit test that tests adding a section with empty name.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/add-section.test

20

Nit: "with an empty name"

44

What does the "{{.+}}" here (and in the case above) capture? I'm not sure they're useful?

[jhenderson]

I realised that the --add-section argument string should probably be parsed in CopyConfig.cpp rather than in ELFObjcopy.cpp and COFFObjcopy.cpp, since the syntax is the same. As such, the error handling should happen there too. It looks like we're missing this test case in the ELF tests, which is why the problem with the file name in the error message wasn't picked up earlier.

Are you happy moving the parsing out of the COFF and ELF-specific bits? In this case, the CopyConfig struct should probably contain a pair of strings - the file name and the new section name, rather than a single string. An ELF-side test-case should probably be added for the missing file too.

llvm/test/tools/llvm-objcopy/COFF/add-section.test

40

"No such file" -> "{{[Nn]}}o such file"

(double check this against other tests, but if I remember rightly, the message case depends on the platform)

45

Sorry, I should have realised this earlier: it doesn't make sense for the error message to include the file name because the error is in the command-line parameters in this case.

See my out-of-line comment for more details.

[sdmitriev]

Sure, I can move --add-section argument string parsing out of ELFObjcopy.cpp and COFFObjcopy.cpp to the CopyConfig.cpp and add necessary test cases. I just suggest to do it as a followup change set instead of doing as part of this patch.

[jhenderson]

In D65040#1601441, @sdmitriev wrote:

Sure, I can move --add-section argument string parsing out of ELFObjcopy.cpp and COFFObjcopy.cpp to the CopyConfig.cpp and add necessary test cases. I just suggest to do it as a followup change set instead of doing as part of this patch.

Okay, sounds good. LGTM.

llvm/docs/CommandGuide/llvm-objcopy.rst

 
 The following options are either agnostic of the file format, or apply to
 multiple file formats.
 
 .. option:: --add-gnu-debuglink <debug-file>
 
  Add a .gnu_debuglink section for ``<debug-file>`` to the output.
 
+.. option:: --add-section <section=file>
+
+ Add a section named ``<section>`` with the contents of ``<file>`` to the
+ output. For ELF objects the section will be of type `SHT_NOTE`, if the name
+ starts with ".note". Otherwise, it will have type `SHT_PROGBITS`. Can be
+ specified multiple times to add multiple sections.
+
 .. option:: --disable-deterministic-archives, -U
 
  Use real values for UIDs, GIDs and timestamps when updating archive member
  headers.
 
 .. option:: --discard-all, -x
 
  Remove most local symbols from the output. Different file formats may limit

 
 ELF-SPECIFIC OPTIONS
 --------------------
 
 The following options are implemented only for ELF objects. If used with other
 objects, :program:`llvm-objcopy` will either emit an error or silently ignore
 them.
 
-.. option:: --add-section <section=file>
-
- Add a section named ``<section>`` with the contents of ``<file>`` to the
- output. The section will be of type `SHT_NOTE`, if the name starts with
- ".note". Otherwise, it will have type `SHT_PROGBITS`. Can be specified multiple
- times to add multiple sections.
-
 .. option:: --add-symbol <name>=[<section>:]<value>[,<flags>]
 
  Add a new symbol called ``<name>`` to the output symbol table, in the section
  named ``<section>``, with value ``<value>``. If ``<section>`` is not specified,
  the symbol is added as an absolute symbol. The ``<flags>`` affect the symbol
  properties. Accepted values are:
 
  - `global` = the symbol will have global binding.

llvm/test/tools/llvm-objcopy/COFF/add-section.test

+# RUN: yaml2obj %s > %t
+
+## Test that llvm-objcopy adds a section to the given object with expected
+## contents.
+# RUN: echo DEADBEEF > %t.sec
+# RUN: llvm-objcopy --add-section=.test.section=%t.sec %t %t1
+# RUN: llvm-readobj --file-headers --sections --section-data %t1 | FileCheck %s --check-prefixes=CHECK-ADD
+
+# CHECK-ADD:      SectionCount: 2
+# CHECK-ADD:      Name: .text
+# CHECK-ADD:      Name: .test.section
+# CHECK-ADD:      Characteristics [
+# CHECK-ADD-NEXT:   IMAGE_SCN_ALIGN_1BYTES
+# CHECK-ADD-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
+# CHECK-ADD-NEXT: ]
+# CHECK-ADD:      SectionData (
+# CHECK-ADD-NEXT:  0000: {{.+}}|DEADBEEF{{.+}}|
+# CHECK-ADD-NEXT: )
+
+## Test that llvm-objcopy can add a section with empty name.
+# RUN: llvm-objcopy --add-section==%t.sec %t %t1.empty.name
+# RUN: llvm-readobj --file-headers --sections --section-data %t1.empty.name | FileCheck %s --check-prefixes=CHECK-ADD-EMPTY-NAME
+
+# CHECK-ADD-EMPTY-NAME:      SectionCount: 2
+# CHECK-ADD-EMPTY-NAME:      Name: .text
+# CHECK-ADD-EMPTY-NAME:      Name: (00 00 00 00 00 00 00 00)
+# CHECK-ADD-EMPTY-NAME:      Characteristics [
+# CHECK-ADD-EMPTY-NAME-NEXT:   IMAGE_SCN_ALIGN_1BYTES
+# CHECK-ADD-EMPTY-NAME-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
+# CHECK-ADD-EMPTY-NAME-NEXT: ]
+# CHECK-ADD-EMPTY-NAME:      SectionData (
+# CHECK-ADD-EMPTY-NAME-NEXT:  0000: {{.+}}|DEADBEEF{{.+}}|
+# CHECK-ADD-EMPTY-NAME-NEXT: )
+
+## Test that llvm-objcopy produces an error if the file with section contents
+## to be added does not exist.
+# RUN: not llvm-objcopy --add-section=.another.section=%t2 %t %t3 2>&1 | FileCheck -DFILE=%t2 %s --check-prefixes=CHECK-ERR1
+
+# CHECK-ERR1: llvm-objcopy{{(.exe)?}}: error:{{.+}}[[FILE]]{{.+}} No such file or directory
+
+## Another negative test for invalid --add-sections command line argument.
+# RUN: not llvm-objcopy --add-section=.another.section %t %t3 2>&1 | FileCheck -DFILE=%t %s --check-prefixes=CHECK-ERR2
+
+# CHECK-ERR2: llvm-objcopy{{(.exe)?}}: error:{{.+}}[[FILE]]{{.+}} bad format for --add-section
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     488B0500000000C3
+symbols:
+...

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

static std::vector<uint8_t> createGnuDebugLinkSectionContents(StringRef File) {
   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);
+// Adds named section with given contents to the object.
+static void addSection(Object &Obj, StringRef Name, ArrayRef<uint8_t> Contents,
+                       uint32_t Characteristics) {
+  bool NeedVA = Characteristics & (IMAGE_SCN_MEM_EXECUTE | IMAGE_SCN_MEM_READ |
+                                   IMAGE_SCN_MEM_WRITE);
 
-  std::vector<Section> Sections;
   Section Sec;
-  Sec.setOwnedContents(createGnuDebugLinkSectionContents(DebugLinkFile));
-  Sec.Name = ".gnu_debuglink";
-  Sec.Header.VirtualSize = Sec.getContents().size();
-  Sec.Header.VirtualAddress = StartRVA;
-  Sec.Header.SizeOfRawData = alignTo(Sec.Header.VirtualSize,
-                                     Obj.IsPE ? Obj.PeHeader.FileAlignment : 1);
+  Sec.setOwnedContents(Contents);
+  Sec.Name = Name;
+  Sec.Header.VirtualSize = NeedVA ? Sec.getContents().size() : 0u;
+  Sec.Header.VirtualAddress = NeedVA ? getNextRVA(Obj) : 0u;
+  Sec.Header.SizeOfRawData =
+      NeedVA ? alignTo(Sec.Header.VirtualSize,
+                       Obj.IsPE ? Obj.PeHeader.FileAlignment : 1)
+             : Sec.getContents().size();
   // 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);
+  Sec.Header.Characteristics = Characteristics;
+
+  Obj.addSections(Sec);
+}
+
+static void addGnuDebugLink(Object &Obj, StringRef DebugLinkFile) {
+  std::vector<uint8_t> Contents =
+      createGnuDebugLinkSectionContents(DebugLinkFile);
+  addSection(Obj, ".gnu_debuglink", Contents,
+             IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
+                 IMAGE_SCN_MEM_DISCARDABLE);
 }
 
 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() &&
if (!Sym.Referenced) {
           Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC &&
           Sym.Sym.SectionNumber != 0)
         return true;
     }
 
     return false;
   });
 
+  for (const auto &Flag : Config.AddSection) {
+    StringRef SecName, FileName;
+    std::tie(SecName, FileName) = Flag.split("=");
+
+    if (FileName.empty())
+      return createStringError(llvm::errc::invalid_argument,
+                               "bad format for --add-section");
+    auto BufOrErr = MemoryBuffer::getFile(FileName);
+    if (!BufOrErr)
+      return createFileError(FileName, errorCodeToError(BufOrErr.getError()));
+    auto Buf = std::move(*BufOrErr);
+
+    addSection(
+        Obj, SecName,
+        makeArrayRef(reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
+                     Buf->getBufferSize()),
+        IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_ALIGN_1BYTES);
+  }
+
   if (!Config.AddGnuDebugLink.empty())
     addGnuDebugLink(Obj, Config.AddGnuDebugLink);
 
   if (Config.AllowBrokenLinks || !Config.BuildIdLinkDir.empty() ||
       Config.BuildIdLinkInput || Config.BuildIdLinkOutput ||
       !Config.SplitDWO.empty() || !Config.SymbolsPrefix.empty() ||
-      !Config.AllocSectionsPrefix.empty() || !Config.AddSection.empty() ||
-      !Config.DumpSection.empty() || !Config.KeepSection.empty() ||
-      !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
-      !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
-      !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
-      !Config.SetSectionFlags.empty() || !Config.SymbolsToRename.empty() ||
-      Config.ExtractDWO || Config.KeepFileSymbols || Config.LocalizeHidden ||
-      Config.PreserveDates || Config.StripDWO || Config.StripNonAlloc ||
-      Config.StripSections || Config.Weaken || Config.DecompressDebugSections ||
+      !Config.AllocSectionsPrefix.empty() || !Config.DumpSection.empty() ||
+      !Config.KeepSection.empty() || !Config.SymbolsToGlobalize.empty() ||
+      !Config.SymbolsToKeep.empty() || !Config.SymbolsToLocalize.empty() ||
+      !Config.SymbolsToWeaken.empty() || !Config.SymbolsToKeepGlobal.empty() ||
+      !Config.SectionsToRename.empty() || !Config.SetSectionFlags.empty() ||
+      !Config.SymbolsToRename.empty() || Config.ExtractDWO ||
+      Config.KeepFileSymbols || Config.LocalizeHidden || Config.PreserveDates ||
+      Config.StripDWO || Config.StripNonAlloc || Config.StripSections ||
+      Config.Weaken || Config.DecompressDebugSections ||
       Config.DiscardMode == DiscardType::Locals ||
       !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for COFF");
   }
 
   return Error::success();
 }

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

size_t COFFWriter::finalizeStringTable() {
 
   for (const auto &S : Obj.getSymbols())
     if (S.Name.size() > COFF::NameSize)
       StrTabBuilder.add(S.Name);
 
   StrTabBuilder.finalize();
 
   for (auto &S : Obj.getMutableSections()) {
-    if (S.Name.size() > COFF::NameSize) {
-      memset(S.Header.Name, 0, sizeof(S.Header.Name));
+    if (S.Name.size() > COFF::NameSize) {
       snprintf(S.Header.Name, sizeof(S.Header.Name), "/%d",
                (int)StrTabBuilder.getOffset(S.Name));
     } else {
-      strncpy(S.Header.Name, S.Name.data(), COFF::NameSize);
+      memcpy(S.Header.Name, S.Name.data(), S.Name.size());
     }
   }
   for (auto &S : Obj.getMutableSymbols()) {
     if (S.Name.size() > COFF::NameSize) {
       S.Sym.Name.Offset.Zeroes = 0;
       S.Sym.Name.Offset.Offset = StrTabBuilder.getOffset(S.Name);
     } else {
       strncpy(S.Sym.Name.ShortName, S.Name.data(), COFF::NameSize);