[PATCH] D149759: [symbolizer] Support symbol lookup

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-symbolizer/sym.test:20
 
 
 # RUN: llvm-symbolizer --print-address --obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
----------------
I generally would avoid double blank lines. My personal rule is similar to what I follow in C++ code:
1) No blank line if the comment is tightly linked to the immediately following block.
2) One blank line if the comment is more general (e.g. it applies to a wider area of code/isn't really targeted at a specific block etc).


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:7
+
+## Command "CODE" supports search by symbol name.
+#
----------------
Prefer a slightly longer form that says something like "Show that the "CODE" command supports search by symbol name." Same goes for below comments.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:8
+## Command "CODE" supports search by symbol name.
+#
+# RUN: llvm-addr2line --obj=%p/Inputs/symbols.so "CODE func_01" | FileCheck --check-prefix=CODE-CMD %s
----------------
See my above comment: as this comment is tightly tied to the test case that immediately follows, it doesn't need a blank line (and regardless, blank lines generally don't have a comment marker, unless they are separating paragraphs within a longer comment).


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:13-14
+
+## GNU addr2line accepts symbol names in command line, llvm-symbolizer also allows
+## this in both GNU and LLVM modes.
+#
----------------
I don't think you should reference GNU here - that's a motivation for the behaviour, but not for the test case for that behaviour. Rather, what this comment could say is "Show that llvm-addr2line and llvm-symbolizer accept symbol names on the command-line."


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:20
+
+## A symbol name may be resolved into more than one location.
+#
----------------



================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:31
+
+## If symbol is not found, in LLVM mode the symbolizer echoes input.
+#
----------------
I think I might be getting confused between different patches, but looking at this again, my feeling is that we don't really want this divergence in behaviour between GNU and LLVM mode, unless there's a strong motivation for the LLVM behaviour style. I might be inclined to create a different precursor patch that unifies the two, so that they do what GNU addr2line does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149759