This is an archive of the discontinued LLVM Phabricator instance.

[tools] [llvm-nm] Default to reading from stdin not a.out
ClosedPublic

Authored by abrachet on Jul 6 2019, 1:15 PM.

Details

Summary

This moves away from defaulting to a.out and uses stdin only if stdin has a file redirected to it. This has been discussed on the llvm-dev mailing list here.

Diff Detail

Event Timeline

abrachet created this revision.Jul 6 2019, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2019, 1:15 PM
abrachet marked an inline comment as done.Jul 6 2019, 1:26 PM

No tests tested if a.out was used as a default file so no need to change any other tests, they are all passing except the one that I added!

llvm/test/tools/llvm-nm/warn-invalid-stdin.test
6

This isn't working right now. FileCheck says the output was actually "The file was not recognized as a valid object file". Does llvm-lit redirect something to stdin?

Please update the llvm-nm documentation with this change.

No tests tested if a.out was used as a default file so no need to change any other tests, they are all passing except the one that I added!

Have you tried testing this manually? I suspect things might go wrong...

llvm/test/tools/llvm-nm/warn-invalid-stdin.test
6

I haven't checked, but I'd expect it to redirect something like /dev/null to stdin by default, to avoid tests hanging if they are run incorrectly. I'm not sure you're going to be able to test this properly.

llvm/tools/llvm-nm/llvm-nm.cpp
1755

Shouldn't this be conditional on whether we are reading from stdin?

abrachet updated this revision to Diff 208385.Jul 8 2019, 6:07 AM

Removed test case. Added documentation.

abrachet marked 3 inline comments as done.Jul 8 2019, 6:14 AM

Have you tried testing this manually? I suspect things might go wrong...

I have tried these

$ llvm-nm
llvm-nm: warning: can't read from keyboard input.
<help message>
$ llvm-nm `which llvm-nm` # this wasn't touched so should work the same
<works>
$ llvm-nm < `which llvm-nm` # should be the same as above now
<works>
llvm/docs/CommandGuide/llvm-nm.rst
17 ↗(On Diff #208385)

I have used the same language here to what existed prior to rL363065

llvm/tools/llvm-nm/llvm-nm.cpp
1755

Oops. Thanks!

Is there a test case showing the new behaviour of reading from stdin now (both explicitly and implicitly)? We should add/extend it for the new (and existing) stdin behaviour.

llvm/docs/CommandGuide/llvm-nm.rst
17 ↗(On Diff #208385)

Let's keep the phrase of "read from" rather than "process on", since llvm-nm doesn't do any processing (which implies major work or editing to me), just reading and interpreting.

abrachet updated this revision to Diff 208390.Jul 8 2019, 6:36 AM
abrachet marked an inline comment as done.
abrachet marked an inline comment as done.Jul 8 2019, 6:38 AM
jhenderson added inline comments.Jul 8 2019, 8:02 AM
llvm/test/tools/llvm-nm/stream-redirection.test
1 ↗(On Diff #208390)

This isn't really about "redirection", so let's just call this test "stdin.test" or similar.

Also, this test should show that no warning is printed in each case (since there could be if the input was coming from a keyboard).

5 ↗(On Diff #208390)

Put a comment here saying that this is to establish a baseline to compare against. My first reaction was "why are we testing output redirection?"

Perhaps also call the output file %t.base or something to that effect.

10 ↗(On Diff #208390)

I don't think you need this test case. It's no different from regular stdin input.

abrachet updated this revision to Diff 208497.Jul 8 2019, 1:41 PM

Updated test case

abrachet marked 3 inline comments as done.Jul 8 2019, 1:43 PM
abrachet updated this revision to Diff 208499.Jul 8 2019, 1:53 PM

Prefer pipe to cmp(1) than creating a file.

jhenderson added inline comments.Jul 9 2019, 4:49 AM
llvm/test/tools/llvm-nm/stdin.test
6 ↗(On Diff #208499)

Perhaps this first sentence could be rephrased slightly:

"Pass an explicit filename to produce a baseline output."

7 ↗(On Diff #208499)

and reading from -> "and reading that file from"

12 ↗(On Diff #208499)

This isn't what you want:

a) grep is deprecated in lit tests. Use FileCheck CHECK-NOT/--implicit-check-not.
b) warnings should be printed to stderr, not stdout, so you're searching the wrong place.

You want something like:

# RUN: llvm-nm %t.o > %t.base 2> %t.err
# RUN: FileCheck %s --input-file=%t.err --allow-empty --implicit-check-not={{.}}

You'll want to add the same check to each of the different test cases.

abrachet updated this revision to Diff 208860.Jul 9 2019, 6:24 PM

Print warning to errs(). Reword comments. Check for warning message with FileCheck not grep.

abrachet marked 4 inline comments as done.Jul 9 2019, 6:25 PM
abrachet added inline comments.
llvm/test/tools/llvm-nm/stdin.test
12 ↗(On Diff #208499)

Thanks a lot!

abrachet marked an inline comment as done.Jul 9 2019, 6:25 PM
jhenderson accepted this revision.Jul 10 2019, 6:07 AM

LGTM, with the two remaining comments addressed.

llvm/test/tools/llvm-nm/stdin.test
7 ↗(On Diff #208499)

My bad, I should have said "and when..."

llvm/tools/llvm-nm/llvm-nm.cpp
1756

Double-check this, but I'm pretty sure most warnings and errors don't end in a full-stop, so please remove it.

This revision is now accepted and ready to land.Jul 10 2019, 6:07 AM
MaskRay added inline comments.Jul 11 2019, 10:49 PM
llvm/tools/llvm-nm/llvm-nm.cpp
1756

keyboard input -> terminal.

MaskRay accepted this revision.Jul 11 2019, 10:50 PM
abrachet updated this revision to Diff 209443.Jul 12 2019, 2:45 AM

Addressed review comments

jhenderson accepted this revision.Jul 12 2019, 3:10 AM
This revision was automatically updated to reflect the committed changes.