[PATCH] D48676: [Local] Teach insertReplacementDbgValues basic integer/pointer conversions

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1811
+        return DIExpression::prependOpcodes(OldDII.getExpression(), Ops,
+                                            DIExpression::WithStackValue);
+      }
----------------
vsk wrote:
> bjope wrote:
> > I don't think WithStackValue is correct if there could be users from findDbgUsers that is a dbg.declare (because dbg.declare is indirect).
> > Maybe unlikely to happen here, but I don't know. The same kind of fault exist in salvageDebugInfo, and I've been trying to understand if we ever expect to have non-empty DIExpressions in dbg.declare (except for DW_OP_LLVM_fragment). For example having a DW_OP_stack_value in dbg.declare seems weird, right?
> > 
> > And what happens if we have a dbg.value and the DIExpression already got a non-empty DIExpression, ending without a DW_OP_stack_value. Then it is indirect, and adding DW_OP_stack_value would turn it into being direct. Or should it perhaps be illegal to have an indirect dbg.value like that?
> > 
> You're correct to point out that the operand of a dbg.{addr,declare} is the address of a variable, not the variable itself. But I don't see how we'd take the sign-extension code path here when the dbg intrinsic operand is a pointer. Actually, allowing this utility and salvageDI to operate on dbg.addr seems useful, because they can preserve debug info across no-op pointer bitcasts.
> 
> I don't think we have any code that would add a DW_OP_stack_value to a dbg.{addr, declare}, but if we did, I don't think it would be weird. IIUC, it would just mean the debugger needs to do pointer arithmetic before loading a variable. That seems just like having a dbg.value for a pointer operand and adding a DW_OP_deref at the end (before DW_OP_stack_value).
> 
> I don't understand your last question about dbg.values with non-empty, non-stack-value expressions. It seems reasonable to create these (say, by using DW_OP_deref), and I don't see why they can't also be treated as stack values.
> I don't understand your last question about dbg.values with non-empty, non-stack-value expressions. It seems reasonable to create these (say, by using DW_OP_deref), and I don't see why they can't also be treated as stack values.

Let's say that we have
 
```
 dbg.value  i32 %x, !y, DIExpression(DW_OP_lit1, DW_OP_add)
```
then it means that the value of !y is given by dereferencing (%x+1).
If %x can be rewritten as a sign-extend of some value %z, and you use 
```
  prependOpcodes(OldDII.getExpression(), Ops, DIExpression::WithStackValue);
```
then, afaik, the DW_OP_stack_value is added at the end of the expression, while the Ops are prepended. So you end up with

```
 dbg.value  i32 %z, !y, DIExpression(Ops, DW_OP_lit1, DW_OP_add, DW_OP_stack_value)
```
So we will end up saying that !y is equal to (%x + 1) instead of that (%x + 1) points to the value.

This could be an unexpected situation here, never supposed to happen. Then some asserts showing those assumptions might be good. Maybe such asserts could be put inside prependOpcodes.


https://reviews.llvm.org/D48676