[objcopy] Refactor CommonConfig to add posibility to specify added/updated sections as MemoryBuffer.

[avl]

Summary

Current objcopy implementation has a possibility to add or update sections.
The incoming section is specified as a pair: section name and name of the file
containing section data. The interface does not allow to specify incoming
section as a memory buffer. This patch adds possibility to specify incoming
section as a memory buffer.

[jhenderson]

llvm/include/llvm/ObjCopy/CommonConfig.h

193

Take the Buffer by R-value reference, since you are releasing its contents.

194

Use an initialiser list.

199

Why shared_ptr?

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

62–64

Here and throughout your test changes, please remove the unused FILE1 variable, and rename FILE2 to FILE (or make equivalent changes).

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

876–880

It would be great if you could pull all this logic into a function for sharing with the update section case. I think the only differences are the error messages and where the final data gets put.

llvm/unittests/ObjCopy/ObjCopyTest.cpp

120

Split this into separate Add and Update test cases, to avoid confusing the two functionalities.

As the fact that the logic is independent of whether it is a debug section or not, remove reference to "debug" and use arbitrary section contents below (e.g. ".foo" with content "1234" or whatever), rather than a real debug section.

Why only an ELF case?

[avl]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

There are many cases where CommonConfig and ConfigManager are copied using "copy" semantic. unique_ptr should be copied with "move" semantic. if unique_ptr would be used here then we need to write strange copy operator:

NewSectionInfo& operator=(const NewSectionInfo& Other) {
   SectionData = std::move(const_cast<NewSectionInfo*>(&Other)->SectionData);
}

[jhenderson]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

There are many cases where CommonConfig and ConfigManager are copied using "copy" semantic

Do they need to copy though? It seems like a reference should be enough.

[avl]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

There are many cases where CommonConfig and ConfigManager are copied using "copy" semantic

Do they need to copy though? It seems like a reference should be enough.

parseStripOptions fills DC.CopyConfigs by copying of initial Config :

    for (StringRef Filename : Positional) {
...
      Config.InputFilename = Filename;
      Config.OutputFilename = Filename;
      DC.CopyConfigs.push_back(ConfigMgr);

"move" semantic could not be used here.

[jhenderson]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

Okay. I'm wondering whether we could change the structure of how that works to allow a core config that has everything except the filenames in, and then that can be taken by reference, or some similar design.

However, actually, I think it would just be simpler to implement a copy constructor that makes a copy of the memory buffer - you already have code that does this in this patch as it is.

[avl]

addressed comments.

[jhenderson]

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

563

llvm/unittests/ObjCopy/ObjCopyTest.cpp

123

I have a marginal prefernce to make Add an enum, as it helps give a bit more meaning to the parameter.

163

170

Add a break at the end of the if.

174–179

These can all be EXPECT rather than ASSERT.

[avl]

addressed comments.

[jhenderson]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

However, actually, I think it would just be simpler to implement a copy constructor that makes a copy of the memory buffer - you already have code that does this in this patch as it is.

Are you planning on looking at this? I still think you shouldn't have a shared_ptr.

llvm/unittests/ObjCopy/ObjCopyTest.cpp

132–133

These two should be ASSERT, since the test will crash if the object isn't valid.

162–163

Ditto.

[avl]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

However, actually, I think it would just be simpler to implement a copy constructor that makes a copy of the memory buffer - you already have code that does this in this patch as it is.

Are you planning on looking at this? I still think you shouldn't have a shared_ptr.

Oh, sorry, I misunderstood the comment.

With shared_ptr we do not need to copy of the memory buffer. For unique_ptr we need to copy of the memory buffer. But then we might have some performance degradation. Would it be OK?

Why using shared_ptr is a problem?

[jhenderson]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

shared_ptr is almost always a design smell in my experience. The majority of the CopyConfig isn't shared (it's copied), so why should one part of it be? Furthermore, if somebody wanted to update the SectionData for one particular config in their library, but use the original for all the others, they'd end up changing it for all of them unintentionally.

[avl]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

shared_ptr is almost always a design smell in my experience. The majority of the CopyConfig isn't shared (it's copied), so why should one part of it be?

There are other parts of CopyConfig which are shared:

