[PATCH] D32315: Introduce a new DWARFContext::getInliningInfoForAddress API to expose pointers to strings stored in DWARF file.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:205
+  /// of the inline stack.
+  void getInliningInfoForAddress(
+      uint64_t Address, DILineInfoSpecifier Spec,
----------------
I'd expect to see changes to DIContext to add this as a virtual function there - and, say, changes to llvm-symbolizer to use this new API to exercise and benefit from the improvement?

(& actually the old version could become non-virtual & implemented in DIContext so all clients don't have to implement both - that'd involve changing the COFF implementation to implement the std::function version too, which will help exercise/demonstrate that this API choice is generically beneficial to both formats, potentially)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:207
+      uint64_t Address, DILineInfoSpecifier Spec,
+      std::function<void(const char *, const char *, const char *, const char *,
+                         uint32_t, uint32_t, uint32_t, uint32_t)>
----------------
You can use llvm::function_ref here (it's like std::function, but doesn't have the overhead of separate allocation - used for when the functor doesn't need to be copied to outlive its scope)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:207
+      uint64_t Address, DILineInfoSpecifier Spec,
+      std::function<void(const char *, const char *, const char *, const char *,
+                         uint32_t, uint32_t, uint32_t, uint32_t)>
----------------
dblaikie wrote:
> dblaikie wrote:
> > You can use llvm::function_ref here (it's like std::function, but doesn't have the overhead of separate allocation - used for when the functor doesn't need to be copied to outlive its scope)
> Might be worth using a struct rather than 8 arguments, for readability?
Is StringRef an option, rather than const char*? (especially if the length is already known  due to previous string ops, perhaps)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:207-208
+      uint64_t Address, DILineInfoSpecifier Spec,
+      std::function<void(const char *, const char *, const char *, const char *,
+                         uint32_t, uint32_t, uint32_t, uint32_t)>
+          Inserter);
----------------
dblaikie wrote:
> You can use llvm::function_ref here (it's like std::function, but doesn't have the overhead of separate allocation - used for when the functor doesn't need to be copied to outlive its scope)
Might be worth using a struct rather than 8 arguments, for readability?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:560
+                       uint32_t, uint32_t, uint32_t, uint32_t)>
+        Inserter) {
   DWARFCompileUnit *CU = getCompileUnitForAddress(Address);
----------------
I'd probably rename this from "Inserter" (& probably update the comment) - it doesn't have to insert anything into anything. "Visitor", "Callback", "FrameHandler"?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:707
+
+bool DWARFDebugLine::LineTable::getFileLineInfoForAddress(
+    uint64_t Address, const char *CompDir, FileLineInfoKind Kind,
----------------
Pushing the use case through LLVM's symbolizer might also remove the need for keeping this function, maybe? Perhaps not.


https://reviews.llvm.org/D32315