[PATCH] D149759: [symbolizer] Support symbol lookup

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for the delay - Phabricator issues and being busy meant I only just got back to this.



================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:246-247
+  // result.
+  if (!Info)
+    return Result;
+
----------------
sepavloff wrote:
> sepavloff wrote:
> > jhenderson wrote:
> > > Ditto.
> > Actually this code was copied from `symbolize*Common`, it will fail exactly in the same cases when fail those functions.
> It seems this case (no error from `getOrCreateModuleInfo` but zero pointer to `SymbolizableModule`) cannot be realized.
Okay, thanks for looking into it.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:12
+
+# Check if a symbol name can be specified on the command line.
+RUN: llvm-addr2line -e %p/Inputs/symbols.so func_01 | FileCheck --check-prefix=SYMB %s
----------------
I think canoncially it should be "command-line" not "command line" (this is based on what someone from our docs team once told me).


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:55
+
+# Show that both symbol and binary file can be specified on the command line.
+RUN: llvm-addr2line "%p/Inputs/symbols.so func_01" | FileCheck --check-prefix=SYMBIN %s
----------------
Strictly speaking, using the `--obj` option to provide the binary file is also specifying it on the command-line. I think this comment should be a little more specific (assuming it matters). I'd also use "input file" rather than "binary file" to clarify the file's purpose. Something like: "Show that both the symbol and input file can be specified in the search string on the command-line." or something like that. 

Similar comments apply below.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:61
+# Show that the case of missing binary file specified on the command line is properly treated.
+RUN: llvm-addr2line "%p/Inputs/666.so func_01" 2>%t.1.stderr | FileCheck --check-prefix=NONEXISTENT %s
+RUN: FileCheck --input-file=%t.1.stderr --check-prefix=BINARY-NOT-FOUND -DMSG=%errc_ENOENT %s
----------------
Nit: here and below, add a space after `>2` to make it stand out more.


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

https://reviews.llvm.org/D149759