llvm dev - Aug 2020 - [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to support multiple location operands

Currently there is a series of patches undergoing review[0] that seek to enable
the use of multiple IR/MIR values when describing a source variable's
location. The current plan for the MIR is to add a new instruction,
DBG_VALUE_LIST, that supports this functionality by having a variable number of
operands. It may be better however to simply replace the existing DBG_VALUE
behaviour entirely instead, and so I'm looking for any comments on this
change before pushing ahead with it.

There are a few differences between the MIR instructions:

Old: DBG_VALUE %x, $noreg, !DILocalVariable("x"), !DIExpression()
New: DBG_VALUE !DILocalVariable("x"), !DIExpression(DW_OP_LLVM_arg, 0,
DW_OP_stack_value), %x

1) The "location" operand is moved to the end, as the instruction is
now variadic such that every operand after the DIExpression is a location
operand.
2) The second operand which currently represents "Indirectness" has
been removed entirely, because this is now explicitly specified in the
DIExpression (see 4).
3) The DIExpression no longer implicitly treats the location operand as the
first element of the expression, instead each location must be explicitly
referenced in the expression using `DW_OP_LLVM_arg, N` for the Nth location
operand.
4) The DIExpression itself must be explicit about whether it evaluates to the
location of a variable or its literal value, by using DW_OP_stack_value in the
latter case (instead of relying on the Indirectness flag, which is both
confusing and redundant[1]).

I believe this is a strict improvement to the expressiveness and clarity of
DBG_VALUE. Although it increases the verbosity of simple expressions, such a
change is necessary to remove potential ambiguities in constant debug
expressions[2]. We will also be relying on the DIExpression to replace the
"Indirectness" flag, since it should now solely determine whether or
not a value is indirect; this brings us closer to the final DWARF
representation. One potential downside is that using DW_OP_stack_value for a
simple single-register DBG_VALUE (as in the example above) would currently lose
information, as it would output the DWARF expression `DW_OP_breg0 RSP+0,
DW_OP_stack_value` instead of the current output `DW_OP_reg0 RSP`. The former is
larger and gives less information, as both expressions evaluate to the same
value but only the latter gives a location for the variable that can be modified
by a debugger. This can be fixed with some pattern matching in the
DwarfExpression class to cover this specific (albeit common) case.

The current approach for the IR is not to add a new instruction, but to add a
new metadata node that contains a list of IR value references (wrapped as
ValueAsMetadata) and use it as the first argument to dbg.value. There is no
syntactic incompatibility between this and the current dbg.value, and therefore
it is possible to support both simultaneously, but I believe it would be
unnecessarily complicated to maintain two separate forms of dbg.value. There is
no immediate plan to change dbg.declare and dbg.addr in the same way: there is
some value in the distinction between the intrinsics, the addresses do not use
constant values (and so avoid the ambiguity described in [2]), and there are few
(possibly no) cases where dbg.addr or dbg.declare intrinsics that use more than
one IR value would actually be produced: only salvageDebugInfo can produce
multi-value debug intrinsics, and debug address intrinsics usually use a
non-salvageable alloca as the location (I am currently unsure as to whether
non-alloca address intrinsics can or should be produced anywhere).

Described here are the differences in the IR intrinsics:

Old: @llvm.dbg.value(metadata i32 %x, metadata !DILocalVariable("x"),
metadata !DIExpression())
New: @llvm.dbg.value(metadata !DIValueList(i32 %x), metadata
!DILocalVariable("x"), metadata !DIExpression(DW_OP_LLVM_arg, 0,
DW_OP_stack_value))

1) The location operand is changed from a single Value to a list of 0 or more
Values.
2) The DIExpression is modified in the same manner as in the MIR instruction
(see above).

In summary, this is a notice of the intent to introduce these changes in the
patch described above. Currently the patches add these modified instructions
alongside the existing ones, but a total replacement would be a better outcome.
This is not a full RFC but is intended to ensure that this change doesn't
catch anyone by surprise and that there are no significant objections.

[0] https://reviews.llvm.org/D82363
[1] https://bugs.llvm.org/show_bug.cgi?id=41675#c8
[2] http://lists.llvm.org/pipermail/llvm-dev/2020-February/139441.html
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200825/1e11e6bd/attachment.html>

