[PATCH] D139859: [symbolizer] Support symbol+offset lookup

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/Symbolize.h:109
   Expected<std::vector<DILineInfo>> findSymbol(const ObjectFile &Obj,
-                                               StringRef Symbol);
-  Expected<std::vector<DILineInfo>> findSymbol(StringRef ModuleName,
-                                               StringRef Symbol);
+                                               StringRef Symbol, uint64_t Ofs);
+  Expected<std::vector<DILineInfo>> findSymbol(const std::string &ModuleName,
----------------
Please don't abbreviate `Offset` to `Ofs`. It makes it harder to understand what the variable actually is for no real benefit. Applies throughout.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:36
 FUNCS:  /tmp/dbginfo{{[/\]+}}symbols.part2.cpp:10
+RUN: llvm-symbolizer --obj=%p/Inputs/symbols.so func_01+0 | FileCheck --check-prefix=SYMLINE0 %s
+#SYMLINE0: /tmp/dbginfo{{[/\]+}}symbols.part1.cpp:12
----------------
Please make sure there is testing for hex as well as decimal numbers.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:37
+RUN: llvm-symbolizer --obj=%p/Inputs/symbols.so func_01+0 | FileCheck --check-prefix=SYMLINE0 %s
+#SYMLINE0: /tmp/dbginfo{{[/\]+}}symbols.part1.cpp:12
+RUN: llvm-symbolizer --obj=%p/Inputs/symbols.so func_01+12 | FileCheck --check-prefix=SYMLINE1 %s
----------------
Same throughout, for consitency with the existing tests.


================
Comment at: llvm/test/tools/llvm-symbolizer/symbol-search.test:44
+RUN: llvm-addr2line -e %p/Inputs/symbols.so func_01+0 | 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
----------------
It's slightly offputting that the same set of arguments don't exist for both the llvm-symbolizer and llvm-addr2line test cases. Could you rationalise them, please, so that they both test the same set?


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:162
                           std::string &ModuleName, object::BuildID &BuildID,
-                          StringRef &Symbol, uint64_t &ModuleOffset) {
+                          StringRef &Symbol, uint64_t &Displacement) {
   ModuleName = BinaryName;
----------------
Why has this name changed?


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:256
+
+  // Otherwise it is a symbol name, probably with offset.
+  Symbol = Offset;
----------------
"probably" implies most of the time, but there's no guarantee that's how the option is used.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:271
+    }
+    // The found '+' is not an offset delimiter.
   }
----------------
A trailing `+` needs testing. Also something of the form `notanumber+alsonotanumber` and `notanumber+12notanumber`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139859