This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Add file names to error messages
ClosedPublic

Authored by seiya on May 15 2019, 11:15 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

seiya created this revision.May 15 2019, 11:15 PM

We need to remove unwrapOrError in the file parser [1] as well to print the file name in parsing errors. Removing unwrapOrError and return Error instead sounds straightforward to me but it could make the code messy. I'd like to hear your comments.

[1]: https://github.com/llvm/llvm-project/blob/4669cf27508ba00274367d62b295e1a7ca20c7ba/llvm/tools/llvm-objcopy/ELF/Object.cpp#L1119

With a lot of the tests, it might be nice to make sure the file name matches exactly. This can be achieved using FileCheck patterns and command-line options like so:

# RUN: yaml2obj %s > %t.o
# RUN: not llvm-objcopy %t.o %t2.o <options> | FileCheck %s -DFILE=%t.o
# CHECK: error: [[FILE]]: error message...

This makes sure we use the right filename in the error, either the input or output filename.

Another thing is that you should make sure that all the code paths you modified are tested where possible. The tests probably already exist, and just need updating to check that the filename appears.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
688 ↗(On Diff #199752)

I don't have a problem with this change, but why did you make it? Also, it should probably be a separate patch, since it's not related to the bug you are trying to fix.

737 ↗(On Diff #199752)

Rather than create the file error here, it might make more sense to call createFileError in findBuildId.

751 ↗(On Diff #199752)

linkToBuildIdDir already calls createFileError in some situations. Rather than repeat it here, it would be better to change any necessary return sites in that function.

seiya marked 3 inline comments as done.May 17 2019, 2:37 AM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
688 ↗(On Diff #199752)

I changed this to print the output file name specified in --dump-section because, for instance, creating the output file named File may fail with an EISDIR error.

I'll remove this line and submit another patch to update this.

737 ↗(On Diff #199752)

Indeed. I'll change findBuildId to take Config and return a FileError from it.

751 ↗(On Diff #199752)

I thought it would be helpful to print the input file name but now realized that it is not so helpful. I'll remove this change and return the error from this function as it is because it already includes the file name which caused the error.

seiya updated this revision to Diff 200205.May 19 2019, 10:36 PM
  • Applied feedbacks from James.
  • Update tests to check if the error message contains the file name.
seiya marked 3 inline comments as done.May 19 2019, 10:39 PM

I noticed that this patch does not include the updates to dump-section.test. I'll update soon.

seiya updated this revision to Diff 200209.May 20 2019, 12:02 AM
  • Updated dump-section.test.
jhenderson accepted this revision.May 20 2019, 4:36 AM

LGTM, but I'd like a second pair of eyes on it from someone like @MaskRay, @rupprecht, or @jakehehrlich before this goes in. I assume you'll need somebody to commit it for you?

This revision is now accepted and ready to land.May 20 2019, 4:36 AM
jakehehrlich accepted this revision.May 20 2019, 12:49 PM

Awesome. LGTM. Tell me if you want me or someone else to land it. You can also email Chris Latner to get access yourself.

seiya added a comment.May 20 2019, 4:33 PM

LGTM, but I'd like a second pair of eyes on it from someone like @MaskRay, @rupprecht, or @jakehehrlich before this goes in. I assume you'll need somebody to commit it for you?

Yes, I would like you to commit this.

seiya added a comment.May 20 2019, 4:34 PM

Awesome. LGTM. Tell me if you want me or someone else to land it. You can also email Chris Latner to get access yourself.

You mean a write access to SVN as described here? https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Awesome. LGTM. Tell me if you want me or someone else to land it. You can also email Chris Latner to get access yourself.

You mean a write access to SVN as described here? https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Yes, since you're now making regular patches.

I'll try to get around to committing this later today, but I've got some critical things to do today, so it might need to wait until @jakehehrlich gets in later.

seiya added a comment.May 21 2019, 3:32 AM

@jhenderson I see. I'll ask Chris for a commit access.

rupprecht accepted this revision.May 21 2019, 5:14 PM

I'm guessing we have more tests that could use the -DINPUT method here to verify the file is there in the error. This patch LGTM though. Thanks!

seiya added a comment.May 21 2019, 9:50 PM

Now I have a commit access. I'll commit this by myself. @jhenderson

seiya added a comment.May 22 2019, 6:39 AM

Changes in 5316a0d200f8 conflicts with this patch. Should I upload this patch again here for review or should I simply resolve conflicts and push to the repository?

Changes in 5316a0d200f8 conflicts with this patch. Should I upload this patch again here for review or should I simply resolve conflicts and push to the repository?

Use your own judgement. If the changes require significant effort to fix, then feel free to provide an updated diff, but if not, you don't need to.

seiya added a comment.May 22 2019, 6:50 AM

Changes in 5316a0d200f8 conflicts with this patch. Should I upload this patch again here for review or should I simply resolve conflicts and push to the repository?

Use your own judgement. If the changes require significant effort to fix, then feel free to provide an updated diff, but if not, you don't need to.

I see. I'll resolve conflicts and commit this patch without uploading here since it only changes the error messages.

This revision was automatically updated to reflect the committed changes.