[PATCH] D70394: [DWARF] Add an api to get "interpreted" location lists

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h:52-61
+inline bool operator<(const DWARFAddressRange &LHS,
+                      const DWARFAddressRange &RHS) {
   return std::tie(LHS.LowPC, LHS.HighPC) < std::tie(RHS.LowPC, RHS.HighPC);
 }
 
+inline bool operator==(const DWARFAddressRange &LHS,
+                       const DWARFAddressRange &RHS) {
----------------
Welcome to/probably best to commit changes like this by themselves and separate from this commit - makes the review easier without these orthogonal fixes, etc.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFLocationExpression.h:32-38
+inline bool operator==(const DWARFLocationExpression &L,
+                       const DWARFLocationExpression &R) {
+  return L.Range == R.Range && L.Expr == R.Expr;
+}
+
+raw_ostream &operator<<(raw_ostream &OS, const DWARFLocationExpression &Loc);
+
----------------
labath wrote:
> These, and other random operators the patch is adding, are to make sure one can make sense of the gtest output when the tests fail.
Maybe introducing these separately with unit tests of their own would be good?


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:158-159
 
+raw_ostream &operator<<(raw_ostream &OS, const SectionedAddress &Addr);
+
 /// This is a value type class that represents a single symbol in the list of
----------------
Separate+unit test, perhaps? (wouldn't bother sending this & the other for review - I imagine they'd be simple enough for review-after-commit)


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:318
+  virtual void onStartCompileUnit(DWARFYAML::Unit &CU) {
+    Length = CU.Version >= 5 ? 8 : 7;
+  }
----------------
labath wrote:
> This doesn't take type units into account, but then the whole of DWARFEmitter does not handle type units AFAICT.
Might be worth a comment or rephrasing to make the 8/7 values clear as to what they relate to/come from?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/statistics-dwo.test:83-84
 CHECK: "call site entries":7
-CHECK: "scope bytes total":2817
-CHECK: "scope bytes covered":506
+CHECK: "scope bytes total":2702
+CHECK: "scope bytes covered":1160
 CHECK: "total function size":594
----------------
labath wrote:
> I haven't checked if these numbers are fully correct, but they should definitely be *more* correct then before, as the code previously did not handle split dwarf location lists at all.
Might be worth a manual check of the clang binary to see that the numbers remain the same in the cases where it's expected & perhaps that the numbers are the same for split and non-split builds if this change is expected to fix a bug where they were divergent?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70394