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

[sbc100]

Summary

Previously we were reporting this error if we were list no symbols
which is not the same thing as the file containing no symbols.

Also, always report the filename when printing errors.

This matches the GNU nm behaviour.

This a followup to https://reviews.llvm.org/D52810

[grimar]

A few comments/suggestions from me.

llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml

14

nit: we often remove the excessive identation. It
makes the output to be a bit more readable.

16

nit: Some of the llvm-nm tests are using ## for comments, just like
many other our tools already do. I'd follow this style as it is really common nowadays.

llvm/test/tools/llvm-nm/X86/nm-no-symbols.test

11

I see that this way of name checking existed before this patch, but FTR my concern
is that the output name depends on the lit naming rules.

I.e. I'd suggest to be a bit more explicit here:

# RUN: yaml2obj %s > %t.o
# RUN: llvm-nm %t.o 2>&1 | FileCheck %s -DFILE=%t.o
...
# CHECK: [[FILE]]: no symbols{{$}}

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

710

I think this either could accept Twines:

writeFileName(raw_ostream &S, const Twine ArchiveName, ...

or I am thinking it could probably be:

std::string getFileName(const Twine ArchiveName, ...)

I think in the latter case the callers code can be a bit nicer/simpler probably.

1739

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";
}

[grimar]

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

710

const Twine -> const Twine &

[jhenderson]

As mentioned in 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.

llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml

2

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.

3

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.

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

1739

No, I don't think so. That would cause it to print "error:" and colour the output, which isn't right.

[sbc100]

• use count

[sbc100]

• double comments
• use [[FILE]]

llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml

2

I'd rather leave it separate if thats ok with you. They seems different enough to me and it makes it easier to run them individually.

3

I'm just trying to show the output is empty. I've updated it to use "count 0" which clearer.

If you want to fix the existing test thats a separate issue.

14

The prevailing style in this directory seems to be to preserve the yaml with indentation and alignment. I'm not sure I agree that its more readable either. So if its OK with you I'll leave this as is for this change and you and propose an widespread change if you feel strongly.

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

710

I was initially going to do that are part of this change but I'm inclined to follow the style of writeFileName below and leave that refactor to a follow up change.

[sbc100]

• static

[sbc100]

Followup change to convert params to StringRef: https://reviews.llvm.org/D72718

[sbc100]

• remove old check

[sbc100]

ptal

[jhenderson]

llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml

14

I'm also of the opinion that new tests are better without the extra whitespace for readability. However, I don't think it's worth the noise to fix up the existing tests - I prefer a more organic migration from one style to the other, i.e. as you change the YAML in an area, update it.

[sbc100]

• rebase

[sbc100]

• remove indentation

[sbc100]

D72718 landed first so switched to StringRef for args. Also remove extra indentation in yaml.

[merge_guards_bot]

Unit tests: fail. 61930 tests passed, 5 failed and 783 were skipped.

failed: Clang.CodeGen/thinlto_backend.ll
failed: Clang.InterfaceStubs/externstatic.c
failed: Clang.InterfaceStubs/function-template-specialization.cpp
failed: Clang.InterfaceStubs/inline.c
failed: lld.ELF/lto/thinlto-obj-path.ll

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: fail. 61930 tests passed, 5 failed and 783 were skipped.

failed: Clang.CodeGen/thinlto_backend.ll
failed: Clang.InterfaceStubs/externstatic.c
failed: Clang.InterfaceStubs/function-template-specialization.cpp
failed: Clang.InterfaceStubs/inline.c
failed: lld.ELF/lto/thinlto-obj-path.ll

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[jhenderson]

LGTM!

[sbc100]

Oops, looks like this broke some tests. Investigating.

llvm/test/ThinLTO/X86/empty-module.ll

 ; RUN: opt -module-summary -o %t.bc %s
 
 ; RUN: rm -f %t2.0
 ; RUN: llvm-lto2 run  %t.bc -r %t.bc,foo,pl -o %t2 -thinlto-distributed-indexes
 ; RUN: llvm-readobj -h %t2.0 | FileCheck %s
-; RUN: llvm-nm %t2.0 2>&1 | FileCheck %s -check-prefix=NO-SYMBOLS
-; NO-SYMBOLS: no symbols
+; RUN: llvm-nm %t2.0 2>&1 | count 0
 
 ; CHECK: Format: ELF64-x86-64
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @foo = ifunc i32 (i32), i64 ()* @foo_ifunc
 
 define internal i64 @foo_ifunc() {
 entry:
   ret i64 0
 }

llvm/test/ThinLTO/X86/strong_non_prevailing.ll

 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/strong_non_prevailing.ll -o %t2.bc
 
 ; RUN: llvm-lto -thinlto-action=run %t.bc %t2.bc -exported-symbol=__llvm_profile_filename
 ; RUN: llvm-nm -o - < %t.bc.thinlto.o | FileCheck %s --check-prefix=EXPORTED
-; RUN: llvm-nm -o - < %t2.bc.thinlto.o 2>&1 | FileCheck %s --check-prefix=NOT_EXPORTED
+; RUN: llvm-nm -o - < %t2.bc.thinlto.o 2>&1 | count 0
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 $__llvm_profile_filename = comdat any
 
 @__llvm_profile_filename = constant [19 x i8] c"default_%m.profraw\00", comdat
 
 ; EXPORTED: R __llvm_profile_filename
-; NOT_EXPORTED-NOT: R __llvm_profile_filename

llvm/test/tools/llvm-nm/X86/nm-no-symbols-local-only.yaml

+## When a file contains only local symbols the "no symbols" error should not
+## be shown, so we expect the output to be completely empty.
+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-nm %t.o 2>&1 | count 0
+
+!ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Symbols:
+  - Name:  foo.c
+    Type:  STT_FILE
+    Index: SHN_ABS