Hi Stephen,

Thanks for working on this.
>In summary, this is a notice of the intent to introduce these changes in the
patch described above. Currently the patches add these modified instructions
alongside the existing ones, but a total replacement would be a better outcome.
This is not a full RFC but is intended to ensure that this change doesn't
catch anyone by surprise and that there are no significant objections.
In my opinion, given this whole picture, total replacement is better approach. 
Furthermore, we can leave the old DBG_VALUE (by renaming it to DBG_VALUE_OLD or
so) to add some time to other folks to completely adjust/switch to the new
DBG_VALUE (e.g. out there might be some downstream features depending on the
DBG_VALUE). WDYT?

Best regards,
Djordje

________________________________
From: llvm-dev <llvm-dev-bounces at lists.llvm.org> on behalf of Tozer,
Stephen via llvm-dev <llvm-dev at lists.llvm.org>
Sent: Tuesday, August 25, 2020 8:09 PM
To: llvm-dev at lists.llvm.org <llvm-dev at lists.llvm.org>
Subject: [llvm-dev] [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to support
multiple location operands

Currently there is a series of patches undergoing review[0] that seek to enable
the use of multiple IR/MIR values when describing a source variable's
location. The current plan for the MIR is to add a new instruction,
DBG_VALUE_LIST, that supports this functionality by having a variable number of
operands. It may be better however to simply replace the existing DBG_VALUE
behaviour entirely instead, and so I'm looking for any comments on this
change before pushing ahead with it.

There are a few differences between the MIR instructions:

Old: DBG_VALUE %x, $noreg, !DILocalVariable("x"), !DIExpression()
New: DBG_VALUE !DILocalVariable("x"), !DIExpression(DW_OP_LLVM_arg, 0,
DW_OP_stack_value), %x

1) The "location" operand is moved to the end, as the instruction is
now variadic such that every operand after the DIExpression is a location
operand.
2) The second operand which currently represents "Indirectness" has
been removed entirely, because this is now explicitly specified in the
DIExpression (see 4).
3) The DIExpression no longer implicitly treats the location operand as the
first element of the expression, instead each location must be explicitly
referenced in the expression using `DW_OP_LLVM_arg, N` for the Nth location
operand.
4) The DIExpression itself must be explicit about whether it evaluates to the
location of a variable or its literal value, by using DW_OP_stack_value in the
latter case (instead of relying on the Indirectness flag, which is both
confusing and redundant[1]).

I believe this is a strict improvement to the expressiveness and clarity of
DBG_VALUE. Although it increases the verbosity of simple expressions, such a
change is necessary to remove potential ambiguities in constant debug
expressions[2]. We will also be relying on the DIExpression to replace the
"Indirectness" flag, since it should now solely determine whether or
not a value is indirect; this brings us closer to the final DWARF
representation. One potential downside is that using DW_OP_stack_value for a
simple single-register DBG_VALUE (as in the example above) would currently lose
information, as it would output the DWARF expression `DW_OP_breg0 RSP+0,
DW_OP_stack_value` instead of the current output `DW_OP_reg0 RSP`. The former is
larger and gives less information, as both expressions evaluate to the same
value but only the latter gives a location for the variable that can be modified
by a debugger. This can be fixed with some pattern matching in the
DwarfExpression class to cover this specific (albeit common) case.

The current approach for the IR is not to add a new instruction, but to add a
new metadata node that contains a list of IR value references (wrapped as
ValueAsMetadata) and use it as the first argument to dbg.value. There is no
syntactic incompatibility between this and the current dbg.value, and therefore
it is possible to support both simultaneously, but I believe it would be
unnecessarily complicated to maintain two separate forms of dbg.value. There is
no immediate plan to change dbg.declare and dbg.addr in the same way: there is
some value in the distinction between the intrinsics, the addresses do not use
constant values (and so avoid the ambiguity described in [2]), and there are few
(possibly no) cases where dbg.addr or dbg.declare intrinsics that use more than
one IR value would actually be produced: only salvageDebugInfo can produce
multi-value debug intrinsics, and debug address intrinsics usually use a
non-salvageable alloca as the location (I am currently unsure as to whether
non-alloca address intrinsics can or should be produced anywhere).

Described here are the differences in the IR intrinsics:

