[llvm-objcopy] Add support for '--' for delimiting options from input/output files

[sdmitriev]

Summary

This will allow to use llvm-objcopy with file names that begin with dashes.

[jhenderson]

I'm not against the functional change, but why have so many tests changed? You should have a single dedicated test that covers this, rather than arbitrarily modifying existing tests to cover the new behaviour. As this is not file format specific, it can even be put in the top-level llvm-objcopy directory, although you may want a comment saying it is testing generic behaviour, since it'll presumably need to use e.g. an ELF object to avoid the "no input file" error.

Is there a particular reason you've singled out llvm-objcopy, and not llvm-strip?

llvm/tools/llvm-objcopy/CopyConfig.cpp

478

Nit: fix this clang-tidy issue.

489

What about the case where a user does llvm-objcopy -- with no arguments or input files? What does GNU objcopy do in this case?

[sdmitriev]

I agree that it would be better to create a single LIT test for a file name beginning with dash, but it is not clear how to write it. I assume such test would require use of %T (directory of a %t), but according to the documentation it is deprecated now. So, I decided to follow the same strategy for testing this patch as was used in a similar llvm-rc change (D56743) - updating few arbitrary tests to use ‘--’ separator for the file names. If you have ideas how to write a test for a file name beginning with ‘-‘, please share.

Is there a particular reason you've singled out llvm-objcopy, and not llvm-strip?

No, no particular reason. I only needed this to be supported in llvm-objcopy, so I’ve changed this tool only. But I can prepare a similar change for llvm-strip as well in a separate patch.

llvm/tools/llvm-objcopy/CopyConfig.cpp

489

GNU objcopy prints help in this case. I have updated this change to match GNU objcopy behavior:

llvm-objcopy -- => print help
llvm-objcopy -- <file> => empty output

[jhenderson]

In D102665#2765456, @sdmitriev wrote:

I agree that it would be better to create a single LIT test for a file name beginning with dash, but it is not clear how to write it. I assume such test would require use of %T (directory of a %t), but according to the documentation it is deprecated now. So, I decided to follow the same strategy for testing this patch as was used in a similar llvm-rc change (D56743) - updating few arbitrary tests to use ‘--’ separator for the file names. If you have ideas how to write a test for a file name beginning with ‘-‘, please share.

You can use something like mkdir %t in tests (see various cases throughout the testsuites) and then cd into that directory, where you can then use relative names, e.g. to files starting with -, without neeing %t to be involved.

Is there a particular reason you've singled out llvm-objcopy, and not llvm-strip?

No, no particular reason. I only needed this to be supported in llvm-objcopy, so I’ve changed this tool only. But I can prepare a similar change for llvm-strip as well in a separate patch.

Sounds reasonable.

llvm/tools/llvm-objcopy/CopyConfig.cpp

478

Actually, auto doesn't really gain us anything here, as the return type is not obvious from the single line here. Just make the type explicit.

[sdmitriev]

Addressed review comments.

[jhenderson]

llvm/test/tools/llvm-objcopy/objcopy-dash-dash.test

1 ↗

