[PATCH] D92013: [DebugInfo] Don't use DW_OP_implicit_value for fragments

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D92013#2453364 <https://reviews.llvm.org/D92013#2453364>, @aprantl wrote:

> In D92013#2413734 <https://reviews.llvm.org/D92013#2413734>, @labath wrote:
>
>> I'd like to also fix this in a more fundamental way, but I think I'd need some guidance from someone more familiar with this code (who would that be?).
>>
>> One idea I had is to add something like `isPureFragment()` to the `DIExpression` class, which would return true if the expression consistent solely of a single DW_OP_LLVM_fragment operand.
>
> You mean  `DIExpression(DW_OP_LLVM_fragment 0, 1)` is "pure" s opposed to `DIExpression(DW_OP_deref, DW_OP_LLVM_fragment 0, 1)`, which isn't? Or something else?

Yes, that's pretty much it.

>> Then this code could use that to detect when it is safe to do a `DwarfExpr.addExpression(std::move(ExprCursor))` to emit a `DW_OP_piece`. One would probably also need to introduce a new LocationKind constant (`ImplicitlyImplicit` :P) to indicate that there's no need to emit `DW_OP_stack_value`. Or it might be possible to reuse one of the existing kinds for this purpose.
>>
>> Does that sounds reasonable?
>
> Are you saying that the code to emit FP constants is not passing the `DW_OP_LLVM_fragment` from `DIExpr` to `DwarfExpr` (and we happen to emit a skip-piece, which often has the same effect)?

Affirmative.

> Shouldn't we then try to get DwarfExpression to process the fragment operator from `DIExpr` instead? I'm probably missing something...

Yes, that is what I (too) think we should do. The thing which makes it tricky (and maybe that's the thing you're missing) is that we have two ways of printing a FP constant (controlled by a debugger tuning parameter, among other things), and they have different constraints on what can be placed after them.

- the first method does that via `DW_OP_const***`. These are regular dwarf operators (pushing a value on the stack) and they can be combined with anything that can be present in the DIExpression (I'm not sure why one would want to e.g. dereference a FP constant, but yeah...)
- the second method uses DW_OP_implicit_value. This operator describes the value directly (it doesn't just push something on the stack). This means we should not place DW_OP_stack_value after it. It also means it cannot be combined with DW_OP_deref, DW_OP_plus, etc. However, it *can* be combined with DW_OP_(bit_)piece (i.e., DW_OP_LLVM_fragment), which is why we need a way to detect whether an expression consists _solely_ of DW_OP_LLVM_fragment. (and yeah, I agree that a dedicated way of representing fragments would be nice)

Another option would be to be conservative, and choose the DW_OP_const method whenever the DIExpr is non-empty, even if it just consists of a DW_OP_LLVM_fragment. That is what this patch, in its current form does. This fixes the problem at hand, but I don't think it's a good long term strategy, as it's not possible to represent floating point values larger than 64 bits in this way (at least not without additional fragmenting).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92013