Old: @llvm.dbg.value(metadata i32 %x, metadata !DILocalVariable("x"),
metadata !DIExpression())
New: @llvm.dbg.value(metadata !DIValueList(i32 %x), metadata
!DILocalVariable("x"), metadata !DIExpression(DW_OP_LLVM_arg, 0,
DW_OP_stack_value))

1) The location operand is changed from a single Value to a list of 0 or more
Values.
2) The DIExpression is modified in the same manner as in the MIR instruction
(see above).

In summary, this is a notice of the intent to introduce these changes in the
patch described above. Currently the patches add these modified instructions
alongside the existing ones, but a total replacement would be a better outcome.
This is not a full RFC but is intended to ensure that this change doesn't
catch anyone by surprise and that there are no significant objections.

[0] https://reviews.llvm.org/D82363
[1] https://bugs.llvm.org/show_bug.cgi?id=41675#c8
[2] http://lists.llvm.org/pipermail/llvm-dev/2020-February/139441.html
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200826/1d6c913a/attachment.html>

> On Aug 25, 2020, at 11:09 AM, Tozer, Stephen via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Currently there is a series of patches undergoing review[0] that seek to
enable the use of multiple IR/MIR values when describing a source variable's
location. The current plan for the MIR is to add a new instruction,
DBG_VALUE_LIST, that supports this functionality by having a variable number of
operands. It may be better however to simply replace the existing DBG_VALUE
behaviour entirely instead, and so I'm looking for any comments on this
change before pushing ahead with it.
Thank you for writing this up! I think this is generally a good idea.
> 
> There are a few differences between the MIR instructions:
> 
> Old: DBG_VALUE %x, $noreg, !DILocalVariable("x"), !DIExpression()
> New: DBG_VALUE !DILocalVariable("x"),
!DIExpression(DW_OP_LLVM_arg, 0, DW_OP_stack_value), %x
> 
> 1) The "location" operand is moved to the end, as the instruction
is now variadic such that every operand after the DIExpression is a location
operand.
Sounds good.
> 2) The second operand which currently represents "Indirectness"
has been removed entirely, because this is now explicitly specified in the
DIExpression (see 4).
Sounds good, too. We may need to pay a cost for rewriting more DIExpressions,
but I don't see a way to make this work with multiple operands otherwise.
> 3) The DIExpression no longer implicitly treats the location operand as the
first element of the expression, instead each location must be explicitly
referenced in the expression using `DW_OP_LLVM_arg, N` for the Nth location
operand.
This is nice and consistent. If we are worried about the extra memory needed we
can still come up with a more efficient encoding of the common case, but the
DIExpression *interface* should present it like this.
> 4) The DIExpression itself must be explicit about whether it evaluates to
the location of a variable or its literal value, by using DW_OP_stack_value in
the latter case (instead of relying on the Indirectness flag, which is both
confusing and redundant[1]).
I'm not sure this will work as stated here. Indirectness is (mostly)
orthogonal to DW_OP_stack_value. DW_OP_stack_value denotes that we reconstructed
the value of the variable, but it doesn't exist in the program ("The
DW_OP_stack_value operation specifies that the object does not exist in memory
but its value is nonetheless known"), for example, a constant value. I
think we want something like DW_OP_deref instead, at least for r-values. For
l-values (=variables a debugger could write to) we would need to have a
discriminator that declares the DBG_VALUE as a memory location (cf. DWARF5
chapter 2.6).

I think this is going in the right direction, we just need to sort out that last
point!

thanks,
adrian

