[llvm-strip] Let llvm-strip continue on encountering an error

[meltq]

This change means that llvm-strip no longer exits immediately upon encountering an error when modifying a file and will instead continue modifying the other inputs. Fixes #129412

[llvmbot]

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

Author: Tejas Vipin (meltq)

Changes

Lets tools that use llvm-objcopy continue with working on files on encountering an error by logging it instead of returning it early. This lets tools like llvm-strip function as expected while processing multiple files. Closes #129412

---

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

1 Files Affected:

• (modified) llvm/tools/llvm-objcopy/llvm-objcopy.cpp (+5-2)

diff --git a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp b/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
index 7e708e309f207..e3d0ba5f8a0bb 100644
--- a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ b/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -247,14 +247,17 @@ int llvm_objcopy_main(int argc, char **argv, const llvm::ToolContext &) {
                           WithColor::error(errs(), ToolName));
     return 1;
   }
+
+  int ret = 0;
+
   for (ConfigManager &ConfigMgr : DriverConfig->CopyConfigs) {
     assert(!ConfigMgr.Common.ErrorCallback);
     ConfigMgr.Common.ErrorCallback = reportWarning;
     if (Error E = executeObjcopy(ConfigMgr)) {
       logAllUnhandledErrors(std::move(E), WithColor::error(errs(), ToolName));
-      return 1;
+      ret = 1;
     }
   }
 
-  return 0;
+  return ret;
 }

[jh7370]

Thanks for the quick PR!

You need to add a test for the behaviour. You should add that test to llvm/test/tools/llvm-objcopy and show the following:

1. That llvm-strip can successfully strip an object that appears later on the command-line after a bad object and returns 1 still.
2. That llvm-strip emits an error for every bad object on the command-line.

You should also add a note in the LLVM release notes for llvm-objcopy for this fix (llvm/docs/release).

A couple of points about the commit title and description, since those end up as the commit message:

1. Use "llvm-strip" in the title, since llvm-objcopy doesn't support multiple inputs.
2. Bit of a nit-pick, but I'd say "Fixes XXXX" rather than "Closes XXXX" because it more clearly indicates that the issue is fixed.
3. I think your description is a bit clunky, so we can improve that. Apart from anything else, there's no need to mention the source where this change was made. Try this:

This change means that llvm-strip no longer exits immediately upon encountering an error when modifying a file and will instead continue modifying the other inputs.

[meltq]

Added the test, cleaned up the code and changed the title, description.

[meltq]

Made suggested changes.

[jh7370]

A couple of process points:

1. Per the documented LLVM policy, please use fixup commits to make updates and avoid force pushes. Force pushing makes it difficult to review what has changed since the previous version of the PR, making reviewers' lives harder.
2. Please don't click the "Resolve Conversation" button when you've addressed a request. As a reviewer, I use unresolved conversations to track what changes have been requested and then can review any changes against these conversations to see if the made changes were what were requested. See https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178 for more discussion.

Aside from that and one nit in the test, LGTM. Do you need help merging?

[jh7370]

Nit, please remove extra spaces.

Suggested change

	- Name: .debugGlobal
	Type: SHT_PROGBITS
	Content: "00000000"
	- Name: .debugGlobal
	Type: SHT_PROGBITS
	Content: "00000000"

[jh7370]

Oh, one more thing, before this can be merged, you need to mark your email address as non-private in GitHub. From How to Submit a Patch:

After your PR is approved, ensure that:

• The PR title and description describe the final changes. These will be used as the title and message of the final squashed commit. The titles and messages of commits in the PR will not be used.
• You have set a valid email address in your GitHub account, see Email Addresses.

[meltq]

Addressed Nit. From my understanding, all commits need to be merged at the end so I rebased and force pushed (unless I was wrong?)
Additionally, my email address should be public now. And I would appreciate you merging.

[jh7370]

Addressed Nit. From my understanding, all commits need to be merged at the end so I rebased and force pushed (unless I was wrong?)

You're right in that the final commit that lands in main should be a single distinct commit (so all fixups should be merged). However, the default merge option we use in GitHub is "Squash and Merge" which squashes the commits into a single commit and then rebases it on main, before pushing, so there's no need to do it manually.

Additionally, my email address should be public now. And I would appreciate you merging.

Noted. I'll let the pre-merge checks finish before doing the merge.