class NameOrPattern {
  StringRef Name;
  // Regex is shared between multiple CommonConfig instances.
  std::shared_ptr<Regex> R;
  std::shared_ptr<GlobPattern> G;

The reason to not copy part of CommonConfig related to Add/Update sections might be intention to avoid copying big piece of data. All other parts of CommonConfig do not require a big storage.

Furthermore, if somebody wanted to update the SectionData for one particular config in their library, but use the original for all the others, they'd end up changing it for all of them unintentionally.

My understanding is that shared part of data is not supposed to be changed. It Is MemoryBuffer which keeps "const char*". If there would be changed particular SectionData member in the particular config - then it would be perfectly fine and it would not affect others.

If someone would cast away const-ness of MemoryBuffer then it seems to be his responsibility to understand the details.

[jhenderson]

llvm/include/llvm/ObjCopy/CommonConfig.h

199

Okay, thanks for the explanation, your comments make sense.

[avl]

addressed comments(returned ASSERT_TRUE back)

[jhenderson]

LGTM!

[avl]

Thank you for the review!

[nickdesaulniers]

So it looks like this commit regressed some functionality we were using in Android's Linux kernel builds.

It looks like with GNU objcopy, can you specify both --dump-section= then --add-section= in one command line invocation with the same file in order to move a section.
https://android.googlesource.com/kernel/common/+/refs/heads/android14-5.15/arch/arm64/Makefile.postlink#21

$ echo 'void x(void) {}` > foo.c
$ clang -c foo.c
$ rm -f foo.text && llvm-objcopy --dump-section=.text=foo.text --add-section=.hello=foo.text foo.o
llvm-objcopy: error: 'foo.text': No such file or directory

I think I can move us to using --rename-section instead of dump then add, but just a heads up in case this regresses others' builds, too.

[MaskRay]

It seems that the change was for llvm-dwarfutil which needs the MemoryBuffer functionality.
The patch changed --add-section to run in twp steps, one before --dump-section (load the file content) and one after (add a section).

In general I wish that users do not expect a particular order between two operations. The order is often unclear/undocumented.

llvm/include/llvm/ObjCopy/CommonConfig.h

 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Support/GlobPattern.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Regex.h"
 // Necessary for llvm::DebugCompressionType::None
 #include "llvm/Target/TargetOptions.h"
 #include <vector>
 
 namespace llvm {
 namespace objcopy {
 

 struct NewSymbolInfo {
   StringRef SymbolName;
   StringRef SectionName;
   uint64_t Value = 0;
   std::vector<SymbolFlag> Flags;
   std::vector<StringRef> BeforeSyms;
 };
 
+// Specify section name and section body for newly added or updated section.
+struct NewSectionInfo {
+  NewSectionInfo() = default;
+  NewSectionInfo(StringRef Name, std::unique_ptr<MemoryBuffer> &&Buffer)
+      : SectionName(Name), SectionData(std::move(Buffer)) {}
+
+  StringRef SectionName;
+  std::shared_ptr<MemoryBuffer> SectionData;
+};
+
 // Configuration for copying/stripping a single file.
 struct CommonConfig {
   // Main input/output options
   StringRef InputFilename;
   FileFormat InputFormat = FileFormat::Unspecified;
   StringRef OutputFilename;
   FileFormat OutputFormat = FileFormat::Unspecified;
 
   // Only applicable when --output-format!=binary (e.g. elf64-x86-64).
   Optional<MachineInfo> OutputArch;
 
   // Advanced options
   StringRef AddGnuDebugLink;
   // Cached gnu_debuglink's target CRC
   uint32_t GnuDebugLinkCRC32;
   Optional<StringRef> ExtractPartition;
   StringRef SplitDWO;
   StringRef SymbolsPrefix;
   StringRef AllocSectionsPrefix;
   DiscardType DiscardMode = DiscardType::None;
 
   // Repeated options
-  std::vector<StringRef> AddSection;
+  std::vector<NewSectionInfo> AddSection;
   std::vector<StringRef> DumpSection;
-  std::vector<StringRef> UpdateSection;
+  std::vector<NewSectionInfo> UpdateSection;
 
   // Section matchers
   NameMatcher KeepSection;
   NameMatcher OnlySection;
   NameMatcher ToRemove;
 