> 
> I believe this is a strict improvement to the expressiveness and clarity of
DBG_VALUE. Although it increases the verbosity of simple expressions, such a
change is necessary to remove potential ambiguities in constant debug
expressions[2]. We will also be relying on the DIExpression to replace the
"Indirectness" flag, since it should now solely determine whether or
not a value is indirect; this brings us closer to the final DWARF
representation. One potential downside is that using DW_OP_stack_value for a
simple single-register DBG_VALUE (as in the example above) would currently lose
information, as it would output the DWARF expression `DW_OP_breg0 RSP+0,
DW_OP_stack_value` instead of the current output `DW_OP_reg0 RSP`. The former is
larger and gives less information, as both expressions evaluate to the same
value but only the latter gives a location for the variable that can be modified
by a debugger. This can be fixed with some pattern matching in the
DwarfExpression class to cover this specific (albeit common) case.
> 
> The current approach for the IR is not to add a new instruction, but to add
a new metadata node that contains a list of IR value references (wrapped as
ValueAsMetadata) and use it as the first argument to dbg.value. There is no
syntactic incompatibility between this and the current dbg.value, and therefore
it is possible to support both simultaneously, but I believe it would be
unnecessarily complicated to maintain two separate forms of dbg.value. There is
no immediate plan to change dbg.declare and dbg.addr in the same way: there is
some value in the distinction between the intrinsics, the addresses do not use
constant values (and so avoid the ambiguity described in [2]), and there are few
(possibly no) cases where dbg.addr or dbg.declare intrinsics that use more than
one IR value would actually be produced: only salvageDebugInfo can produce
multi-value debug intrinsics, and debug address intrinsics usually use a
non-salvageable alloca as the location (I am currently unsure as to whether
non-alloca address intrinsics can or should be produced anywhere).
> 
> Described here are the differences in the IR intrinsics:
> 
> Old: @llvm.dbg.value(metadata i32 %x, metadata
!DILocalVariable("x"), metadata !DIExpression())
> New: @llvm.dbg.value(metadata !DIValueList(i32 %x), metadata
!DILocalVariable("x"), metadata !DIExpression(DW_OP_LLVM_arg, 0,
DW_OP_stack_value))
> 
> 1) The location operand is changed from a single Value to a list of 0 or
more Values.
> 2) The DIExpression is modified in the same manner as in the MIR
instruction (see above).
> 
> In summary, this is a notice of the intent to introduce these changes in
the patch described above. Currently the patches add these modified instructions
alongside the existing ones, but a total replacement would be a better outcome.
This is not a full RFC but is intended to ensure that this change doesn't
catch anyone by surprise and that there are no significant objections.
> 
> [0] https://reviews.llvm.org/D82363
> [1] https://bugs.llvm.org/show_bug.cgi?id=41675#c8
> [2] http://lists.llvm.org/pipermail/llvm-dev/2020-February/139441.html
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

>Furthermore, we can leave the old DBG_VALUE (by renaming it to DBG_VALUE_OLD
or so) to add some time to other folks to completely adjust/switch to the new
DBG_VALUE (e.g. out there might be some downstream features depending on the
DBG_VALUE). WDYT?
>From a technical PoV, this is very easy to do with the current patch. From a
practical point of view, both cases will require people downstream to update any
DBG_VALUE tests or other direct references to the instruction that they have,
but the feature I see as being most likely to cause issues is the removal of the
Indirectness flag, as it's possible that people are depending on it. I
*believe* that it should be possible to replicate any such behaviour with the
new DBG_VALUE as stated in this RFC, but it's probably safest to keep the
old DBG_VALUE as you suggested in case there are difficulties with this
approach.
________________________________
From: Djordje Todorovic <Djordje.Todorovic at syrmia.com>
Sent: 26 August 2020 08:01
To: llvm-dev at lists.llvm.org <llvm-dev at lists.llvm.org>; Tozer,
Stephen <stephen.tozer at sony.com>
Subject: Re: [llvm-dev] [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to
support multiple location operands

Hi Stephen,

Thanks for working on this.
>In summary, this is a notice of the intent to introduce these changes in the
patch described above. Currently the patches add these modified instructions
alongside the existing ones, but a total replacement would be a better outcome.
This is not a full RFC but is intended to ensure that this change doesn't
catch anyone by surprise and that there are no significant objections.
In my opinion, given this whole picture, total replacement is better approach. 
Furthermore, we can leave the old DBG_VALUE (by renaming it to DBG_VALUE_OLD or
so) to add some time to other folks to completely adjust/switch to the new
DBG_VALUE (e.g. out there might be some downstream features depending on the
DBG_VALUE). WDYT?

Best regards,
Djordje

________________________________
From: llvm-dev <llvm-dev-bounces at lists.llvm.org> on behalf of Tozer,
Stephen via llvm-dev <llvm-dev at lists.llvm.org>
Sent: Tuesday, August 25, 2020 8:09 PM
To: llvm-dev at lists.llvm.org <llvm-dev at lists.llvm.org>
Subject: [llvm-dev] [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to support
multiple location operands

Currently there is a series of patches undergoing review[0] that seek to enable
the use of multiple IR/MIR values when describing a source variable's
location. The current plan for the MIR is to add a new instruction,
DBG_VALUE_LIST, that supports this functionality by having a variable number of
operands. It may be better however to simply replace the existing DBG_VALUE
behaviour entirely instead, and so I'm looking for any comments on this
change before pushing ahead with it.

There are a few differences between the MIR instructions:

Old: DBG_VALUE %x, $noreg, !DILocalVariable("x"), !DIExpression()
New: DBG_VALUE !DILocalVariable("x"), !DIExpression(DW_OP_LLVM_arg, 0,
DW_OP_stack_value), %x

1) The "location" operand is moved to the end, as the instruction is
now variadic such that every operand after the DIExpression is a location
operand.
2) The second operand which currently represents "Indirectness" has
been removed entirely, because this is now explicitly specified in the
DIExpression (see 4).
3) The DIExpression no longer implicitly treats the location operand as the
first element of the expression, instead each location must be explicitly
referenced in the expression using `DW_OP_LLVM_arg, N` for the Nth location
operand.
4) The DIExpression itself must be explicit about whether it evaluates to the
location of a variable or its literal value, by using DW_OP_stack_value in the
latter case (instead of relying on the Indirectness flag, which is both
confusing and redundant[1]).

