[Mlir-commits] [lldb] [llvm] [mlir] [DebugInfo] Add symbolic branches to DIExpression (PR #210850)

[Scott Linder]

================
@@ -555,8 +558,30 @@ bool DwarfExpression::addExpression(
   // and not any other parts of the following DWARF expression.
   assert(!IsEmittingEntryValue && "Can't emit entry value around expression");
 
-  std::optional<DIExpression::ConvertOp> PrevConvertOp;
+  struct LabelOffset {
+    uint64_t ID;
+    uint64_t Offset;
+  };
+  struct BranchFixup {
+    uint64_t LabelID;
+    uint64_t PlaceholderOffset;
+  };
+  constexpr unsigned BranchOffsetByteSize = 2;
+
+  // Iterating over ExprCursor doesn't consume it.
+  bool HasSymbolicBranches =
+      llvm::any_of(ExprCursor, [](DIExpression::ExprOperand Op) {
+        return Op.is(dwarf::DW_OP_LLVM_bra) || Op.is(dwarf::DW_OP_LLVM_skip);
+      });
+
+  SmallVector<LabelOffset, 4> Labels;
+  SmallVector<BranchFixup, 4> Fixups;
+  // Buffer the expression until every label has a byte offset, then patch the
+  // branches.
+  if (HasSymbolicBranches)
----------------
slinder1 wrote:

The fact that `entry_value` only works in very narrow cases that this patch avoids conflicting with is just a quirk of the current implementation. My concern here is that re-using the same buffer adds a barrier to improving `entry_value` handling in the future.

For example, if we just slightly relax `entry_value` to allow it to refer to `arg 0` in expressions with only one arg (this is actually what I thought we already supported, but it turns out we require `isSingleLocationExpression`), then we should allow:

```
DW_OP_LLVM_label 0, DW_OP_LLVM_arg 0, DW_OP_LLVM_entry_value 1, DW_OP_bra 0
```

I imagine we could throw together supporting this in a similar way to what you have here: we lookahead for the `entry_value` at the start of `addExpression` and note where we need to do the buffering.

Without symbolic branches re-using the (single) buffer that is already used by `entry_value` today this is a much simpler change.

We can just cross that road when we come to it, though; the `entry_value` implementation seems like it should just be replaced with something that names the register directly, and when lowering it we can generate the `entry_value` block stuff. In the original proposal that eventually became the locations-on-the-stack changes to DWARF6 we actually [added `DW_OP_LLVM_call_frame_entry_reg <REGNUM>`](https://llvm.org/docs/AMDGPUDwarfExtensionsForHeterogeneousDebugging.html#a-2-5-4-4-1-general-location-description-operations) that does this at the DWARF level, but we can just lower an equivalent LLVM-internal opcode to `entry_value` and still get the benefit in the compiler.

https://github.com/llvm/llvm-project/pull/210850