This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Print error when executed with no input files
ClosedPublic

Authored by Elvina on Jul 13 2020, 11:35 AM.

Details

Summary

This patch changes llvm-readelf (and llvm-readobj for consistency) behavior. This means to print the error when executed with no input files. Reading from stdin can be achieved via a '-' for the input object.
Fixes https://bugs.llvm.org/show_bug.cgi?id=46400

Yours,
Elvina
Software Engineer
Advanced Software Technology Lab, Huawei

Diff Detail

Event Timeline

Elvina created this revision.Jul 13 2020, 11:35 AM

LGTM, but I'd prefer splitting the large amount of - appending to a separate patch and commit it beforehand.

FWIW, it rather bugs me when tools print their entire (long) help upon command-line syntax error. Simply printing an error is generally better. (E.g. "error: no input files specified.")

FWIW, it rather bugs me when tools print their entire (long) help upon command-line syntax error. Simply printing an error is generally better. (E.g. "error: no input files specified.")

+1 to not printing full help unless it is explicitly asked for.

Thanks for looking at this. Is there a specific test-case for llvm-readobj's behaviour with no input file? You should add one if not.

GNU readelf prints the following warning in this case readelf: Warning: Nothing to do. and returns 1. I'm happy for us to deviate from GNU in this case slightly, perhaps as suggested above with the simple error message, but we definitely should return non-zero.

Elvina updated this revision to Diff 278317.EditedJul 15 2020, 2:51 PM

Thanks to everyone for the review! I changed llvm-readobj's behavior as suggested, so now an error message is displayed if no input files are provided and also made a separate patch for all updated tests https://reviews.llvm.org/D83912

Elvina retitled this revision from [llvm-readobj] Print help when executed with no input files to [llvm-readobj] Print error when executed with no input files.Jul 15 2020, 3:05 PM
Elvina edited the summary of this revision. (Show Details)
Elvina edited the summary of this revision. (Show Details)Jul 15 2020, 3:12 PM
jhenderson accepted this revision.Jul 16 2020, 12:43 AM

LGTM, once a couple of nits are fixed.

By the way, you can show that two commits have dependencies on each other by using the "Edit Related Revisions" Phabricator UI option, or by putting "Depends on DXXXXXX" in the description of the patch.

llvm/test/tools/llvm-readobj/basic.test
7 ↗(On Diff #278317)

Nit: missing full stop.

llvm/tools/llvm-readobj/llvm-readobj.cpp
695

Use lower case for the first letter of error and warning messages (see https://llvm.org/docs/CodingStandards.html#error-and-warning-messages).

This revision is now accepted and ready to land.Jul 16 2020, 12:43 AM

Oh, this probably warrants a release note adding too. Can you do that @Elvina?

@jhenderson Could you please commit on my behalf? I don't have commit rights.

@jhenderson Could you please commit on my behalf? I don't have commit rights.

Can do. What do you want your git commit author name and email address to be? (Feel free to directly email me if you don't want it posted on Phabricator directly).

By the way, if you address a comment by a reviewer, you can check the "Done" box on the inline comment before uploading your diff, and it will be marked as done. It helps highlight when a comment has been addressed/not addressed.

llvm/docs/ReleaseNotes.rst
126 ↗(On Diff #278489)

I would rephrase this slightly as follows:

"llvm-readobj and llvm-readelf behavior has changed to report an error when executed with no input files instead of reading an input from stdin. Reading from stdin can still be achieved by specifying `-` as an input file.

Elvina updated this revision to Diff 278827.Jul 17 2020, 10:01 AM
Elvina marked 3 inline comments as done.

Updated Release Notes

@jhenderson Could you please commit on my behalf? I don't have commit rights.

Can do. What do you want your git commit author name and email address to be? (Feel free to directly email me if you don't want it posted on Phabricator directly).

Thanks a lot! I sent you an email.

This revision was automatically updated to reflect the committed changes.