[PATCH] D102355: Add support for DWARF embedded source to llvm-symbolizer

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:68-71
+    if (L < FirstLine)
+      continue;
+    if (L > LastLine)
+      break;
----------------
Could you pull this (& any other refactoring you think makes the semantic-changing patch more isolated/focussed) into a preliminary commit?


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:259-276
+      int64_t FirstLine =
+          std::max(static_cast<int64_t>(1),
+                   int64_t(LineInfo.Line) - Config.SourceContextLines / 2);
+      int64_t LastLine = FirstLine + Config.SourceContextLines - 1;
+      size_t FirstLinePos = StringRef::npos, Pos = 0;
+      for (int64_t Line = 1; Line <= LastLine; ++Line, ++Pos) {
+        if (Line == FirstLine)
----------------
I take it this is trimming the trailing lines that won't be needed, while leaving the leading lines so the algorithm in printContext can still retrieve the lines in a source-agnostic way (so it can retrieve them in the same way from embedded source as non-embedded source)

It seems unfortunate to have split the pruning logic in two places like this - perhaps a generic function that takes the line iterator & just the desired context (pruning both leading and trailing lines) - then use that function here, and have the "Source" attribute contain the fully pruned text.

Then in `printContext` if the `Source` value is provided, use that literally - otherwise open the file and prune its leading/trailing using the same function?

(also that refactoring -  pulling out such a reusable pruning function - should be done as a separate commit)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102355