[PATCH] D60067: [llvm-symbolizer] Add llvm-addr2line.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-symbolizer/addr2line.test:1
+# Should print the function name with '-f'.
+# Should print an inlined function name even with '-i=0' (the default)
----------------
Rather than adding a test that tests each of the places where llvm-addr2line is different to llvm-symbolizer, I think it would make a lot more sense to add test cases using llvm-addr2line to the existing llvm-symbolizer tests, where the behaviour is different.


================
Comment at: tools/llvm-symbolizer/llvm-symbolizer.cpp:97-99
+static cl::alias ClBinaryNameAliasE("e", cl::desc("Alias for -obj"),
+                                    cl::Grouping, cl::NotHidden,
+                                    cl::aliasopt(ClBinaryName));
----------------
This change should be separated out into a different patch, as it affects llvm-symbolizer behaviour too.


================
Comment at: tools/llvm-symbolizer/llvm-symbolizer.cpp:149
 
+static cl::opt<DIPrinter::OutputStyle>
+    ClOutputStyle("output-style", cl::init(DIPrinter::OutputStyle::LLVM),
----------------
Test cases for this new option?


================
Comment at: tools/llvm-symbolizer/llvm-symbolizer.cpp:254
+  cl::ParseCommandLineOptions(
+      argc, argv, IsAddr2Line ? "llvm-addr2line\n" : "llvm-symbolizer\n");
 
----------------
Maybe worth having a basic test that shows that the usage text is correct for both tools.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60067