[llvm-objcopy] Check for missing argument values

[quic-akaryaki]

Report an error if a required value for a command line argument is missing.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (quic-akaryaki)

Changes

Report an error if a required value for a command line argument is missing.

---

Full diff: https://github.com/llvm/llvm-project/pull/70710.diff

2 Files Affected:

• (added) llvm/test/tools/llvm-objcopy/tool-options.test (+4)
• (modified) llvm/tools/llvm-objcopy/ObjcopyOptions.cpp (+6)

diff --git a/llvm/test/tools/llvm-objcopy/tool-options.test b/llvm/test/tools/llvm-objcopy/tool-options.test
new file mode 100644
index 000000000000000..7116ff39485562b
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/tool-options.test
@@ -0,0 +1,4 @@
+## An error must be reported if a required argument value is missing.
+# RUN: not llvm-objcopy --only-section 2>&1 | FileCheck --check-prefix=CHECK-NO-VALUE %s
+# RUN: not llvm-objcopy -O 2>&1 | FileCheck --check-prefix=CHECK-NO-VALUE %s
+# CHECK-NO-VALUE: error: argument to '{{.*}}' is missing
diff --git a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
index d33adb0b6a3e478..63c3873b1e94340 100644
--- a/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
+++ b/llvm/tools/llvm-objcopy/ObjcopyOptions.cpp
@@ -552,6 +552,12 @@ objcopy::parseObjcopyOptions(ArrayRef<const char *> RawArgsArr,
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
+  if (MissingArgumentCount)
+    return createStringError(
+        errc::invalid_argument,
+        "argument to '%s' is missing (expected %d value(s))",
+        InputArgs.getArgString(MissingArgumentIndex), MissingArgumentCount);
+
   if (InputArgs.size() == 0 && DashDash == RawArgsArr.end()) {
     printHelp(T, errs(), ToolType::Objcopy);
     exit(1);

[alexander-shaposhnikov]

to me LG, I'd wait for @jh7370

[jh7370]

Why are you using a wildcard pattern here? It seems sensible to properly check the message generated. This also is only testing part of the message, but there are other parts that are important too.

[quic-akaryaki]

I am using a wildcard to reuse one pattern for several (two) different options. The point of this test is to verify that the message gets printed. I think there is enough context in the message to avoid misidentification for other messages.
I am not checking if the message is exactly the same because I don't see it as a realistic possibility.
I don't think a random corruption in the output is likely, and testing for it would be outside of the scope of this test.

[jh7370]

I think there is enough context in the message to avoid misidentification for other messages.

Agreed.

I don't think a random corruption in the output is likely, and testing for it would be outside of the scope of this test.

This isn't why I think you should check the exact contents. Checking the exact contents explicitly will show that you've used the correct %* stuff in your createStringError call. Equally, should the code change in the future to use a different mechanism to parse the command-line/report the error/etc, it would show that the change is still reporting the context correctly.

[alexander-shaposhnikov]

nit: since at the moment there is only one check prefix in the entire test you can simply use the default one (CHECK)

[quic-akaryaki]

I agree, but I thought this test could be expanded in the future and in that case, it will be possible to add other patterns without rewriting existing ones.

[quic-akaryaki]

Sorry it was too strong. It is certainly possibly to add new check prefixes without rewriting the default one, but then the default one would stand out awkwardly, in my opinion.

[jh7370]

Unless you have explicit plans to extend the test in the near future, go with the simpler approach now (default prefix). If we need to change it in the future, we can.

[jh7370]

You're still not checking the latter part of the message, and in particular the substitution for the argument count.

[quic-akaryaki]

I added the check for the full message in 49c078a. Thanks.

[jh7370]

Are there any options that expect 2+ values (i.e. something that would give a different response to "expected 1 value(s)"? If so, it might be worth switching one of the cases for that one, just for additional bonus coverage without additional test complexity.

Otherwise, LGTM.

[alexander-shaposhnikov]

LG

[quic-akaryaki]

I'm sorry I dropped the ball on this PR. I closed the branch as part of clean up. There may still be some value in this change.

[quic-akaryaki]

Are there any options that expect 2+ values (i.e. something that would give a different response to "expected 1 value(s)"? If so, it might be worth switching one of the cases for that one, just for additional bonus coverage without additional test complexity.

Otherwise, LGTM.

I don't think there are multi-arg options in objcopy. There are some when objcopy is invoked as llvm-install-name-tool, but they are handled separately.

[quic-akaryaki]

CoverageMapping/mcdc-system-headers.cpp test fails on Windows, which runs clang and filecheck only. That can't be related to this change. I'm not a frequent committer, is it an acceptable practice to merge PR with unrelated failures? I could continue rebasing until it passes, but this seems a waste of resources as each build takes a few hours.

[jh7370]

CoverageMapping/mcdc-system-headers.cpp test fails on Windows, which runs clang and filecheck only. That can't be related to this change. I'm not a frequent committer, is it an acceptable practice to merge PR with unrelated failures? I could continue rebasing until it passes, but this seems a waste of resources as each build takes a few hours.

Yes, merging with unrelated failures is fine.