[Lldb-commits] [PATCH] D119178: Add support for generating debug-info for structured bindings of structs and arrays

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I think this looks pretty good! I have a few questions inside.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4667
+
+  SmallVector<uint64_t, 13> Expr;
+  AppendAddressSpaceXDeref(AddressSpace, Expr);
----------------
13 seems to be unnecessarily high. Shouldn't 1 be enough for the expected single DW_OP_deref?


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4700
+        Expr.push_back(
+            CGM.getContext().toCharUnitsFromBits(fieldOffset).getQuantity());
+      }
----------------
this looks good!


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4713
+                           .toCharUnitsFromBits(value * typeSize)
+                           .getQuantity());
+      }
----------------
Can C++ arrays ever have a non-zero stride? (perhaps due to element alignment?)


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4716
+    }
+  }
+
----------------
Should there be an else { assert("unhandled binding expressions"); } here or are there other expressions that just don't need special handling?


================
Comment at: clang/test/CodeGenCXX/debug-info-structured-binding.cpp:3
+
+// CHECK: call void @llvm.dbg.declare(metadata %struct.A* %[[F:[0-9]+]], metadata ![[F:[0-9]+]], metadata !DIExpression())
+// CHECK: call void @llvm.dbg.declare(metadata %struct.A* %[[F:[0-9]+]], metadata ![[F:[0-9]+]], metadata !DIExpression(DW_OP_plus_uconst, [[F:[0-9]+]]))
----------------
We should check what F is, too, right?


================
Comment at: lldb/test/API/lang/cpp/structured-binding/main.cpp:69
+         tx2 + ty2 + tz2; // break here
+}
----------------
Nice!


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

https://reviews.llvm.org/D119178