[PATCH] D65040: [llvm-objcopy] Add support for --add-section for COFF

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:39
+
+# CHECK-ERR1: llvm-objcopy{{(.exe)?}}: error: '[[FILE]]': '[[FILE1]]': No such file or directory
+
----------------
"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)


================
Comment at: llvm/test/tools/llvm-objcopy/COFF/add-section.test:44
+
+# CHECK-ERR2: llvm-objcopy{{(.exe)?}}: error: '[[FILE]]': bad format for --add-section
+
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65040/new/

https://reviews.llvm.org/D65040