llvm/test/tools/llvm-nm/X86/nm-no-symbols.test

 # RUN: yaml2obj %s > %t.o
-# RUN: llvm-nm %t.o 2>&1 | FileCheck %s
-# RUN: llvm-nm --print-file-name %t.o 2>&1 | FileCheck %s --check-prefix=CHECK-PRINT-FILE-NAME
+# RUN: llvm-nm %t.o 2>&1 | FileCheck %s -DFILE=%t.o
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 
-# CHECK: {{^}}no symbols{{$}}
-
-# CHECK-PRINT-FILE-NAME: nm-no-symbols.test{{.*}}.o: no symbols{{$}}
+# CHECK: [[FILE]]: no symbols{{$}}

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

static Optional<std::string> demangle(StringRef Name, bool StripUnderscore) {
   free(Undecorated);
   return S;
 }
 
 static bool symbolIsDefined(const NMSymbol &Sym) {
   return Sym.TypeChar != 'U' && Sym.TypeChar != 'w' && Sym.TypeChar != 'v';
 }
 
+static void writeFileName(raw_ostream &S, StringRef ArchiveName,
+                          StringRef ArchitectureName) {
+  if (!ArchitectureName.empty())
+    S << "(for architecture " << ArchitectureName << "):";
+  if (OutputFormat == posix && !ArchiveName.empty())
+    S << ArchiveName << "[" << CurrentFilename << "]: ";
+  else {
+    if (!ArchiveName.empty())
+      S << ArchiveName << ":";
+    S << CurrentFilename << ": ";
+  }
+}
+
 static void sortAndPrintSymbolList(SymbolicFile &Obj, bool printName,
                                    StringRef ArchiveName,
                                    StringRef ArchitectureName) {
   if (!NoSort) {
     using Comparator = bool (*)(const NMSymbol &, const NMSymbol &);
     Comparator Cmp;
     if (NumericSort)
       Cmp = &compareSymbolAddress;
if (isSymbolList64Bit(Obj)) {
     case Radix::x:
       printFormat = OutputFormat == posix ? "%" PRIx64 : "%08" PRIx64;
       break;
     default:
       printFormat = OutputFormat == posix ? "%" PRId64 : "%08" PRId64;
     }
   }
 
-  auto writeFileName = [&](raw_ostream &S) {
-    if (!ArchitectureName.empty())
-      S << "(for architecture " << ArchitectureName << "):";
-    if (OutputFormat == posix && !ArchiveName.empty())
-      S << ArchiveName << "[" << CurrentFilename << "]: ";
-    else {
-      if (!ArchiveName.empty())
-        S << ArchiveName << ":";
-      S << CurrentFilename << ": ";
-    }
-  };
-
-  if (SymbolList.empty()) {
-    if (PrintFileName)
-      writeFileName(errs());
-    errs() << "no symbols\n";
-  }
-
   for (const NMSymbol &S : SymbolList) {
     uint32_t SymFlags;
     std::string Name = S.Name.str();
     MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
     if (Demangle) {
       if (Optional<std::string> Opt = demangle(S.Name, MachO))
         Name = *Opt;
     }
     if (S.Sym.getRawDataRefImpl().p)
       SymFlags = S.Sym.getFlags();
     else
       SymFlags = S.SymFlags;
 
     bool Undefined = SymFlags & SymbolRef::SF_Undefined;
     bool Global = SymFlags & SymbolRef::SF_Global;
     bool Weak = SymFlags & SymbolRef::SF_Weak;
     if ((!Undefined && UndefinedOnly) || (Undefined && DefinedOnly) ||
         (!Global && ExternalOnly) || (Weak && NoWeakSymbols))
       continue;
     if (PrintFileName)
-      writeFileName(outs());
+      writeFileName(outs(), ArchiveName, ArchitectureName);
     if ((JustSymbolName ||
          (UndefinedOnly && MachO && OutputFormat != darwin)) &&
         OutputFormat != posix) {
       outs() << Name << "\n";
       continue;
     }
 
     char SymbolAddrStr[23], SymbolSizeStr[23];
static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool printName,
   unsigned int Nsect = 0;
   MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
   if (!SegSect.empty() && MachO) {
     Nsect = getNsectForSegSect(MachO);
     // If this section is not in the object file no symbols are printed.
     if (Nsect == 0)
       return;
   }
-  if (!MachO || !DyldInfoOnly) {
+  if (!(MachO && DyldInfoOnly)) {
     for (BasicSymbolRef Sym : Symbols) {
       uint32_t SymFlags = Sym.getFlags();
       if (!DebugSyms && (SymFlags & SymbolRef::SF_FormatSpecific))
         continue;
       if (WithoutAliases && (SymFlags & SymbolRef::SF_Indirect))
         continue;
       // If a "-s segname sectname" option was specified and this is a Mach-O
       // file and this section appears in this file, Nsect will be non-zero then
if (DyldInfoOnly || AddDyldInfo ||
           }
           I++;
         }
       }
     }
   }
 
   CurrentFilename = Obj.getFileName();
+
+  if (Symbols.empty() && SymbolList.empty()) {
+    writeFileName(errs(), ArchiveName, ArchitectureName);
+    errs() << "no symbols\n";
+  }
+
   sortAndPrintSymbolList(Obj, printName, ArchiveName, ArchitectureName);
 }
 
 // checkMachOAndArchFlags() checks to see if the SymbolicFile is a Mach-O file
 // and if it is and there is a list of architecture flags is specified then
 // check to make sure this Mach-O file is one of those architectures or all
 // architectures was specificed.  If not then an error is generated and this
 // routine returns false.  Else it returns true.