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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: MaskRay.
jhenderson added a comment.

Sorry for the non-existent response up to now - I'm rather snowed under with review requests.

@MaskRay, any comments?



================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:1
+# To regenerate input, run clang -g -c on the following as '/tmp/tmp.c':
+#   int c;
----------------
Rather than using a canned object in this test, could you write a simple bit of code in assembly that has a few symbols and then use `llvm-mc -g` to assemble it with debug information? I would expect this to work for --line-numbers.

Canned object files are opaque and hard to maintain.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:4
+#   double d;
+#   extern int c;
+#   int foo() {
----------------
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.


================
Comment at: llvm/test/tools/llvm-nm/X86/line-numbers.test:12
+#   int main() {
+#     return e;
+#   }
----------------
As well as an undefined data symbol, you might want an undefined function too.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:703
+  const auto *Obj = dyn_cast<ObjectFile>(S.Sym.getObject());
+  if (!Obj)
+    return;
----------------
Test case?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:708-709
+  section_iterator Sec = cantFail(SR.getSection());
+  if (Sec != Obj->section_end())
+    SectionIndex = Sec->getIndex();
+  object::SectionedAddress Address = {cantFail(SR.getAddress()), SectionIndex};
----------------
Test case? I'm guessing this handles e.g. common or absolute symbols that don't have an associate section?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:719-720
+    for (const SectionRef RelocsSec : Obj->sections()) {
+      if (RelocsSec.relocations().empty())
+        continue;
+      SectionRef TextSec = *cantFail(RelocsSec.getRelocatedSection());
----------------
Test case?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:722-723
+      SectionRef TextSec = *cantFail(RelocsSec.getRelocatedSection());
+      if (!TextSec.isText())
+        continue;
+      for (const RelocationRef R : RelocsSec.relocations()) {
----------------
Is this tested?

I believe you can have references to symbols from outside text sections after all (e.g. populating a variable with an address of a data symbol).


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:725-726
+      for (const RelocationRef R : RelocsSec.relocations()) {
+        if (R.getSymbol() != SR)
+          continue;
+        Expected<DILineInfo> ResOrErr = Symbolizer.symbolizeCode(
----------------
For this to be tested, you need at least two undefined symbols referenced via relocations in the same relocation section, but I don't see them, so I'm guessing it's not.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:730
+        if (Error E = ResOrErr.takeError()) {
+          consumeError(std::move(E));
+          continue;
----------------
Why `consumeError` here rather than reporting the error? Same in various places below.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:764
+  };
+  if (FileName.empty())
+    return;
----------------
You need testing for symbols that don't end up with an associated file/line number (both defined and undefined).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150987