[PATCH] D149759: [symbolizer] Support symbol lookup

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 9 inline comments as done.
sepavloff added inline comments.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-empty-line.test:12
 RUN: llvm-symbolizer --output-style=GNU -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
-RUN:   | FileCheck %s --check-prefix=GNU
+RUN:   | FileCheck %s --check-prefix=GNUOUT
 
----------------
jhenderson wrote:
> I don't think these check-suffixes are particularly understandable any more, given the changed behaviour. I'd suggest instead renaming them all and moving the check patterns to be immediately after the group that uses them. Something like:
> 
> ```
> RUN: llvm-symbolizer -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
> RUN:   | FileCheck %s --check-prefix=SYMB-LLVM
> 
> RUN: llvm-symbolizer --output-style=LLVM -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
> RUN:   | FileCheck %s --check-prefix=SYMB-LLVM
> 
> SYMB-LLVM: x.c:14:0
> SYMB-LLVM-EMPTY:
> SYMB-LLVM-NEXT: some text2
> 
> RUN: llvm-symbolizer --output-style=GNU -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
> RUN:   | FileCheck %s --check-prefix=SYMB-GNU
> 
> SYMB-GNU: x.c:14
> SYMB-GNU-NEXT: some text2
> 
> RUN: llvm-addr2line -i -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
> RUN:   | FileCheck %s --check-prefix=ADDR-GNU
> 
> RUN: llvm-addr2line --output-style=GNU -i -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
> RUN:   | FileCheck %s --check-prefix=ADDR-GNU
> 
> ADDR-GNU: x.c:14
> ADDR-GNU-NEXT: ??:0
> 
> RUN: llvm-addr2line --output-style=LLVM -i -e %p/Inputs/addr.exe < %p/Inputs/addr.inp \
> RUN:   | FileCheck %s --check-prefix=ADDR-LLVM
> 
> ADDR-LLVM: x.c:14:0
> ADDR-LLVM-EMPTY:
> ADDR-LLVM-NEXT: ??:0
> ```
> 
Yes, it looks better. Thank you!


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:1
+# This test uses ELF shared object `Inputs/symbols.so` built for x86_64 using
+# the instructions from `Inputs/symbols.h`.
----------------
jhenderson wrote:
> You're using a weird mix of comment markers in this file. The standard rules in newer tools tests are
>   - `##` for actual comments;
>   - `#` for RUN and CHECK lines;
>   - all comment markers should have a space between them and the rest of the line (e.g. `# CHECK:` or `## This is a comment`).
>   - optionally, if the test consists solely of things prefixed with a comment marker, you can drop one `#` from each of the above, but it's more common to have them than not.
> 
> In addition, this test should have a comment explaining what the test is testing, probably before this comment.
Thank you for eplanations. I modernized also sym.test, it didn't follow these rules.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:9
+RUN: llvm-addr2line -e %p/Inputs/symbols.so func_01 | FileCheck --check-prefix=A2L0 %s
+RUN: llvm-addr2line -e %p/Inputs/symbols.so func_01 | FileCheck --check-prefix=A2L0 %s
+#A2L0: /tmp/dbginfo{{[/\]+}}symbols.part1.cpp:12
----------------
jhenderson wrote:
> Why is this test case epeated twice?
It should be `llvm-addr2line` and `llvm-symbolizer` variants. Fixed.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:471
   Config.Verbose = Args.hasArg(OPT_verbose);
+  Config.IsAddr2Line = IsAddr2Line;
 
----------------
jhenderson wrote:
> This should be controlled by the output-style command-line option (see my comment elsewhere).
Initialization of `Config.IsGNUStyle` is now below in this function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149759