[PATCH] D120168: [Debuginfo][LSR][NFC] Add support for salvaging variadic dbg.value intrinsics [1/2]

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added a comment.

In D120168#3360280 <https://reviews.llvm.org/D120168#3360280>, @Orlando wrote:

>> I can clarify the comment. It's my understanding that non-empty DIExpressions must have a DW_OP_stack_value terminator. The expressions generated by the SCEVDbgValueBuilder objects do not have these terminators (as the expressions may be inserted into other expressions). If the pre-LSR expression was empty, once the complete salvaging expression has been generated, then the terminator must be appended.
>
> My main confusion was in why the addition of `stack_value` was necessary in the patch but wasn't added before. I'd missed that you were previously using `prependOpcodes` with param `stackValue=true`, but are now adding the opcode yourself. And I agree that adding a `stack_value` is correct in this instance because you're generating implicit locations (though I think "non-empty DIExpressions must have a DW_OP_stack_value terminator" is not true in general?). In any case, sorry for the protracted review as a result of my misunderstanding!
>
> `prependOpcodes` also accounts for inserting a `stack_value` opcode before fragment opcodes, which your implementation doesn't appear to handle. Would it be possible to rearrange the code to use `prependOpcodes` so that all cases are accounted for?

I understand your query now. I think because of the new method used to construct the new expression (parse the old expression left-to-right and push onto a new expression, substituting salvage expressions when necessary) that prependopcodes is not applicable. I think the lambda i added today to insert the terminator is a clear explicit method to ensure the terminator is present.


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

https://reviews.llvm.org/D120168