[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 ↗

(On Diff #208279)

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 ↗

(On Diff #208279)

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–18

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–18

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

7

Perhaps this first sentence could be rephrased slightly:

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

8

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

13

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

13

Thanks a lot!

[jhenderson]

LGTM, with the two remaining comments addressed.

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

8

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/docs/CommandGuide/llvm-nm.rst

 llvm-nm - list LLVM bitcode and object file's symbol table
 ==========================================================
 
 .. program:: llvm-nm
 
 SYNOPSIS
 --------
 
 :program:`llvm-nm` [*options*] [*filenames...*]
 
 DESCRIPTION
 -----------
 
 The :program:`llvm-nm` utility lists the names of symbols from the LLVM bitcode
 files, object files, or :program:`ar` archives containing them, named on the
 command line.  Each symbol is listed along with some simple information about
-its provenance.  If no filename is specified, *a.out* is used as the input.
-If *-* is used as a filename, :program:`llvm-nm` will read a file from its
-standard input stream.
+its provenance.  If no filename is specified, or *-* is used as a filename,
+:program:`llvm-nm` will read a file from its standard input stream.
 
 :program:`llvm-nm`'s default output format is the traditional BSD :program:`nm`
 output format.  Each such output record consists of an (optional) 8-digit
 hexadecimal address, followed by a type code character, followed by a name, for
 each symbol.  One record is printed per line; fields are separated by spaces.
 When the address is omitted, it is replaced by 8 spaces.
 
 Type code characters currently supported, and their meanings, are as follows.

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

+## Test llvm-nm when using stdin both explicitly (using '-' as a filename)
+## and implicitly (not specifying any filename).
+
+# RUN: yaml2obj %s -o %t.o
+
+## Pass an explicit filename to produce a baseline output. llvm-nm should
+## have the same behavior when opening a file itself and reading that file
+## from its standard input stream.
+# RUN: llvm-nm %t.o > %t.base 2> %t.err
+
+## Make sure there is no warning message about no file redirected to stdin.
+# RUN: FileCheck %s --input-file=%t.err --allow-empty --implicit-check-not={{.}}
+
+# RUN: llvm-nm - < %t.o > %t.explicit 2> %t.err
+# RUN: FileCheck %s --input-file=%t.err --allow-empty --implicit-check-not={{.}}
+# RUN: cmp %t.base %t.explicit
+
+# RUN: llvm-nm < %t.o > %t.implicit 2> %t.err
+# RUN: FileCheck %s --input-file=%t.err --allow-empty --implicit-check-not={{.}}
+# RUN: cmp %t.base %t.implicit
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
+Symbols:
+  - Name:    symbol_a
+    Section: .text

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 (Filename == "-" && sys::Process::StandardInIsUserInput()) {
+    WithColor::warning(errs(), 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) {