I believe this is a strict improvement to the expressiveness and clarity of
DBG_VALUE. Although it increases the verbosity of simple expressions, such a
change is necessary to remove potential ambiguities in constant debug
expressions[2]. We will also be relying on the DIExpression to replace the
"Indirectness" flag, since it should now solely determine whether or
not a value is indirect; this brings us closer to the final DWARF
representation. One potential downside is that using DW_OP_stack_value for a
simple single-register DBG_VALUE (as in the example above) would currently lose
information, as it would output the DWARF expression `DW_OP_breg0 RSP+0,
DW_OP_stack_value` instead of the current output `DW_OP_reg0 RSP`. The former is
larger and gives less information, as both expressions evaluate to the same
value but only the latter gives a location for the variable that can be modified
by a debugger. This can be fixed with some pattern matching in the
DwarfExpression class to cover this specific (albeit common) case.

The current approach for the IR is not to add a new instruction, but to add a
new metadata node that contains a list of IR value references (wrapped as
ValueAsMetadata) and use it as the first argument to dbg.value. There is no
syntactic incompatibility between this and the current dbg.value, and therefore
it is possible to support both simultaneously, but I believe it would be
unnecessarily complicated to maintain two separate forms of dbg.value. There is
no immediate plan to change dbg.declare and dbg.addr in the same way: there is
some value in the distinction between the intrinsics, the addresses do not use
constant values (and so avoid the ambiguity described in [2]), and there are few
(possibly no) cases where dbg.addr or dbg.declare intrinsics that use more than
one IR value would actually be produced: only salvageDebugInfo can produce
multi-value debug intrinsics, and debug address intrinsics usually use a
non-salvageable alloca as the location (I am currently unsure as to whether
non-alloca address intrinsics can or should be produced anywhere).

Described here are the differences in the IR intrinsics:

Old: @llvm.dbg.value(metadata i32 %x, metadata !DILocalVariable("x"),
metadata !DIExpression())
New: @llvm.dbg.value(metadata !DIValueList(i32 %x), metadata
!DILocalVariable("x"), metadata !DIExpression(DW_OP_LLVM_arg, 0,
DW_OP_stack_value))

1) The location operand is changed from a single Value to a list of 0 or more
Values.
2) The DIExpression is modified in the same manner as in the MIR instruction
(see above).

In summary, this is a notice of the intent to introduce these changes in the
patch described above. Currently the patches add these modified instructions
alongside the existing ones, but a total replacement would be a better outcome.
This is not a full RFC but is intended to ensure that this change doesn't
catch anyone by surprise and that there are no significant objections.

[0] https://reviews.llvm.org/D82363
[1] https://bugs.llvm.org/show_bug.cgi?id=41675#c8
[2] http://lists.llvm.org/pipermail/llvm-dev/2020-February/139441.html
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200902/dec6d1ce/attachment.html>