[llvm-objcopy] Improve --add-section argument string parsing

[grimar]

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

44 ↗

(On Diff #212008)

If you do not report the file name anymore then you do not need -DFILE=%t

49 ↗

(On Diff #212008)

The same.

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

41 ↗

(On Diff #212008)

Perhaps DFILE1 and DFILE2 would be better when you have 2 files.

I'd also suggest renaming CHECK-ERR* -> ERR*.

53 ↗

(On Diff #212008)

FWIW I'd omit llvm-objcopy{{(.exe)?}}: part here and above. It doesn't seem useful actually to test.

[sdmitriev]

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

44 ↗

(On Diff #212008)

Right.

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

41 ↗

(On Diff #212008)

Done.

53 ↗

(On Diff #212008)

Done.

[abrachet]

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

41 ↗

(On Diff #212008)

Hi @sdmitriev there is a done check box in the top right of an inline comment that is generally preferred to responding done. That way others can us the 'Hide "done" Inlines" option and it's also less noise.

[jhenderson]

LGTM. Thanks!

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

 # 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=%t -DFILE1=%t2 %s --check-prefixes=CHECK-ERR1
+# RUN: not llvm-objcopy --add-section=.another.section=%t2 %t %t3 2>&1 | FileCheck -DFILE1=%t -DFILE2=%t2 %s --check-prefixes=ERR1
 
-# CHECK-ERR1: llvm-objcopy{{(.exe)?}}: error: '[[FILE]]': '[[FILE1]]': {{[Nn]}}o such file or directory
+# ERR1: error: '[[FILE1]]': '[[FILE2]]': {{[Nn]}}o 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
+# RUN: not llvm-objcopy --add-section=.another.section %t %t3 2>&1 | FileCheck %s --check-prefixes=ERR2
 
-# CHECK-ERR2: llvm-objcopy{{(.exe)?}}: error: '[[FILE]]': bad format for --add-section
+# 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
+
+# ERR3: error: bad format for --add-section: missing file name
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: [  ]
 sections:
   - Name:            .text
     Characteristics: [  ]
     Alignment:       4
     SectionData:     488B0500000000C3
 symbols:
 ...

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

 # CHECK: Name: .test3
 # CHECK: Name: .symtab
 # CHECK: Name: .strtab
 # CHECK: Name: .shstrtab
 # CHECK: Name: .test2
 # CHECK: 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 %s --check-prefixes=ERR1
+
+# ERR1: error: '[[FILE1]]': '[[FILE2]]': {{[Nn]}}o such file or directory
+
+## 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/trunk/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

Obj.removeSymbols([&](const Symbol &Sym) {
 
     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()),

llvm/trunk/tools/llvm-objcopy/CopyConfig.cpp

Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   }
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_remove_section))
     Config.ToRemove.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_section))
     Config.KeepSection.emplace_back(Arg->getValue(), UseRegex);
   for (auto Arg : InputArgs.filtered(OBJCOPY_only_section))
     Config.OnlySection.emplace_back(Arg->getValue(), UseRegex);
-  for (auto Arg : InputArgs.filtered(OBJCOPY_add_section))
-    Config.AddSection.push_back(Arg->getValue());
+  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);
+  }
   for (auto Arg : InputArgs.filtered(OBJCOPY_dump_section))
     Config.DumpSection.push_back(Arg->getValue());
   Config.StripAll = InputArgs.hasArg(OBJCOPY_strip_all);
   Config.StripAllGNU = InputArgs.hasArg(OBJCOPY_strip_all_gnu);
   Config.StripDebug = InputArgs.hasArg(OBJCOPY_strip_debug);
   Config.StripDWO = InputArgs.hasArg(OBJCOPY_strip_dwo);
   Config.StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);