   // Symbol matchers
   NameMatcher SymbolsToGlobalize;

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

static Error handleArgs(const CommonConfig &Config,
   if (!Config.SetSectionFlags.empty())
     for (Section &Sec : Obj.getMutableSections()) {
       const auto It = Config.SetSectionFlags.find(Sec.Name);
       if (It != Config.SetSectionFlags.end())
         Sec.Header.Characteristics = flagsToCharacteristics(
             It->second.NewFlags, Sec.Header.Characteristics);
     }
 
-  for (const auto &Flag : Config.AddSection) {
-    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);
-
+  for (const NewSectionInfo &NewSection : Config.AddSection) {
     uint32_t Characteristics;
-    const auto It = Config.SetSectionFlags.find(SecName);
+    const auto It = Config.SetSectionFlags.find(NewSection.SectionName);
     if (It != Config.SetSectionFlags.end())
       Characteristics = flagsToCharacteristics(It->second.NewFlags, 0);
     else
       Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_ALIGN_1BYTES;
 
-    addSection(
-        Obj, SecName,
-        makeArrayRef(reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
-                     Buf->getBufferSize()),
+    addSection(Obj, NewSection.SectionName,
+               makeArrayRef(reinterpret_cast<const uint8_t *>(
+                                NewSection.SectionData->getBufferStart()),
+                            NewSection.SectionData->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;
+  for (const NewSectionInfo &NewSection : Config.UpdateSection) {
+    auto It = llvm::find_if(Obj.getMutableSections(), [&](auto &Sec) {
+      return Sec.Name == NewSection.SectionName;
     });
     if (It == Obj.getMutableSections().end())
       return createStringError(errc::invalid_argument,
                                "could not find section with name '%s'",
-                               SecName.str().c_str());
+                               NewSection.SectionName.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())
+          NewSection.SectionName.str().c_str());
+    if (ContentSize < NewSection.SectionData->getBufferSize())
       return createStringError(
           errc::invalid_argument,
           "new section cannot be larger than previous section");
-    It->setOwnedContents({Buf->getBufferStart(), Buf->getBufferEnd()});
+    It->setOwnedContents({NewSection.SectionData->getBufferStart(),
+                          NewSection.SectionData->getBufferEnd()});
   }
 
   if (!Config.AddGnuDebugLink.empty())
     if (Error E = addGnuDebugLink(Obj, Config.AddGnuDebugLink))
       return E;
 
   if (COFFConfig.Subsystem || COFFConfig.MajorSubsystemVersion ||
       COFFConfig.MinorSubsystemVersion) {

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

for (SymbolFlag FlagValue : SymInfo.Flags)
     };
 
   Obj.SymbolTable->addSymbol(
       SymInfo.SymbolName, Bind, Type, Sec, Value, Visibility,
       Sec ? (uint16_t)SYMBOL_SIMPLE_INDEX : (uint16_t)SHN_ABS, 0);
 }
 
 static Error
-handleUserSection(StringRef Flag,
+handleUserSection(const NewSectionInfo &NewSection,
                   function_ref<Error(StringRef, ArrayRef<uint8_t>)> F) {
-  std::pair<StringRef, StringRef> SecPair = Flag.split("=");
-  StringRef SecName = SecPair.first;
-  StringRef File = SecPair.second;
-  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr = MemoryBuffer::getFile(File);
-  if (!BufOrErr)
-    return createFileError(File, errorCodeToError(BufOrErr.getError()));
-  std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
-  ArrayRef<uint8_t> Data(
-      reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
-      Buf->getBufferSize());
-  return F(SecName, Data);
+  ArrayRef<uint8_t> Data(reinterpret_cast<const uint8_t *>(
+                             NewSection.SectionData->getBufferStart()),
+                         NewSection.SectionData->getBufferSize());
+  return F(NewSection.SectionName, Data);
 }
 
 // This function handles the high level operations of GNU objcopy including
 // handling command line options. It's important to outline certain properties
 // we expect to hold of the command line operations. Any operation that "keeps"
 // should keep regardless of a remove. Additionally any removal should respect
 // any previous removals. Lastly whether or not something is removed shouldn't
 // depend a) on the order the options occur in or b) on some opaque priority
if (!Config.SetSectionAlignment.empty()) {
     }
   }
 
   if (Config.OnlyKeepDebug)
     for (auto &Sec : Obj.sections())
       if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE)
         Sec.Type = SHT_NOBITS;
 
-  for (const auto &Flag : Config.AddSection) {
+  for (const NewSectionInfo &AddedSection : Config.AddSection) {
     auto AddSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
       OwnedDataSection &NewSection =
           Obj.addSection<OwnedDataSection>(Name, Data);
       if (Name.startswith(".note") && Name != ".note.GNU-stack")
         NewSection.Type = SHT_NOTE;
       return Error::success();
     };
-    if (Error E = handleUserSection(Flag, AddSection))
+    if (Error E = handleUserSection(AddedSection, AddSection))
       return E;
   }
 
-  for (StringRef Flag : Config.UpdateSection) {
+  for (const NewSectionInfo &NewSection : Config.UpdateSection) {
     auto UpdateSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
       return Obj.updateSection(Name, Data);
     };
-    if (Error E = handleUserSection(Flag, UpdateSection))
+    if (Error E = handleUserSection(NewSection, UpdateSection))
       return E;
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
                                         Config.GnuDebugLinkCRC32);
 
   // If the symbol table was previously removed, we need to create a new one

llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp

static void updateAndRemoveSymbols(const CommonConfig &Config,
                                    const MachOConfig &MachOConfig,
                                    Object &Obj) {
   for (SymbolEntry &Sym : Obj.SymTable) {
     auto I = Config.SymbolsToRename.find(Sym.Name);
     if (I != Config.SymbolsToRename.end())
       Sym.Name = std::string(I->getValue());
   }
 
-  auto RemovePred = [Config, MachOConfig,
+  auto RemovePred = [&Config, &MachOConfig,
                      &Obj](const std::unique_ptr<SymbolEntry> &N) {
     if (N->Referenced)
       return false;
     if (MachOConfig.KeepUndefined && N->isUndefinedSymbol())
       return false;
     if (N->n_desc & MachO::REFERENCED_DYNAMICALLY)
       return false;
     if (Config.StripAll)
for (const std::unique_ptr<Section> &Sec : LC.Sections) {
         return Error::success();
       }
     }
 
   return createStringError(object_error::parse_failed, "section '%s' not found",
                            SecName.str().c_str());
 }
 
-static Error addSection(StringRef SecName, StringRef Filename, Object &Obj) {
-  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
-      MemoryBuffer::getFile(Filename);
-  if (!BufOrErr)
-    return createFileError(Filename, errorCodeToError(BufOrErr.getError()));
-  std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
-
-  std::pair<StringRef, StringRef> Pair = SecName.split(',');
+static Error addSection(const NewSectionInfo &NewSection, Object &Obj) {
+  std::pair<StringRef, StringRef> Pair = NewSection.SectionName.split(',');
   StringRef TargetSegName = Pair.first;
   Section Sec(TargetSegName, Pair.second);
-  Sec.Content = Obj.NewSectionsContents.save(Buf->getBuffer());
+  Sec.Content =
+      Obj.NewSectionsContents.save(NewSection.SectionData->getBuffer());
   Sec.Size = Sec.Content.size();
 
   // Add the a section into an existing segment.
   for (LoadCommand &LC : Obj.LoadCommands) {
     Optional<StringRef> SegName = LC.getSegmentName();
     if (SegName && SegName == TargetSegName) {
       uint64_t Addr = *LC.getSegmentVMAddr();
       for (const std::unique_ptr<Section> &S : LC.Sections)
if (FoundSec == FoundSeg->Sections.end())
     return createStringError(errc::invalid_argument,
                              "could not find section with name '%s'",
                              SecName.str().c_str());
 
   assert(FoundSec->get()->CanonicalName == (SegName + "," + SecName).str());
   return *FoundSec->get();
 }
 
-static Error updateSection(StringRef SecName, StringRef Filename, Object &O) {
-  Expected<Section &> SecToUpdateOrErr = findSection(SecName, O);
+static Error updateSection(const NewSectionInfo &NewSection, Object &O) {
+  Expected<Section &> SecToUpdateOrErr = findSection(NewSection.SectionName, O);
 
   if (!SecToUpdateOrErr)
     return SecToUpdateOrErr.takeError();
   Section &Sec = *SecToUpdateOrErr;
 
-  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
-      MemoryBuffer::getFile(Filename);
-  if (!BufOrErr)
-    return createFileError(Filename, errorCodeToError(BufOrErr.getError()));
-  std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
-
-  if (Buf->getBufferSize() > Sec.Size)
+  if (NewSection.SectionData->getBufferSize() > Sec.Size)
     return createStringError(
         errc::invalid_argument,
         "new section cannot be larger than previous section");
-  Sec.Content = O.NewSectionsContents.save(Buf->getBuffer());
+  Sec.Content = O.NewSectionsContents.save(NewSection.SectionData->getBuffer());
   Sec.Size = Sec.Content.size();
   return Error::success();
 }
 
 // isValidMachOCannonicalName returns success if Name is a MachO cannonical name
 // ("<segment>,<section>") and lengths of both segment and section names are
 // valid.
 static Error isValidMachOCannonicalName(StringRef Name) {
static Error handleArgs(const CommonConfig &Config,
 
   updateAndRemoveSymbols(Config, MachOConfig, Obj);
 
   if (Config.StripAll)
     for (LoadCommand &LC : Obj.LoadCommands)
       for (std::unique_ptr<Section> &Sec : LC.Sections)
         Sec->Relocations.clear();
 
-  for (const auto &Flag : Config.AddSection) {
-    std::pair<StringRef, StringRef> SecPair = Flag.split("=");
-    StringRef SecName = SecPair.first;
-    StringRef File = SecPair.second;
-    if (Error E = isValidMachOCannonicalName(SecName))
+  for (const NewSectionInfo &NewSection : Config.AddSection) {
+    if (Error E = isValidMachOCannonicalName(NewSection.SectionName))
       return E;
-    if (Error E = addSection(SecName, File, Obj))
+    if (Error E = addSection(NewSection, Obj))
       return E;
   }
 
-  for (const auto &Flag : Config.UpdateSection) {
-    StringRef SectionName;
-    StringRef FileName;
-    std::tie(SectionName, FileName) = Flag.split('=');
-    if (Error E = isValidMachOCannonicalName(SectionName))
+  for (const NewSectionInfo &NewSection : Config.UpdateSection) {
+    if (Error E = isValidMachOCannonicalName(NewSection.SectionName))
       return E;
-    if (Error E = updateSection(SectionName, FileName, Obj))
+    if (Error E = updateSection(NewSection, Obj))
       return E;
   }
 
   if (Error E = processLoadCommands(MachOConfig, Obj))
     return E;
 
   return Error::success();
 }

llvm/lib/ObjCopy/wasm/WasmObjcopy.cpp

for (StringRef Flag : Config.DumpSection) {
     StringRef FileName;
     std::tie(SecName, FileName) = Flag.split("=");
     if (Error E = dumpSectionToFile(SecName, FileName, Obj))
       return createFileError(FileName, std::move(E));
   }
 
   removeSections(Config, Obj);
 
-  for (StringRef Flag : Config.AddSection) {
-    StringRef SecName, FileName;
-    std::tie(SecName, FileName) = Flag.split("=");
-    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
-        MemoryBuffer::getFile(FileName);
-    if (!BufOrErr)
-      return createFileError(FileName, errorCodeToError(BufOrErr.getError()));
+  for (const NewSectionInfo &NewSection : Config.AddSection) {
     Section Sec;
     Sec.SectionType = llvm::wasm::WASM_SEC_CUSTOM;
-    Sec.Name = SecName;
-    std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
+    Sec.Name = NewSection.SectionName;
+
+    std::unique_ptr<MemoryBuffer> BufferCopy = MemoryBuffer::getMemBufferCopy(
+        NewSection.SectionData->getBufferStart(),
+        NewSection.SectionData->getBufferIdentifier());
     Sec.Contents = makeArrayRef<uint8_t>(
-        reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
-        Buf->getBufferSize());
-    Obj.addSectionWithOwnedContents(Sec, std::move(Buf));
+        reinterpret_cast<const uint8_t *>(BufferCopy->getBufferStart()),
+        BufferCopy->getBufferSize());
+
+    Obj.addSectionWithOwnedContents(Sec, std::move(BufferCopy));
   }
 
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CommonConfig &Config, const WasmConfig &,
                              object::WasmObjectFile &In, raw_ostream &Out) {
   Reader TheReader(In);

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

 # CHECK-EXTENDED-RELOCS-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
 # CHECK-EXTENDED-RELOCS-NEXT: ]
 # CHECK-EXTENDED-RELOCS:      SectionData (
 # CHECK-EXTENDED-RELOCS-NEXT:  0000: {{.+}}|DEADBEEF{{.+}}|
 # CHECK-EXTENDED-RELOCS-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 -DFILE1=%t -DFILE2=%t2 -DMSG=%errc_ENOENT %s --check-prefixes=ERR1
+# RUN: not llvm-objcopy --add-section=.another.section=%t2 %t %t3 2>&1 | FileCheck -DFILE=%t2 -DMSG=%errc_ENOENT %s --check-prefixes=ERR1
 
-# ERR1: error: '[[FILE1]]': '[[FILE2]]': [[MSG]]
+# ERR1: error: '[[FILE]]': [[MSG]]
 
 ## Another negative test for invalid --add-sections command line argument.
 # RUN: not llvm-objcopy --add-section=.another.section %t %t3 2>&1 | FileCheck %s --check-prefixes=ERR2
 
 # ERR2: error: bad format for --add-section: missing '='
 
 ## Negative test for invalid --add-sections argument - missing file name.
 # RUN: not llvm-objcopy --add-section=.section.name= %t %t3 2>&1 | FileCheck %s --check-prefixes=ERR3

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

 # CHECK-NEXT: AddressAlignment: 1
 # CHECK-NEXT: EntrySize: 0
 # CHECK-NEXT: SectionData (
 # CHECK-NEXT:   0000: DEADBEEF
 # CHECK-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=.section.name=%t.missing %t %t.out 2>&1 | FileCheck -DFILE1=%t -DFILE2=%t.missing -DMSG=%errc_ENOENT %s --check-prefixes=ERR1
+# RUN: not llvm-objcopy --add-section=.section.name=%t.missing %t %t.out 2>&1 | FileCheck -DFILE=%t.missing -DMSG=%errc_ENOENT %s --check-prefixes=ERR1
 
-# ERR1: error: '[[FILE1]]': '[[FILE2]]': [[MSG]]
+# ERR1: error: '[[FILE]]': [[MSG]]
 
 ## Negative test for invalid --add-sections argument - missing '='.
 # RUN: not llvm-objcopy --add-section=.section.name %t %t.out 2>&1 | FileCheck %s --check-prefixes=ERR2
 
 # ERR2: error: bad format for --add-section: missing '='
 
 ## Negative test for invalid --add-sections argument - missing file name.
 # RUN: not llvm-objcopy --add-section=.section.name= %t %t.out 2>&1 | FileCheck %s --check-prefixes=ERR3
 
 # ERR3: error: bad format for --add-section: missing file name

llvm/test/tools/llvm-objcopy/MachO/add-section-error.test

 ## Test --add-section error messages.
 
 # RUN: yaml2obj %p/Inputs/x86_64.yaml -o %t
 # RUN: echo -n abcdefg > %t.data
 
 ## Error case 1: Nonexistent input file is specified by --add-section.
 # RUN: not llvm-objcopy --add-section __TEXT,__text=%t.missing %t %t.nonexistent-file 2>&1 \
 # RUN:   | FileCheck %s -DINPUT=%t -DSECTION_DATA_FILE=%t.missing -DMSG=%errc_ENOENT --check-prefix=NONEXSITENT-FILE
-# NONEXSITENT-FILE: error: '[[INPUT]]': '[[SECTION_DATA_FILE]]': [[MSG]]
+# NONEXSITENT-FILE: error: '[[SECTION_DATA_FILE]]': [[MSG]]
 
 ## Error case 2: Too long segment name.
 # RUN: not llvm-objcopy --add-section __TOOOOOOOOO_LONG,__text=%t.data %t %t.too-long-seg-name 2>&1 \
 # RUN:   | FileCheck %s -DINPUT=%t --check-prefix=TOO-LONG-SEG-NAME
 # TOO-LONG-SEG-NAME: error: '[[INPUT]]': too long segment name: '__TOOOOOOOOO_LONG'

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

 # RUN: not llvm-objcopy --update-section __TEXT,__noexist=%t.diff %t /dev/null 2>&1 | FileCheck %s --check-prefix=NO-SECTION
 # NO-SECTION: error: {{.*}}could not find section with name '__noexist'
 
 # RUN: yaml2obj --docnum=2 %s -o %t
 
 # RUN: llvm-objcopy --update-section __TEXT,__text=%t.diff %t - | obj2yaml | FileCheck %s --check-prefix=FULL-SECNAME
 # FULL-SECNAME: content: '41414142'
 
-# RUN: not llvm-objcopy --update-section __text=%t.dff %t /dev/null 2>&1 | FileCheck %s --check-prefix=NON-CANONICAL-SECNAME
+# RUN: not llvm-objcopy --update-section __text=%t.diff %t /dev/null 2>&1 | FileCheck %s --check-prefix=NON-CANONICAL-SECNAME
 # NON-CANONICAL-SECNAME: error: {{.*}}invalid section name '__text' (should be formatted as '<segment name>,<section name>')
 
 --- !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x00000003
   filetype:        0x00000001

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

if (!UnsupportedFlags.empty())
     return createStringError(errc::invalid_argument,
                              "unsupported flag%s for --add-symbol: '%s'",
                              UnsupportedFlags.size() > 1 ? "s" : "",
                              join(UnsupportedFlags, "', '").c_str());
 
   return SI;
 }
 
+// Parse input option \p ArgValue and load section data. This function
+// extracts section name and name of the file keeping section data from
+// ArgValue, loads data from the file, and stores section name and data
+// into the vector of new sections \p NewSections.
+static Error loadNewSectionData(StringRef ArgValue, StringRef OptionName,
+                                std::vector<NewSectionInfo> &NewSections) {
+  if (!ArgValue.contains('='))
+    return createStringError(errc::invalid_argument,
+                             "bad format for " + OptionName + ": missing '='");
+
+  std::pair<StringRef, StringRef> SecPair = ArgValue.split("=");
+  if (SecPair.second.empty())
+    return createStringError(errc::invalid_argument, "bad format for " +
+                                                         OptionName +
+                                                         ": missing file name");
+
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+      MemoryBuffer::getFile(SecPair.second);
+  if (!BufOrErr)
+    return createFileError(SecPair.second,
+                           errorCodeToError(BufOrErr.getError()));
+
+  NewSections.push_back({SecPair.first, std::move(*BufOrErr)});
+  return Error::success();
+}
+
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig>
 objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
                              function_ref<Error(Error)> ErrorCallback) {
   DriverConfig DC;
   ObjcopyOptTable T;
objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_section))
     if (Error E = Config.KeepSection.addMatcher(NameOrPattern::create(
             Arg->getValue(), SectionMatchStyle, ErrorCallback)))
       return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_only_section))
     if (Error E = Config.OnlySection.addMatcher(NameOrPattern::create(
             Arg->getValue(), SectionMatchStyle, ErrorCallback)))
       return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_add_section)) {
-    StringRef ArgValue(Arg->getValue());
-    if (!ArgValue.contains('='))
-      return createStringError(errc::invalid_argument,
-                               "bad format for --add-section: missing '='");
-    if (ArgValue.split("=").second.empty())
-      return createStringError(
-          errc::invalid_argument,
-          "bad format for --add-section: missing file name");
-    Config.AddSection.push_back(ArgValue);
+    if (Error Err = loadNewSectionData(Arg->getValue(), "--add-section",
+                                       Config.AddSection))
+      return std::move(Err);
   }
   for (auto Arg : InputArgs.filtered(OBJCOPY_update_section)) {
-    StringRef ArgValue(Arg->getValue());
-    if (!ArgValue.contains('='))
-      return createStringError(errc::invalid_argument,
-                               "bad format for --update-section: missing '='");
-    if (ArgValue.split("=").second.empty())
-      return createStringError(
-          errc::invalid_argument,
-          "bad format for --update-section: missing file name");
-    Config.UpdateSection.push_back(ArgValue);
+    if (Error Err = loadNewSectionData(Arg->getValue(), "--update-section",
+                                       Config.UpdateSection))
+      return std::move(Err);
   }
   for (auto *Arg : InputArgs.filtered(OBJCOPY_dump_section)) {
     StringRef Value(Arg->getValue());
     if (Value.split('=').second.empty())
       return createStringError(
           errc::invalid_argument,
           "bad format for --dump-section, expected section=file");
     Config.DumpSection.push_back(Value);

llvm/unittests/ObjCopy/ObjCopyTest.cpp

copySimpleInMemoryFileImpl(
       R"(
 --- !WASM
 FileHeader:
   Version:         0x00000001
 ...
 )",
       [](const Binary &File) { return File.isWasm(); });
 }
+
+enum Action : uint8_t { AddSection, UpdateSection };
+
+void addOrUpdateSectionToFileImpl(
+    const char *YamlCreationString,
+    std::function<bool(const Binary &File)> IsValidFormat,
+    StringRef NewSectionName, StringRef NewSectionData, Action SectionAction) {
+  auto ErrHandler = [&](const Twine &Msg) { FAIL() << "Error: " << Msg; };
+
+  // Create Object file from YAML description.
+  SmallVector<char> Storage;
+  std::unique_ptr<ObjectFile> Obj =
+      yaml2ObjectFile(Storage, YamlCreationString, ErrHandler);
+  ASSERT_TRUE(Obj);
+  ASSERT_TRUE(IsValidFormat(*Obj));
+
+  std::unique_ptr<MemoryBuffer> NewSectionBuffer =
+      MemoryBuffer::getMemBuffer(NewSectionData, NewSectionName, false);
+  std::string Name;
+  if (Obj->isMachO())
+    Name = "__TEXT," + NewSectionName.str();
+  else
+    Name = NewSectionName.str();
+
+  ConfigManager Config;
+  Config.Common.OutputFilename = "a.out";
+  if (SectionAction == AddSection)
+    Config.Common.AddSection.push_back({Name, std::move(NewSectionBuffer)});
+  else
+    Config.Common.UpdateSection.push_back({Name, std::move(NewSectionBuffer)});
+
+  // Call executeObjcopyOnBinary()
+  SmallVector<char> DataVector;
+  raw_svector_ostream OutStream(DataVector);
+  Error Err = objcopy::executeObjcopyOnBinary(Config, *Obj.get(), OutStream);
+  ASSERT_FALSE(std::move(Err));
+
+  MemoryBufferRef Buffer(StringRef(DataVector.data(), DataVector.size()),
+                         Config.Common.OutputFilename);
+
+  // Check copied file.
+  Expected<std::unique_ptr<Binary>> Result = createBinary(Buffer);
+  ASSERT_THAT_EXPECTED(Result, Succeeded());
+  ASSERT_TRUE(IsValidFormat(**Result));
+  ASSERT_TRUE((*Result)->isObject());
+
+  // Check that copied file has the new section.
+  bool HasNewSection = false;
+  for (const object::SectionRef &Sect :
+       static_cast<ObjectFile *>((*Result).get())->sections()) {
+    Expected<StringRef> SectNameOrErr = Sect.getName();
+    ASSERT_THAT_EXPECTED(SectNameOrErr, Succeeded());
+
+    if (*SectNameOrErr == NewSectionName) {
+      HasNewSection = true;
+      Expected<StringRef> SectionData = Sect.getContents();
+      ASSERT_THAT_EXPECTED(SectionData, Succeeded());
+      EXPECT_TRUE(Sect.getSize() == NewSectionData.size());
+      EXPECT_TRUE(memcmp(SectionData->data(), NewSectionData.data(),
+                         NewSectionData.size()) == 0);
+      break;
+    }
+  }
+  EXPECT_TRUE(HasNewSection);
+}
+
+TEST(AddSection, COFF) {
+  SCOPED_TRACE("addSectionToFileCOFF");
+
+  addOrUpdateSectionToFileImpl(
+      R"(
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     E800000000C3C3C3
+symbols:
+...
+)",
+      [](const Binary &File) { return File.isCOFF(); }, ".foo", "1234",
+      AddSection);
+}
+
+TEST(AddSection, ELF) {
+  SCOPED_TRACE("addSectionToFileELF");
+
+  addOrUpdateSectionToFileImpl(
+      R"(
+--- !ELF
+FileHeader:
+   Class:    ELFCLASS64
+   Data:     ELFDATA2LSB
+   Type:     ET_REL)",
+      [](const Binary &File) { return File.isELF(); }, ".foo", "1234",
+      AddSection);
+}
+
+TEST(AddSection, MachO) {
+  SCOPED_TRACE("addSectionToFileMachO");
+
+  addOrUpdateSectionToFileImpl(
+      R"(
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x80000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      152
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         __TEXT
+    vmaddr:          0
+    vmsize:          4
+    fileoff:         184
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          184
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+...
+)",
+      [](const Binary &File) { return File.isMachO(); }, "__foo", "1234",
+      AddSection);
+}
+
+TEST(AddSection, Wasm) {
+  SCOPED_TRACE("addSectionToFileWasm");
+
+  addOrUpdateSectionToFileImpl(
+      R"(
+--- !WASM
+FileHeader:
+  Version:         0x00000001
+...
+)",
+      [](const Binary &File) { return File.isWasm(); }, ".foo", "1234",
+      AddSection);
+}
+
+TEST(UpdateSection, COFF) {
+  SCOPED_TRACE("updateSectionToFileCOFF");
+
+  addOrUpdateSectionToFileImpl(
+      R"(
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .foo
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     E800000000C3C3C3
+symbols:
+...
+)",
+      [](const Binary &File) { return File.isCOFF(); }, ".foo", "1234",
+      UpdateSection);
+}
+
+TEST(UpdateSection, ELF) {
+  SCOPED_TRACE("updateSectionToFileELF");
+
+  addOrUpdateSectionToFileImpl(
+      R"(
+--- !ELF
+FileHeader:
+  Class:    ELFCLASS64
+  Data:     ELFDATA2LSB
+  Type:     ET_REL
+Sections:
+  - Name:            .foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Content:        "12345678"
+)",
+      [](const Binary &File) { return File.isELF(); }, ".foo", "1234",
+      UpdateSection);
+}
+
+TEST(UpdateSection, MachO) {
+  SCOPED_TRACE("updateSectionToFileMachO");
+
+  addOrUpdateSectionToFileImpl(
+      R"(
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x80000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      152
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         __TEXT
+    vmaddr:          0
+    vmsize:          4
+    fileoff:         184
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __foo
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          184
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+...
+)",
+      [](const Binary &File) { return File.isMachO(); }, "__foo", "1234",
+      UpdateSection);
+}