[PATCH] D72658: [llvm-nm] Don't report "no symbols" error for files that contain symbols

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

As mentioned in D52810 <https://reviews.llvm.org/D52810>, GNU nm's behaviour is the same for both a symbol table with only a null symbol and a completely missing symbol table (both print "no symbols"). I think both need testing.



================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml:1
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-nm %t.o 2>&1 | FileCheck --allow-empty %s
----------------
It migth make more sense to merge this and the existing test into a single test, with multiple YAML documents, since it's only a minor variation.


================
Comment at: llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml:2
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-nm %t.o 2>&1 | FileCheck --allow-empty %s
+
----------------
I know this existed in the pre-existing test, but I feel like that test should be fixed too. This won't prove that "no symbols" is printed on stderr, because it's merged with stdout. I think it would be best to pipe stderr to a separate file completely and check that file.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1740
+    writeFileName(errs(), ArchiveName, ArchitectureName);
+    errs() << "no symbols\n";
+  }
----------------
grimar wrote:
> You probably can use the existent `error()` method from line 224 for reporting:
> 
> ```
> static void error(Twine Message, Twine Path = Twine()) {
>   HadError = true;
>   WithColor::error(errs(), ToolName) << Path << ": " << Message << ".\n";
> }
> ```
No, I don't think so. That would cause it to print "error:" and colour the output, which isn't right.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72658/new/

https://reviews.llvm.org/D72658