(On Diff #346106)

Nit: in newer llvm-objcopy tests, we tend to use ## for comment markers, to make the comments stand out clearly from the test directives.

Also, no need for "objcopy" to be in the name. dash-dash.test would be sufficient. Since the functionality for llvm-strip would be nearly identical, it could live in the same test.

4–5 ↗

(On Diff #346106)

I would change the commnet slightly to make it generic - after all, non-dashed files are still valid after this point too. Something like:

"Check that llvm-objcopy correctly treats paths after "--" as file paths, even if they start with dashes."

You would then have a case where the name has a dash as a leading character, and another where it doesn't. I'd make the filenames with a leading dash valid options too.

Also, I recommend you test the following cases:

1. -- is specified with options but no input files.
2. -- is specified with input files but no options.

8 ↗

(On Diff #346106)

You don't need to check the full file properties. In fact, I'd recommend doing a run without -- and then use cmp to compare this with the output of the different test cases.

10–19 ↗

(On Diff #346106)

This can be dramatically simplified. We don't need a section at all, and I believe the Machine is optional too. You can also remove excess padding, and add the --- to properly start the YAML.

[sdmitriev]

Addressed review comments.

[jhenderson]

LGTM, with one suggestion.

llvm/test/tools/llvm-objcopy/dash-dash.test

13

I'd put the -- before test-obj here. That way, you have coverage for all positional arguments being after the --. It also removes the potential for positional arguments either side of the -- treating the first as an option (in terms of whether the lack of options is an issue).

[sdmitriev]

Applied code review suggestions.

[hubert.reinterpretcast]

llvm/test/tools/llvm-objcopy/dash-dash.test

10

This is causing a bot failure:
https://lab.llvm.org/staging/#/builders/126/builds/421/steps/5/logs/FAIL__LLVM__dash-dash_test

If you have some cmp that requires the --, I have found that using env POSIXLY_CORRECT=1 cmp would help.

[hubert.reinterpretcast]

llvm/test/tools/llvm-objcopy/dash-dash.test

10

Or using cmp -- out-obj1 --only-section would work.

llvm/test/tools/llvm-objcopy/dash-dash.test

+## Create a clean temporary directory and cd to it to use relative paths.
+# RUN: rm -rf %t && mkdir %t && cd %t
+
+## Check that llvm-objcopy correctly treats paths after '--' as file paths,
+## even if they start with dashes.
+# RUN: yaml2obj %s -o test-obj
+# RUN: llvm-objcopy --only-section=.test test-obj -- --only-section
+# RUN: llvm-objcopy --only-section=.test test-obj out-obj1
+# RUN: cmp out-obj1 -- --only-section
+
+## Check the case when '--' is specified with input files but no options.
+# RUN: llvm-objcopy -- test-obj --add-section
+# RUN: llvm-objcopy test-obj out-obj2
+# RUN: cmp out-obj2 -- --add-section
+
+## Check the case when '--' is specified with options but no input files.
+# RUN: not llvm-objcopy --only-section=.test -- 2>&1 | FileCheck %s --check-prefixes=CHECK-NO-INPUT
+
+# CHECK-NO-INPUT: error: no input file specified
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_REL

llvm/test/tools/llvm-objcopy/tool-help-message.test

 # RUN: llvm-objcopy -h | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
 # RUN: llvm-objcopy --help | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
 # RUN: not llvm-objcopy 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
+# RUN: not llvm-objcopy -- 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
 # RUN: not llvm-objcopy -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
 # RUN: llvm-strip -h | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
 # RUN: llvm-strip --help | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
 # RUN: not llvm-strip 2>&1 | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
 # RUN: not llvm-strip -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s

llvm/tools/llvm-objcopy/CopyConfig.cpp

static void printHelp(const opt::OptTable &OptTable, raw_ostream &OS,
   // but libOption does not have that yet.
   OS << "\nPass @FILE as argument to read options from FILE.\n";
 }
 
 // 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>
-parseObjcopyOptions(ArrayRef<const char *> ArgsArr,
+parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
                     llvm::function_ref<Error(Error)> ErrorCallback) {
   DriverConfig DC;
   ObjcopyOptTable T;
+
+  const char *const *DashDash =
+      std::find_if(RawArgsArr.begin(), RawArgsArr.end(),
+                   [](StringRef Str) { return Str == "--"; });
+  ArrayRef<const char *> ArgsArr = makeArrayRef(RawArgsArr.begin(), DashDash);
+  if (DashDash != RawArgsArr.end())
+    DashDash = std::next(DashDash);
+
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
-  if (InputArgs.size() == 0) {
+  if (InputArgs.size() == 0 && DashDash == RawArgsArr.end()) {
     printHelp(T, errs(), ToolType::Objcopy);
     exit(1);
   }
 
   if (InputArgs.hasArg(OBJCOPY_help)) {
     printHelp(T, outs(), ToolType::Objcopy);
     exit(0);
   }
 
   if (InputArgs.hasArg(OBJCOPY_version)) {
     outs() << "llvm-objcopy, compatible with GNU objcopy\n";
     cl::PrintVersionMessage();
     exit(0);
   }
 
   SmallVector<const char *, 2> Positional;
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_UNKNOWN))
     return createStringError(errc::invalid_argument, "unknown argument '%s'",
                              Arg->getAsString(InputArgs).c_str());
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_INPUT))
     Positional.push_back(Arg->getValue());
+  std::copy(DashDash, RawArgsArr.end(), std::back_inserter(Positional));
 
   if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");
 
   if (Positional.size() > 2)
     return createStringError(errc::invalid_argument,
                              "too many positional arguments");