[PATCH] D118633: [Symbolizer] Add Build ID flag to llvm-symbolizer.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the rework. I've added some basic comments, but I think @phosek (or someone else) would be better to continue this review beyond these points.



================
Comment at: llvm/docs/CommandGuide/llvm-symbolizer.rst:189
+  Look up the object using the given build ID, specified as a hexadecimal
+  string. Mutually exclusive with the --obj family.
+
----------------
I'd delete "family" since "-e" etc are just aliases, and therefore it's implied.


================
Comment at: llvm/docs/CommandGuide/llvm-symbolizer.rst:240
   Path to object file to be symbolized. If ``-`` is specified, read the object
-  directly from the standard input stream.
+  directly from the standard input stream. Mutually exclusive with --build-id.
 
----------------



================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/Symbolize.h:61-66
   Expected<DILineInfo> symbolizeCode(const ObjectFile &Obj,
                                      object::SectionedAddress ModuleOffset);
   Expected<DILineInfo> symbolizeCode(const std::string &ModuleName,
                                      object::SectionedAddress ModuleOffset);
+  Expected<DILineInfo> symbolizeCode(ArrayRef<uint8_t> BuildID,
+                                     object::SectionedAddress ModuleOffset);
----------------
Not necessarily something that you need to do right now, but nonetheless worth thinking about: it's clear by adding a third variation of each of these methods that some sort of refactoring would be appropriate to simplify extensibility of this code. In this case, I'd probably make each of the different input types (object, module name, build ID) a separate class, with the relevant methods attached. `symbolizeCode` would then just become something like:
```
template <typename T>
Expected<DILineInfo> symbolizeCode(const T &Input, object::SectionedAddress ModuleOffset) {
  return Input.symbolizeCode(ModuleOffset, ...);
}
```
and so on. Adding a new input kind wouldn't then require modifying this class again, keeping this class stable, and just adding new classes for each input kind.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:389
+  if (I != BuildIDPaths.end()) {
+    // If an error was recorded
+    if (I->second.empty())
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118633