[PATCH] D150987: [llvm-nm] Add --line-numbers flag

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:27-41
+# RUN: llvm-mc --filetype=obj %t/undef-no-relocs.s -o %t/undef-no-relocs.o
+# RUN: llvm-nm --line-numbers %t/undef-no-relocs.o | FileCheck %s --check-prefix=UNDEF-NO-RELOCS --match-full-lines --implicit-check-not={{.}}
+
+# UNDEF-NO-RELOCS: U undef
+
+# RUN: llvm-mc -g --filetype=obj %t/undef-data-reloc.s -o %t/undef-data-reloc.o
+# RUN: llvm-nm --line-numbers %t/undef-data-reloc.o | FileCheck %s --check-prefix=UNDEF-DATA-RELOC --match-full-lines --implicit-check-not={{.}}
----------------
It's not immediately clear to readers of this why you can't have some or all of these folded into main.s. It would be useful to have comments explaining a) exactly what each test case is trying to test that is different to other cases, and b) why they can't be part of main.s.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:63
+.globl undef
+#--- undef-data-reloc.s
+.globl undef
----------------
It would be clearer if there was a blank line between each input file.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:697
+    return;
+  SymbolRef SR = S.Sym;
+  uint64_t SectionIndex = object::SectionedAddress::UndefSection;
----------------
I'm not convinced there's any value in this local variable. In fact, the name `SR` is non-obvious as to its purpose or anything, and this function is somewhat long. I'd just get rid of the local variable entirely.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:703
+  const auto *Obj = dyn_cast<ObjectFile>(S.Sym.getObject());
+  if (!Obj)
+    return;
----------------
jhenderson wrote:
> Test case?
Although this is marked as done, it's not clear to me what test case is covering it. Could you please elaborate a bit more?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:730
+        if (Error E = ResOrErr.takeError()) {
+          consumeError(std::move(E));
+          continue;
----------------
jhenderson wrote:
> Why `consumeError` here rather than reporting the error? Same in various places below.
Now that you're reporting the error, there needs to be test cases for the error cases.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:764
+  };
+  if (FileName.empty())
+    return;
----------------
jhenderson wrote:
> You need testing for symbols that don't end up with an associated file/line number (both defined and undefined).
Although this is marked as done, I expected to see some test cases that had a mixture of "stuff found in the debug information" and "stuff not found in the debug information" to properly exercise this behaviour.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:772
                             StringRef ArchiveName, StringRef ArchitectureName) {
+  llvm::symbolize::LLVMSymbolizer Symbolizer;
+
----------------
jhenderson wrote:
> phosek wrote:
> > It'd be better if we could avoid instantiating `LLVMSymbolizer` unless needed since this is a non-trivial object. A simple way to do would be to move it to `printLineNumbers` and make it `static` but there might a cleaner way.
> `std::optional<LLVMSymbolizer>` which is initialised if LineNumbers is true, perhaps?
> 
> Nit: I don't think you need the `llvm::` prefix here, right?
> Nit: I don't think you need the llvm:: prefix here, right?

Still unaddressed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150987