[PATCH] D149759: [symbolizer] Support symbol lookup

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for the delay - I was away last week, and am still catching up on reviews.



================
Comment at: llvm/test/tools/llvm-symbolizer/flag-grouping.test:6
 
-CHECK: some text
+CHECK: something
 CHECK: 0x40054d: inctwo
----------------
Let's match the full line in all of these cases where the text has changed.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:9
+# RUN: llvm-symbolizer --obj=%p/Inputs/symbols.so "CODE func_01" | FileCheck --check-prefix=CODE_CMD %s
+# CODE_CMD: /tmp/dbginfo{{[/\]+}}symbols.part1.cpp:12
+
----------------
I forgot to mention that I prefer `-` to `_` in prefix names. It avoids weird awkardness like `CODE_CMD-NEXT` for example, and `-` is easier to type on an English keyboard :D


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:19
+
+# RUN: llvm-addr2line --obj=%p/Inputs/symbols.so func_666 | FileCheck --check-prefix=NONEXISTENT_ADDR %s
+# NONEXISTENT_ADDR: ??
----------------
`ADDR` is probably not a good abbreviation for `addr2line` since it could be confused with `address` which is a a very relevant term to these tests. Perhaps `A2L`? Apologies for the churn.


================
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`.
----------------
sepavloff wrote:
> 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.
Can I ask you to spin off sym.test's updates into a new review? I've got no objection to the update (sym.test itself is a little bit archaic), but it's not really related to this patch.

Also "when address" -> "when an address"


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:4
+
+RUN: llvm-addr2line --obj=%p/Inputs/symbols.so "CODE func_01" | FileCheck --check-prefix=PRFUNC %s
+RUN: llvm-symbolizer --obj=%p/Inputs/symbols.so "CODE func_01" | FileCheck --check-prefix=PRFUNC %s
----------------
jhenderson wrote:
> I can't tell what "PRFUNC" is supposed to stand for. It might be useful to add a comment before each test case explaining what that specific case is testing.
The individual sub cases within this test file could still do with some comments to make their purpose clearer. I think I follow it better now with the recent improvements, but more clarity would help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149759