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

Thu Sep 7 16:13:20 PDT 2023

mysterymath added inline comments.

================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:2
+# RUN: llvm-nm --line-numbers %p/Inputs/test.IRobj-x86_64 | FileCheck %s --strict-whitespace --check-prefix=BITCODE
+# BITCODE-NOT: :
+
----------------
MaskRay wrote:
> What does this check?
Added a comment for this.

================
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={{.}}
----------------
jhenderson wrote:
> 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.
Added comments before each of these section for this.

================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:4
+#   double d;
+#   extern int c;
+#   int foo() {
----------------
jhenderson wrote:
> I'm not sure what this line is intended to be testing? Should that be an `extern int e;`?
> 
> I think it would be useful to have some comments explaining what each line is trying to test.
Yes; 'c' was a typo, it should have been 'e'. Regardless, I've renamed all these symbols to give an indication of why the symbol is part of the test case.

================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:697
+    return;
+  SymbolRef SR = S.Sym;
+  uint64_t SectionIndex = object::SectionedAddress::UndefSection;
----------------
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`.

================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:703
+  const auto *Obj = dyn_cast<ObjectFile>(S.Sym.getObject());
+  if (!Obj)
+    return;
----------------
jhenderson wrote:
> 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?
This is covered by the initial bitcode file test case; this is a symbolic file that is not an object file, so this dynamic cast fails.

================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:730
+        if (Error E = ResOrErr.takeError()) {
+          consumeError(std::move(E));
+          continue;
----------------
jhenderson wrote:
> 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.
A test case that specifically targets this line encodes a proof that the line is reachable, but I don't have one, as these checks represent instead that there is no known proof that the case isn't reachable. Even if a proof of reachability or non-reachability were available, it would depend on encapsulated implementation details of Symbolizer and earlier portions of llvm-nm, and it shouldn't be baked into the behavior of printing line numbers, since that's logically independent from what kind of module validation has been done up until that point. Even if these cases aren't actually reachable (and thus fully untestable), there should still be checks here, since the reason they're not reachable is so contingent on other factors that should be free to change independently.

================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:764
+  };
+  if (FileName.empty())
+    return;
----------------
jhenderson wrote:
> 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.
There is already some of this present in `main.s`, e.g., the absolute value case has no DWARF. I've added additional cases to `main.s` to show as many mixtures as are feasible using only the mechanisms available to `llvm-mc`. Handcrafting DWARF for the test case would make the abstraction level of the test mis-match the abstraction level of the implementation. The implementation is fully abstract over debugging formats, so ideally, it should be possible to update the test cases for such code without the ability to craft DWARF by hand.

================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:772
                             StringRef ArchiveName, StringRef ArchitectureName) {
+  llvm::symbolize::LLVMSymbolizer Symbolizer;
+
----------------
jhenderson wrote:
> 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?
Ah sorry.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150987