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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1811
+        return DIExpression::prependOpcodes(OldDII.getExpression(), Ops,
+                                            DIExpression::WithStackValue);
+      }
----------------
bjope wrote:
> 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.
Now I understand, thanks. The key language from the docs I was missing is:
"DIExpressions also follow this model: A DIExpression that doesn’t have a trailing DW_OP_stack_value will describe an address when combined with a concrete location."

This means the current patch does not handle any dbg.{addr,declare} or non-empty DIExpressions properly. It would mess up the variable's address calculation and/or pre-existing stack_value computations.

I think we should handle this case. I'll try to find a way to normalize expressions as stack values before appending any DW_OPs to them.


https://reviews.llvm.org/D48676