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

[abrachet]

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.

[abrachet]

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?

[jhenderson]

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]

Removed test case. Added documentation.

[abrachet]

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!

[jhenderson]

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.

[jhenderson]

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 test case

[abrachet]

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

[jhenderson]

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]

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

[abrachet]

llvm/test/tools/llvm-nm/stdin.test

12 ↗

(On Diff #208499)

Thanks a lot!

[jhenderson]

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.

[MaskRay]

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

1756

keyboard input -> terminal.

[abrachet]

Addressed review comments

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

+## Tests that when no input file is specified stdin is
+## read but only when not reading from keyoard input.
+
+# RUN: not llvm-nm 2>&1 | FileCheck %s
+
+# CHECK: llvm-nm: warning: can't read from keyboard input.
+# CHECK-NEXT: OVERVIEW: llvm symbol table dumper

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

 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/Wasm.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <vector>
 
 using namespace llvm;
if (none_of(ArchFlags, [&](const std::string &Name) {
       })) {
     error("No architecture specified", Filename);
     return false;
   }
   return true;
 }
 
 static void dumpSymbolNamesFromFile(std::string &Filename) {
+  if (sys::Process::StandardInIsUserInput()) {
+    WithColor::warning(outs(), ToolName) << "can't read from keyboard input.\n";
+    cl::PrintHelpMessage();
+    HadError = true;
+    return;
+  }
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getFileOrSTDIN(Filename);
   if (error(BufferOrErr.getError(), Filename))
     return;
 
   LLVMContext Context;
   LLVMContext *ContextPtr = NoLLVMBitcode ? nullptr : &Context;
   Expected<std::unique_ptr<Binary>> BinaryOrErr =
int main(int argc, char **argv) {
   // The relative order of these is important. If you pass --size-sort it should
   // only print out the size. However, if you pass -S --size-sort, it should
   // print out both the size and address.
   if (SizeSort && !PrintSize)
     PrintAddress = false;
   if (OutputFormat == sysv || SizeSort)
     PrintSize = true;
   if (InputFilenames.empty())
-    InputFilenames.push_back("a.out");
+    InputFilenames.push_back("-");
   if (InputFilenames.size() > 1)
     MultipleFiles = true;
 
   // If both --demangle and --no-demangle are specified then pick the last one.
   if (NoDemangle.getPosition() > Demangle.getPosition())
     Demangle = !NoDemangle;
 
   for (unsigned i = 0; i < ArchFlags.size(); ++i) {