[llvm-objcopy][COFF] Implement --update-section

[mstorsjo]

This looks sensible to me, thanks!

I think we should let @jhenderson have a look at it still though. I think he's out at the moment, but he's hopefully back soon. (Then again, the 14.x branch is happening soon, and in case he's not back before then, and if you have a need to have this included in the branch, I think it could be ok to go ahead sooner, and deal with other feedback later.)

[MaskRay]

In D118189#3275535, @mstorsjo wrote:

This looks sensible to me, thanks!

I think we should let @jhenderson have a look at it still though. I think he's out at the moment, but he's hopefully back soon. (Then again, the 14.x branch is happening soon, and in case he's not back before then, and if you have a need to have this included in the branch, I think it could be ok to go ahead sooner, and deal with other feedback later.)

Sounds good.

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

11

Use llvm-readobj -S -x .text to test the section header and dump the content.

21

Add a test for an IMAGE_SCN_CNT_UNINITIALIZED_DATA section like .bss. There should be an error similar to ELF SHT_NOBITS

27

Consider adding another section and testing two --update-section

[abrachet]

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

21

This works because the data size is 0. Do you want an explicit check for IMAGE_SCN_CNT_UNINITIALIZED_DATA?

[MaskRay]

ELF has such a diagnostic

% fllvm-objcopy --update-section .bss=xxx a.o b.o
/tmp/RelA/bin/llvm-objcopy: error: 'a.o': section '.bss' can't be updated because it does not have contents

% objcopy --update-section .bss=xxx a.o b.o
objcopy: b.o[.bss]: section has no contents

(I might use cannot instead of "can't" for the ELF diagnostic.)

PE/COFF may need something similar.

[MaskRay]

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

17

Align the values, i.e.

# SMALLER:      Section {
# SMALLER-NEXT:   Number: 1
# SMALLER-NEXT:   Name: .text
# SMALLER-NOT:  }
# SMALLER:        RawDataSize: 4

And use NOT: } to ensure RawDataSize is in the same Section.

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

258

266

269

[MaskRay]

I agree it'd be great to let @jhenderson have a look at it, but seem fine to catch up the 14.x branch and address other issues.

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

38

Hex dump of section '.text': should be aligned, too.

49

This needs a diagnostic.

70

delete unused symbols:

[abrachet]

Thanks for the review @MaskRay

In D118189#3278244, @MaskRay wrote:

I agree it'd be great to let @jhenderson have a look at it, but seem fine to catch up the 14.x branch and address other issues.

Yes I'll wait for James. Theres no rush on my end to get this in before llvm 14.

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

70

Looks like I can't error: missing required key 'symbols'

[jhenderson]

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

17

Just confirming that it is deliberate to keep the section size the same, padded with 0? For ELF, the section is shrunk, which feels more like what I'd expect.

26–27

I have a minor preference for the suggested edit style for flowing over multiple lines.

50

If this is a system error message, there may be a between different OS versions. I believe lit now has substitutions for common error messages to handle this. Look for "%errc_ENOENT" for example usage.

[abrachet]

Change output section size

[abrachet]

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

17

I've changed the size of the section

[jhenderson]

LGTM, but please wait for @MaskRay.

[MaskRay]

Thanks!

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

+# RUN: echo -n AAAB > %t.diff
+# RUN: echo -n AAA > %t.smaller
+# RUN: echo -n AAAAA > %t.larger
+
+# RUN: yaml2obj %s -o %t
+
+# RUN: llvm-objcopy --update-section=.text=%t.diff %t - | llvm-readobj -S -x .text - | FileCheck %s
+# CHECK:        Section {
+# CHECK-NEXT:     Number: 1
+# CHECK-NEXT:     Name: .text
+# CHECK-NOT:    }
+# CHECK:          RawDataSize: 4
+# CHECK:          Hex dump of section '.text':
+# CHECK-NEXT:     0x00000000 41414142 AAAB
+
+# RUN: llvm-objcopy --update-section=.text=%t.smaller %t - | llvm-readobj -S -x .text - | FileCheck %s --check-prefix=SMALLER
+# SMALLER:      Section {
+# SMALLER-NEXT:   Number: 1
+# SMALLER-NEXT:   Name: .text
+# SMALLER-NOT:  }
+# SMALLER:        RawDataSize: 3
+# SMALLER:        Hex dump of section '.text':
+# SMALLER-NEXT:   0x00000000 414141 AAA
+
+# RUN: llvm-objcopy --update-section=.text=%t.diff --update-section=.other=%t.diff %t - | \
+# RUN:   llvm-readobj -S -x .text -x .other - | FileCheck %s --check-prefix=MULTIPLE
+# MULTIPLE:     Section {
+# MULTIPLE-NEXT:  Number: 1
+# MULTIPLE-NEXT:  Name: .text
+# MULTIPLE-NOT: }
+# MULTIPLE:       RawDataSize: 4
+# MULTIPLE:     Section {
+# MULTIPLE-NEXT:  Number: 2
+# MULTIPLE-NEXT:  Name: .other
+# MULTIPLE-NOT: }
+# MULTIPLE:       RawDataSize: 4
+# MULTIPLE:       Hex dump of section '.text':
+# MULTIPLE-NEXT:  0x00000000 41414142 AAAB
+# MULTIPLE:       Hex dump of section '.other':
+# MULTIPLE-NEXT:  0x00000000 41414142 AAAB
+
+# RUN: not llvm-objcopy --update-section=.text=%t.larger %t /dev/null 2>&1 | FileCheck %s --check-prefix=TOO-LARGE
+# TOO-LARGE: error: {{.*}}new section cannot be larger than previous section
+
+# RUN: not llvm-objcopy --update-section=.bss=%t.diff %t /dev/null 2>&1 | FileCheck %s --check-prefix=NO-CONTENTS
+# NO-CONTENTS: error: {{.*}}section '.bss' cannot be updated because it does not have contents
+
+# RUN: not llvm-objcopy --update-section=.text=%t.noexist %t /dev/null 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOENT -DENOENT=%errc_ENOENT
+# NOENT: error: {{.*}}: [[ENOENT]]
+
+# RUN: not llvm-objcopy --update-section=.noexist=%t.diff %t /dev/null 2>&1 | FileCheck %s --check-prefix=NO-SECTION
+# NO-SECTION: error: {{.*}}could not find section with name '.noexist'
+
+--- !COFF
+header:
+  Machine:           IMAGE_FILE_MACHINE_AMD64
+  Characteristics:   [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     '41414141'
+  - Name:            .other
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     '42424242'
+  - Name:            .bss
+    Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA ]
+    SizeOfRawData:   0
+symbols:
+...

llvm/test/tools/llvm-objcopy/ELF/update-section.test

 # ADD-UPDATE:      Name: .added
 # ADD-UPDATE:      Size:
 # ADD-UPDATE-SAME:   {{ }}8{{$}}
 # ADD-UPDATE:      SectionData (
 # ADD-UPDATE-NEXT:   |11113333|
 # ADD-UPDATE:      )
 
 # ERR-NO-SECTION: error: {{.*}}section '.nosection' not found
-# ERR-NOBITS-TYPE: error: {{.*}}section '.nobits_type' can't be updated because it does not have contents
-# ERR-NULL-TYPE: error: {{.*}}section '.null_type' can't be updated because it does not have contents
+# ERR-NOBITS-TYPE: error: {{.*}}section '.nobits_type' cannot be updated because it does not have contents
+# ERR-NULL-TYPE: error: {{.*}}section '.null_type' cannot be updated because it does not have contents
 # ERR-LARGER: error: {{.*}}cannot fit data of size 9 into section '.in_segment' with size 8 that is part of a segment
 
 # MISSING-EQ: error: bad format for --update-section: missing '='
 # MISSING-FILE: error: bad format for --update-section: missing file name

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

for (const auto &Flag : Config.AddSection) {
 
     addSection(
         Obj, SecName,
         makeArrayRef(reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
                      Buf->getBufferSize()),
         Characteristics);
   }
 
+  for (StringRef Flag : Config.UpdateSection) {
+    StringRef SecName, FileName;
+    std::tie(SecName, FileName) = Flag.split('=');
+
+    auto BufOrErr = MemoryBuffer::getFile(FileName);
+    if (!BufOrErr)
+      return createFileError(FileName, errorCodeToError(BufOrErr.getError()));
+    auto Buf = std::move(*BufOrErr);
+
+    auto It = llvm::find_if(Obj.getMutableSections(), [SecName](auto &Sec) {
+      return Sec.Name == SecName;
+    });
+    if (It == Obj.getMutableSections().end())
+      return createStringError(errc::invalid_argument,
+                               "could not find section with name '%s'",
+                               SecName.str().c_str());
+    size_t ContentSize = It->getContents().size();
+    if (!ContentSize)
+      return createStringError(
+          errc::invalid_argument,
+          "section '%s' cannot be updated because it does not have contents",
+          SecName.str().c_str());
+    if (ContentSize < Buf->getBufferSize())
+      return createStringError(
+          errc::invalid_argument,
+          "new section cannot be larger than previous section");
+    It->setOwnedContents({Buf->getBufferStart(), Buf->getBufferEnd()});
+  }
+
   if (!Config.AddGnuDebugLink.empty())
     if (Error E = addGnuDebugLink(Obj, Config.AddGnuDebugLink))
       return E;
 
   if (COFFConfig.Subsystem || COFFConfig.MajorSubsystemVersion ||
       COFFConfig.MinorSubsystemVersion) {
     if (!Obj.IsPE)
       return createStringError(

llvm/tools/llvm-objcopy/COFF/Object.h

struct Section {
   void setContentsRef(ArrayRef<uint8_t> Data) {
     OwnedContents.clear();
     ContentsRef = Data;
   }
 
   void setOwnedContents(std::vector<uint8_t> &&Data) {
     ContentsRef = ArrayRef<uint8_t>();
     OwnedContents = std::move(Data);
+    Header.SizeOfRawData = OwnedContents.size();
   }
 
   void clearContents() {
     ContentsRef = ArrayRef<uint8_t>();
     OwnedContents.clear();
   }
 
 private:

llvm/tools/llvm-objcopy/ELF/Object.cpp

Error Object::updateSection(StringRef Name, ArrayRef<uint8_t> Data) {
   if (It == Sections.end())
     return createStringError(errc::invalid_argument, "section '%s' not found",
                              Name.str().c_str());
 
   auto *OldSec = It->get();
   if (!OldSec->hasContents())
     return createStringError(
         errc::invalid_argument,
-        "section '%s' can't be updated because it does not have contents",
+        "section '%s' cannot be updated because it does not have contents",
         Name.str().c_str());
 
   if (Data.size() > OldSec->Size && OldSec->ParentSegment)
     return createStringError(errc::invalid_argument,
                              "cannot fit data of size %zu into section '%s' "
                              "with size %zu that is part of a segment",
                              Data.size(), Name.str().c_str(), OldSec->Size);