[PATCH] D56587: Fix sign/zero extension in Dwarf expressions.

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1867
+
+      return DIExpression::appendToStack(DIExpression::appendToStack(DII.getExpression(), dwarf::DW_OP_convert, ToBT), dwarf::DW_OP_convert, FromBT);
     };
----------------
markus wrote:
> bjope wrote:
> > I still have trouble to understand how/if helps the debugger (or llc) to know how many bits to dereference. I assume that llc needs to prepend a DW_OP_deref_type in case of a memory location, or DW_OP_regval_type in case of a register location to get the correct type on the expression stack for the original value. And then we only need one DW_OP_convert to convert to the final type.
> > In case we think that it is ok to dereference more bits than `ToBits` (i.e. the smaller size), then we need to adjust the address to take care of endianess somewhere.
> > If we go down this road, then I think we need some hack to get DW_OP_deref_type/DW_OP_regval_type in place first. Or what do you think?
> > 
> > I still have trouble to understand how/if helps the debugger (or llc) to know how many bits to dereference. 
> Ok, lets try to clarify that then so that we all can agree here.
> 
> > I assume that llc needs to prepend a DW_OP_deref_type in case of a memory location, or DW_OP_regval_type in case of a register location to get the correct type on the expression stack for the original value. And then we only need one DW_OP_convert to convert to the final type.
> Yes, down the road that would be ideal as I see it. In the meantime we could do with using two DW_OP_convert ops in sequence as in this patch.  
> 
> > In case we think that it is ok to dereference more bits than ToBits (i.e. the smaller size), then we need to adjust the address to take care of endianess somewhere.
> I think that we immediately need to start using DW_OP_deref_size instead of DW_OP_deref to cover the endianess effects, but this is really a separate bug/issue. Modifying the address seems a less desirable way to achieve this.
> 
> > If we go down this road, then I think we need some hack to get DW_OP_deref_type/DW_OP_regval_type in place first. Or what do you think?
> I think that we can start using two DW_OP_convert in sequence and then treat the DW_OP_deref_size as a separate issue and finally DW_OP_deref_type and DW_OP_regval_type as long term goals.
> 
> Makes sense?
> I think that we can start using two DW_OP_convert in sequence and then treat the DW_OP_deref_size as a separate issue and finally DW_OP_deref_type and DW_OP_regval_type as long term goals.
>
> Makes sense?

Makes a little sense at least. I guess it all depends on if this is supposed to be "complete" or just a partial solution. If we go for the latter then you need to update the description (and probably also add some code comments here mentioning that this still gives faulty result in certain situations). Extra credits for adding test cases that show that we still do wrong in some situations.

At least it seems like we agree that for a "complete" solution we need to express how many bits that should be dereferenced instead of the first DW_OP_convert. With this patch we still present wrong values in the debugger sometimes (at least for big endian platforms, right?).


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

https://reviews.llvm.org/D56587