[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:1-2
+# Check that printing line numbers is not attempted for symbolic object files
+# that are not object files.
+# RUN: llvm-nm --line-numbers %p/Inputs/test.IRobj-x86_64 | FileCheck %s --strict-whitespace --check-prefix=BITCODE
----------------
Nit: new tests in many of the LLVM tools use `##` for comment markers, to help them stand out from the lit and FileCheck directives.

I don't understand how a "symbolic object file" is not already an "object file". This comment looks suspiciously like it's essentially copying code terms into the English prose, without actually stating what is significant. What is a "symbolic object file"? What is an "object file" in this context? Most importantly, how are the two different?


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:4
+# RUN: llvm-nm --line-numbers %p/Inputs/test.IRobj-x86_64 | FileCheck %s --strict-whitespace --check-prefix=BITCODE
+# BITCODE-NOT: :
+
----------------
If you are trying to show that there is no output, it is better to do `count 0` instead of a FileCheck command. However, I'm guessing that the output isn't expected to be empty. Showing that ":" is not in the output feels very fragile though - it could easily appear in a file path (if an absolute path is used on Windows, for example), or indeed other parts of the output. Better would be to match the output that is produced and use stuff like `--implicit-check-not={{.}}` to show that this is the entire output that is present.

`--strict-whitespace` also has no purpose in this particular test case.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:8
+# RUN: split-file %s %t
+# RUN: llvm-mc -g --filetype=obj %t/main.s -o %t/main.o
+# RUN: llvm-nm -l %t/main.o | FileCheck %s --match-full-lines --implicit-check-not={{.}}
----------------
It would make sense for this case to have a comment too, since all the others now do.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:22
+
+# Check that in the absence of DWARF no line number information is printed.
+# RUN: llvm-mc --filetype=obj %t/main.s -o %t/no-dwarf.o
----------------
Can I suggest "... in the absence of any DWARF in the whole object, no line number ..." to clarify the distinction between this and the individual cases `data_no_dwarf` etc.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:42-44
+# Check that printing line numbers for undefined values does not include
+# relocations for non-text sections. This is broken out of main.s to ensure
+# that the data relocation for undef comes first.
----------------
Nit: I'm confused what line wrapping rules you're following in this test file - compare this to the previous comment, and you'll note that one is not wrapped at all, whereas this one is broken up over three lines.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:76
+
+# Note; llvm-mc -g produces no DWARF for data.
+.data
----------------
Same below.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:697
+    return;
+  SymbolRef SR = S.Sym;
+  uint64_t SectionIndex = object::SectionedAddress::UndefSection;
----------------
mysterymath wrote:
> jhenderson wrote:
> > 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.
> This performs a cast; I've rewritten this to explicitly call the constructor to make this clearer. Renamed to `Sym`.
Could `const SymbolRef Sym(S.Sym);` be used? That seems clearer to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150987