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

[Tozer, Stephen via llvm-dev]

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

[Djordje Todorovic via llvm-dev]

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...@lists.llvm.org> on behalf of Tozer, Stephen via llvm-dev <llvm...@lists.llvm.org>
Sent: Tuesday, August 25, 2020 8:09 PM
To: llvm...@lists.llvm.org <llvm...@lists.llvm.org>
Subject: [llvm-dev] [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to support multiple location operands

 

[Adrian Prantl via llvm-dev]

> On Aug 25, 2020, at 11:09 AM, Tozer, Stephen via llvm-dev <llvm...@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...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Tozer, Stephen via 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....@syrmia.com>
Sent: 26 August 2020 08:01
To: llvm...@lists.llvm.org <llvm...@lists.llvm.org>; Tozer, Stephen <stephe...@sony.com>
Subject: Re: [llvm-dev] [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to support multiple location operands

 

[Tozer, Stephen via llvm-dev]

> 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).

This is a tricky one. Right now, DIExpressions sort-of mimic DWARF, but with differences that aren't always immediately clear. The reason why I chose DW_OP_stack_value for the direct-value-case instead of using DW_OP_deref for the indirect-value-case is that it is more like actual DWARF: a DWARF expression is either empty, a register, a memory address, or an implicit location. The new representation handles each of these faithfully to DWARF, except for being unable to distinguish between the register and memory case for a single register argument. In DWARF, the difference is that a register location uses `DW_OP_reg N`, while any reference to a register's value in any other type of location uses `DW_OP_breg N`. We cannot specify these in LLVM since we only generate these operators at the end; previously, this was the job of the indirectness flag.

Rather than reintroducing a flag just for this purpose however, I instead propose that we treat this as a special (albeit common) case: we can use `DW_OP_LLVM_arg 0, DW_OP_stack_value` with a single register operand. We already reduce the DIExpression to specialized DWARF operators at the point of DWARF emission, for example: `<Register N>, DW_OP_plus_uconst 5` becomes `DW_OP_bregN RSP+5`. If in any case where a stack value expression consists of only a single register it is valid to convert it to a register location, then this should be a valid transformation; I can't think of any cases where it wouldn't be, since if we get a variable's value directly from a single register then it necessarily exists at that location. The only exception would be where, for one reason or another, we want DWARF to believe that the location is implicit and thus cannot be written to; if such a case exists then it might be suitable grounds to change the behaviour here.

This does have the potential to cause confusion to a reader unfamiliar with this behaviour, but for a reader examining debug info in enough detail that the removal of DW_OP_stack_value raises an eyebrows, I think simply noting the behaviour in code comments and the documentation would be sufficient.

---

From: Adrian Prantl <apr...@apple.com>
Sent: 27 August 2020 17:42
To: Tozer, Stephen <stephe...@sony.com>
Cc: llvm...@lists.llvm.org <llvm...@lists.llvm.org>

Subject: Re: [llvm-dev] [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to support multiple location operands

> On Aug 25, 2020, at 11:09 AM, Tozer, Stephen via llvm-dev <llvm...@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

[Adrian Prantl via llvm-dev]

On Sep 2, 2020, at 7:01 AM, Tozer, Stephen <stephe...@sony.com> wrote:

> 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).

This is a tricky one. Right now, DIExpressions sort-of mimic DWARF, but with differences that aren't always immediately clear. The reason why I chose DW_OP_stack_value for the direct-value-case instead of using DW_OP_deref for the indirect-value-case is that it is more like actual DWARF: a DWARF expression is either empty, a register, a memory address, or an implicit location.

Yeah, because that decision can only be made much later in LLVM in AsmPrinter/DwarfExpression.cpp.

The new representation handles each of these faithfully to DWARF, except for being unable to distinguish between the register and memory case for a single register argument. In DWARF, the difference is that a register location uses `DW_OP_reg N`, while any reference to a register's value in any other type of location uses `DW_OP_breg N`. We cannot specify these in LLVM since we only generate these operators at the end; previously, this was the job of the indirectness flag.

In DWARF, DW_OP_reg(x) is a register l-value, all others can either be l-values or r-values depending on whether there is a DW_OP_stack_value/DW_OP_implicit* at the end. 

Rather than reintroducing a flag just for this purpose however, I instead propose that we treat this as a special (albeit common) case: we can use `DW_OP_LLVM_arg 0, DW_OP_stack_value` with a single register operand.

I think it would be confusing to talk about registers at the LLVM IR / DIExpression level. "SSA-Values"?

We already reduce the DIExpression to specialized DWARF operators at the point of DWARF emission, for example: `<Register N>, DW_OP_plus_uconst 5` becomes `DW_OP_bregN RSP+5`. If in any case where a stack value expression consists of only a single register it is valid to convert it to a register location,

I don't think that's correct, because a DW_OP_stack_value is an rvalue. But maybe I misunderstood what you were trying to say.

then this should be a valid transformation; I can't think of any cases where it wouldn't be, since if we get a variable's value directly from a single register then it necessarily exists at that location. The only exception would be where, for one reason or another, we want DWARF to believe that the location is implicit and thus cannot be written to; if such a case exists then it might be suitable grounds to change the behaviour here.

This does have the potential to cause confusion to a reader unfamiliar with this behaviour, but for a reader examining debug info in enough detail that the removal of DW_OP_stack_value raises an eyebrows, I think simply noting the behaviour in code comments and the documentation would be sufficient.

We should start be defining what DW_OP_stack_value really means in LLVM debug info metadata. I believe it should just mean "r-value".

-- adrian

[Tozer, Stephen via llvm-dev]

> Yeah, because that decision can only be made much later in LLVM in AsmPrinter/DwarfExpression.cpp.

> In DWARF, DW_OP_reg(x) is a register l-value, all others can either be l-values or r-values depending on whether there is a DW_OP_stack_value/DW_OP_implicit* at the end.

Yes, it might not be clear but that's what I'm trying to say. Out of the non-empty DWARF locations, register and memory locations are l-values, implicit locations are r-values. You can technically use DW_OP_breg in an l-value, but not for register locations. This is why when we have a DBG_VALUE that has a single register location operand with an otherwise empty DIExpression, we need some indicator to determine whether we want to produce the register location [DW_OP_reg] or the memory location [DW_OP_breg] (currently this indicator is the indirectness flag).

> I think it would be confusing to talk about registers at the LLVM IR / DIExpression level. "SSA-Values"?

I think terminology is a bit difficult here because this work concerns both the llvm.dbg.value intrinsic and the DBG_VALUE instruction, which operate on different kinds of arguments. I think "location operands" is probably the best description for them, since they are operands to a DIExpression which is used to compute the variable location.

> I don't think that's correct, because a DW_OP_stack_value is an rvalue. But maybe I misunderstood what you were trying to say.

> We should start be defining what DW_OP_stack_value really means in LLVM debug info metadata. I believe it should just mean "r-value".

Having given it some more thought, I've changed my mind - I agree that we shouldn't use DW_OP_stack_value in this case, because it would be changing its meaning which is to explicitly declare the expression to be an implicit location/r-value. My current line of thinking is that it would be better to introduce a new operator, named DW_OP_LLVM_direct or something similar, which has the meaning "the variable's exact value is produced by the preceding expression", and would replace DW_OP_stack_value as it is currently used within LLVM.

To summarise the logic behind using this operator: LLVM debug info does not need to explicitly care about r-values or l-values before DWARF emission, only whether we're describing a variable's memory location, a variable's exact value, or some other implicit location (such as implicit_pointer). Whether an expression is an r-value or l-value can be trivially determined at the end of the pipeline (addMachineRegExpression already does this).

For an expression ending with DW_OP_LLVM_direct: if the preceding expression is only a single register then we emit a register location, if the preceding expression ends with DW_OP_deref then we can remove the deref and emit a memory location, and otherwise we emit the expression with DW_OP_stack_value. In expression syntax it would behave like an implicit operator, in that it can only appear at the end of an expression and is incompatible with any implicit operators, including DW_OP_stack_value.

The alternative I see for this is using a flag or a new DIExpression operator that explicitly declares a single register DBG_VALUE to be a register location, while it would otherwise be treated as a memory location, and use stack_value for all other cases. The main reason I prefer the "direct" operator is that LLVM doesn't need to know whether a DIExpression results in an l-value location or an r-value location; it only needs to know how to compute the variable's location and then determine whether that computation resolves to an l-value or r-value at the end. Maintaining two separate representations for stack value locations and register locations when we don't need to is an unnecessary burden, especially when it may be possible for a given dbg.value/DBG_VALUE to switch back and forth between them.

[Adrian Prantl via llvm-dev]

> On Sep 4, 2020, at 3:00 AM, Tozer, Stephen <stephe...@sony.com> wrote:
>
> > Yeah, because that decision can only be made much later in LLVM in AsmPrinter/DwarfExpression.cpp.
> > In DWARF, DW_OP_reg(x) is a register l-value, all others can either be l-values or r-values depending on whether there is a DW_OP_stack_value/DW_OP_implicit* at the end.
>
> Yes, it might not be clear but that's what I'm trying to say. Out of the non-empty DWARF locations, register and memory locations are l-values, implicit locations are r-values. You can technically use DW_OP_breg in an l-value, but not for register locations. This is why when we have a DBG_VALUE that has a single register location operand with an otherwise empty DIExpression, we need some indicator to determine whether we want to produce the register location [DW_OP_reg] or the memory location [DW_OP_breg] (currently this indicator is the indirectness flag).
>
> > I think it would be confusing to talk about registers at the LLVM IR / DIExpression level. "SSA-Values"?
>
> I think terminology is a bit difficult here because this work concerns both the llvm.dbg.value intrinsic and the DBG_VALUE instruction, which operate on different kinds of arguments. I think "location operands" is probably the best description for them, since they are operands to a DIExpression which is used to compute the variable location.
>
> > I don't think that's correct, because a DW_OP_stack_value is an rvalue. But maybe I misunderstood what you were trying to say.
> > We should start be defining what DW_OP_stack_value really means in LLVM debug info metadata. I believe it should just mean "r-value".
>
> Having given it some more thought, I've changed my mind - I agree that we shouldn't use DW_OP_stack_value in this case, because it would be changing its meaning which is to explicitly declare the expression to be an implicit location/r-value. My current line of thinking is that it would be better to introduce a new operator, named DW_OP_LLVM_direct or something similar, which has the meaning "the variable's exact value is produced by the preceding expression", and would replace DW_OP_stack_value as it is currently used within LLVM.

Can you elaborate what "direct" means? I'm having trouble understanding what the opposite (a non-exact value) would be.

>
> To summarise the logic behind using this operator: LLVM debug info does not need to explicitly care about r-values or l-values before DWARF emission,

I don't think that statement is correct. Based on the semantics, LLVM IR knows that a dbg.declare is an l-value — the debugger can write to it and the value will be changed when continuing the program execution. It can also decide that a "working copy" of the value, described by a dbg.value is a legit read-only representation of the variable, but can't be written to because, e.g., the value exists in more than one place at once.

At the moment we don't make the lvalue/rvalue distinction in LLVM at all. We make an educated guess in AsmPrinter. But that's wrong and something we should strive to fix during this redesigning.

> only whether we're describing a variable's memory location, a variable's exact value, or some other implicit location (such as implicit_pointer). Whether an expression is an r-value or l-value can be trivially determined at the end of the pipeline (addMachineRegExpression already does this).

As stated above, I don't think we can trivially determine this, because (at least for dbg.values) this info was lost already in LLVM IR. Unless we say the dbg.declare / dbg.value distinction is what determines lvalues vs. rvalues.

>
> For an expression ending with DW_OP_LLVM_direct: if the preceding expression is only a single register then we emit a register location, if the preceding expression ends with DW_OP_deref then we can remove the deref and emit a memory location, and otherwise we emit the expression with DW_OP_stack_value. In expression syntax it would behave like an implicit operator, in that it can only appear at the end of an expression and is incompatible with any implicit operators, including DW_OP_stack_value.
>
> The alternative I see for this is using a flag or a new DIExpression operator that explicitly declares a single register DBG_VALUE to be a register location, while it would otherwise be treated as a memory location, and use stack_value for all other cases. The main reason I prefer the "direct" operator is that LLVM doesn't need to know whether a DIExpression results in an l-value location or an r-value location; it only needs to know how to compute the variable's location and then determine whether that computation resolves to an l-value or r-value at the end. Maintaining two separate representations for stack value locations and register locations when we don't need to is an unnecessary burden, especially when it may be possible for a given dbg.value/DBG_VALUE to switch back and forth between them.

I do think that your insight that we need one (or more?) additional discriminator of some kind is correct — we just need to find the right semantics for it.

thanks,
adrian

[Tozer, Stephen via llvm-dev]

> Can you elaborate what "direct" means? I'm having trouble understanding what the opposite (a non-exact value) would be.

Apologies, "exact" was a misleading/incorrect term. By direct, I mean that the expression computes the value of the variable, as opposed to its memory address, or the value that it points to. Within LLVM, where we don't have DW_OP_reg/DW_OP_breg but instead simply refer to a generic SSA value, this could mean either a register location or stack value.

> At the moment we don't make the lvalue/rvalue distinction in LLVM at all. We make an educated guess in AsmPrinter. But that's wrong and something we should strive to fix during this redesigning.

I think the opposite; I don't believe there's any reason we need to make the explicit lvalue/rvalue distinction until we're writing DWARF. To put it in more general terms, I think that the IR/MIR debug value instructions should only care about how the variable's value can be computed. Whether the result of that computation is an lvalue is unimportant within LLVM itself as far as I can tell, and is redundant when it can be computed from just the DIExpression and location operands.

>As stated above, I don't think we can trivially determine this, because (at least for dbg.values) this info was lost already in LLVM IR. Unless we say the dbg.declare / dbg.value distinction is what determines lvalues vs. rvalues.

With the proposed operator, it would be trivial to determine lvalue vs rvalue debug values with a set of rules (ignoring any fragment operator, which may appear at the end but does not affect the location type):

  1. If the expression is empty, or any location arguments are $noreg => Empty
  2. If the expression ends with DW_OP_implicit_ptr => Implicit pointer (rvalue)
  3. If the expression ends with DW_OP_stack_value =>Stack value (rvalue)  // LLVM should produce LLVM_direct instead.
  4. If the expression ends with DW_OP_LLVM_direct, then...
    4a. If the preceding expression is just DW_OP_LLVM_arg, 0 and the only location operand is a register => Register location (lvalue)
    4b. Otherwise => Stack value (rvalue)

  5. Otherwise => Memory location (lvalue)

This covers all the expected cases without ambiguity or almost any reduced expressiveness. I believe that the only expression that LLVM will not be able to produce like this is DW_OP_bregN, DW_OP_stack_value due to fact that when DW_OP_LLVM_direct is used, this would be written as a register location instead of a stack value. I don't think there are any cases where we would choose to emit a stack value location when we're able to produce a register location instead, so this shouldn't be a problem.

[Adrian Prantl via llvm-dev]

On Sep 11, 2020, at 11:12 AM, Tozer, Stephen <stephe...@sony.com> wrote:

> Can you elaborate what "direct" means? I'm having trouble understanding what the opposite (a non-exact value) would be.

Apologies, "exact" was a misleading/incorrect term. By direct, I mean that the expression computes the value of the variable, as opposed to its memory address, or the value that it points to.

That sounds to me to be the same concept that I am calling rvalue vs. lvalue. Do you agree, or is there some subtlety that I am missing?

Within LLVM, where we don't have DW_OP_reg/DW_OP_breg but instead simply refer to a generic SSA value, this could mean either a register location or stack value.

> At the moment we don't make the lvalue/rvalue distinction in LLVM at all. We make an educated guess in AsmPrinter. But that's wrong and something we should strive to fix during this redesigning.

I think the opposite; I don't believe there's any reason we need to make the explicit lvalue/rvalue distinction until we're writing DWARF.

Here is an example of why I think that an optimization pass must have the ability to downgrade an lvalue to an rvalue:

; BEFORE

%foo = i32 ...

%mem = alloca i32

call %llvm.dbg.declare(%mem, !DILocalVariable("x"))

store i32* %mem, i32 %foo

...

store i32* %mem, i32 0

...

store i32* %mem, i32 %foo

...

; AFTER

%foo = i32 ...

%mem = alloca i32

call %llvm.dbg.value(%mem, !DILocalVariable("x"), !DIExpression(DW_OP_deref))

store i32* %mem, i32 %foo

; optimization eliminated the store of 0 to %mem and replaced all loads of %mem with "i32 0".

call %llvm.dbg.value(%mem, !DILocalVariable("x"), !DIExpression(DW_OP_constu 0, DW_OP_stack_value))

...

call %llvm.dbg.value(%mem, !DILocalVariable("x"), !DIExpression(DW_OP_deref))

...

The optimization eliminated the store of constant 0 to %mem and replaced all loads of %mem in the subsequent block with a constant "i32 0". This means that we need mark the first dbg.value (that would otherwise look like an lvalue) as an rvalue, because writing a new value to %mem there would not affect the code that is now hardcoded to use a constant 0 value for "x".

what do you think?

-- adrian

[Tozer, Stephen via llvm-dev]

> That sounds to me to be the same concept that I am calling r-value vs. l-value. Do you agree, or is there some subtlety that I am missing?

I've been assuming that the l-value vs r-value distinction is analogous to C++: an l-value can be written to by the debugger, an r-value cannot. A memory location and a register location are both l-values, while implicit locations (stack value/implicit pointer) are r-values. Directness on the other hand, I've been using to mean "the variable's value is equal to the value computed by the DIExpression". Applied to a DWARF expression this description would only refer to a stack value. LLVM's DIExpressions are different however, because we don't have DW_OP_reg or DW_OP_breg: we simply refer to the register and use context to determine which it should be. The best example of this is from this example[0], expanded on here with the location type:

$noreg, (plus_uconst, 8),                -> DW_OP_breg7 RSP+8)                    Memory    l-value  Indirect

     0, (plus_uconst, 8),                -> DW_OP_breg7 RSP+8)                    Memory    l-value  Indirect

$noreg, (plus_uconst, 8, stackval),      -> DW_OP_breg7 RSP+8, stackval           Stack     r-value  Direct

     0, (plus_uconst, 8, stackval),      -> DW_OP_breg7 RSP+8, stackval           Stack     r-value  Direct

$noreg, (plus_uconst, 8, deref),         -> DW_OP_breg7 RSP+8                     Memory    l-value  Indirect

     0, (plus_uconst, 8, deref),         -> DW_OP_breg7 RSP+8, deref              Memory    l-value  Indirect

$noreg, (plus_uconst, 8, deref, stackval)-> DW_OP_breg7 RSP+8, deref, stackval    Stack     r-value  Direct

     0, (plus_uconst, 8, deref, stackval)-> DW_OP_breg7 RSP+8, deref, stackval    Stack     r-value  Direct

$noreg, (),                              -> DW_OP_reg7 RSP                        Register  l-value  Direct

     0, (),                              -> DW_OP_breg7 RSP+0                     Memory    l-value  Indirect

$noreg, (deref),                         -> DW_OP_breg7 RSP+0                     Memory    l-value  Indirect

     0, (deref),                         -> DW_OP_breg7 RSP+0, DW_OP_deref        Memory    l-value  Indirect

The point of using DW_OP_LLVM_direct is that it supplants the current directness flag, without the redundancy or unintuitive behaviour that the flag does. I believe the only reason that the indirectness flag is necessary right now is to allow register locations to be emitted, by delineating the register location "$rsp, $noreg, ()" -> DW_OP_reg7 RSP" from the memory location "$rsp, 0, () -> DW_OP_breg7 RSP+0". Outside of this case the existing representation is sufficient for all other locations, and ideally the indirectness flag would have no effect (although unfortunately it does). The justification for DW_OP_LLVM_direct rests on the idea that we will generally choose to produce "DW_OP_reg7 RSP" instead of "DW_OP_breg7 RSP, DW_OP_stack_value". It doesn't prevent us from using DW_OP_stack_value instead if we have an exception, but I don't believe there are any. Using DW_OP_LLVM_direct instead of directness and stackval for the table above, we get this:

(plus_uconst, 8),                     -> DW_OP_breg7 RSP+8                     Memory    l-value  Indirect

(plus_uconst, 8, LLVM_direct),        -> DW_OP_breg7 RSP+8, stackval           Stack     r-value  Direct

(plus_uconst, 8, deref),              -> DW_OP_breg7 RSP+8, deref              Memory    l-value  Indirect

(plus_uconst, 8, deref, LLVM_direct), -> DW_OP_breg7 RSP+8, deref, stackval    Stack     r-value  Direct

(),                                   -> DW_OP_breg7 RSP+0                     Memory    l-value  Inirect

(LLVM_direct),                        -> DW_OP_reg7 RSP                        Register  l-value  Direct

(deref),                              -> DW_OP_breg7 RSP+0, deref              Memory    l-value  Indirect

(deref, LLVM_direct),                 -> DW_OP_breg7 RSP+0, deref, stackval    Stack     r-value  Direct

Two of the examples in this table should be excluded from actual use: the two rows that end with "deref, LLVM_direct" shouldn't be produced within LLVM, because we can cancel the two operators out to give a memory location rather than producing a "deref, stackval" expression. This can be done in LLVM itself through the DIExpression interface, so that we don't hold DIExpressions in an incorrect intermediate state. I'm currently operating under the belief that if a dbg.value can be an l-value, it always should be; if not, then we can use DW_OP_stack_value instead in all cases where we require a given dbg.value to be an r-value.

To give an example of why having this could be more useful than just applying stack value, consider a hypothetical "DIExpression optimizer" pass applied to the following code:

    // `int a` is live...

    int b = a + 5;

    int c = b - 5;

If both b and c are optimized out and salvaged, then we end up with the following dbg.values:

    @llvm.dbg.value(i32 %a, !"b", !DIExpression(DW_OP_plus_uconst, 5, DW_OP_LLVM_direct))

    @llvm.dbg.value(i32 %a, !"c", !DIExpression(DW_OP_plus_uconst, 5, DW_OP_constu, 5, DW_OP_minus, DW_OP_LLVM_direct))

    ; DIExpressions optimized...

    @llvm.dbg.value(i32 %a, !"b", !DIExpression(DW_OP_plus_uconst, 5, DW_OP_LLVM_direct))

    @llvm.dbg.value(i32 %a, !"c", !DIExpression(DW_OP_LLVM_direct))

In this admittedly strange case, we start with b and c as l-values (before they are optimized out), they then become r-values due to optimization, and finally c is a valid l-value again. If we instead applied DW_OP_stack_value when we salvage, then c would not be recovered as an l-value. If we had DW_OP_implicit_ptr instead of DW_OP_LLVM_direct, then the result would be an r-value either way; likewise if we were referencing a memory location, the result would be an l-value regardless of how we modified it.

I suspect there may be disagreements over whether c should share an l-value location with a, since this means that a user could write to either c or a, and that doing so would assign to both of them. My personal belief is that even if it seems confusing, we shouldn't arbitrarily restrict write-access to variables on criteria that will not always be clear to a debug user; whether or not to apply such a restriction should be left to the debugger, rather than being baked into the information we produce for it. Personal opinions aside, the other reason I'm taking this approach right now is that it matches LLVM's existing behaviour. If we have the source code:

    int a = ...

    int b = a;

We will produce the IR:

    call void @llvm.dbg.value(metadata i32 %a, metadata !13, metadata !DIExpression()), !dbg !15

    call void @llvm.dbg.value(metadata i32 %a, metadata !14, metadata !DIExpression()), !dbg !15

This IR will in turn produce register locations for both variables at the same location. Based on that, I believe that the current expected behaviour is that if two source variables map to the same actual location then they should share a DWARF location.

> Here is an example of why I think that an optimization pass must have the ability to downgrade an l-value to an r-value:

> ...

> The optimization eliminated the store of constant 0 to %mem and replaced all loads of %mem in the subsequent block with a constant "i32 0". This means that we need mark the first dbg.value (that would otherwise look like an l-value) as an r-value, because writing a new value to %mem there would not affect the code that is now hardcoded to use a constant 0 value for x.

The behaviour seen by the user in this case is:

; BEFORE

call %llvm.dbg.declare(%mem, !DILocalVariable("x"))

store i32* %mem, i32 %foo

; "x" is set to %foo, and can be written to

...

store i32* %mem, i32 0

; "x" is set to 0, and can be written to

...

store i32* %mem, i32 %foo

; "x" is set to %foo, and can be written to

...

; AFTER

call %llvm.dbg.value(%mem, !DILocalVariable("x"), !DIExpression(DW_OP_deref))

store i32* %mem, i32 %foo

; "x" is set to %foo, and can be written to

...

call %llvm.dbg.value(%mem, !DILocalVariable("x"), !DIExpression(DW_OP_constu 0, DW_OP_stack_value))

; "x" is set to %foo, and is read-only

...

call %llvm.dbg.value(%mem, !DILocalVariable("x"), !DIExpression(DW_OP_deref))

; "x" is set to its value prior to 0, and can be written to

...

I don't think there's any issue with a variable being an r-value at some points in its live range and an l-value at others; in this case I think it's correct that the first dbg.value should be an l-value. Any write to x will not affect the code after the eliminated store, but even without optimizations x would be set to 0 (overriding any debugger assignment) at that point anyway. I do agree that the code produced may be slightly confusing to a user; this code likely maps to something along the lines of:

    int x = foo;

    ...

    x = 0;

    ...

    x = foo;

If the user breaks just after the first "x = foo" and assigns `x = 5` in the debugger, they will see the correct result for all subsequent uses of x until the next assignment to x. When they step over "x = 0", x has the value 0 (as expected) and it becomes read-only. Finally after stepping over the next "x = foo" they will see that `x == 5`, which might not make a lot of sense when the user is expecting it to be assigned a different value. Even so, this information is a correct representation of the program state.

The alternative of making the first dbg.value an r-value is restrictive - if the initial assignment to x is at the top of a large function that the user is debugging, and the other two assignments occur at the very end, it would likely be frustrating to the user that they have no write-access to x throughout the function. Because of this I don't believe that it would be right to make the first dbg.value an r-value; I think again that choosing to apply these restrictions should be left to the debugger.

[0] https://bugs.llvm.org/show_bug.cgi?id=41675#c8

[Adrian Prantl via llvm-dev]

On Sep 15, 2020, at 10:56 AM, Tozer, Stephen <stephe...@sony.com> wrote:

> That sounds to me to be the same concept that I am calling r-value vs. l-value. Do you agree, or is there some subtlety that I am missing?

I've been assuming that the l-value vs r-value distinction is analogous to C++: an l-value can be written to by the debugger, an r-value cannot.

Up to here, I fully agree!

A memory location and a register location are both l-values, while implicit locations (stack value/implicit pointer) are r-values.

Here, however I think I need to to do a better job at explaining my thoughts ;-)

You are correct to point out that all l-values must be memory or register locations, since they need to be something writeable. An l-value can't be a constant since you can't write to a constant.

But — not all memory locations are l-values. If we have a DWARF location list for variable "x" which points to a memory address for the first n instructions and the switches to a constant for the remainder of the scope, the memory address is not guaranteed to be an l-value, because writing the the memory address cannot affect the later part of the function where the variable is a constant.

Directness on the other hand, I've been using to mean "the variable's value is equal to the value computed by the DIExpression".

as opposed to "the value computed by the expression is (the address) of the variable's storage"?

That makes sense, and I think for "direct" values in your definition it is true that all direct values are r-values.

Applied to a DWARF expression this description would only refer to a stack value. LLVM's DIExpressions are different however, because we don't have DW_OP_reg or DW_OP_breg: we simply refer to the register and use context to determine which it should be. The best example of this is from this example[0], expanded on here with the location type:

$noreg, (plus_uconst, 8),                -> DW_OP_breg7 RSP+8)                    Memory    l-value  Indirect

     0, (plus_uconst, 8),                -> DW_OP_breg7 RSP+8)                    Memory    l-value  Indirect

$noreg, (plus_uconst, 8, stackval),      -> DW_OP_breg7 RSP+8, stackval           Stack     r-value  Direct

     0, (plus_uconst, 8, stackval),      -> DW_OP_breg7 RSP+8, stackval           Stack     r-value  Direct

$noreg, (plus_uconst, 8, deref),         -> DW_OP_breg7 RSP+8                     Memory    l-value  Indirect

     0, (plus_uconst, 8, deref),         -> DW_OP_breg7 RSP+8, deref              Memory    l-value  Indirect

$noreg, (plus_uconst, 8, deref, stackval)-> DW_OP_breg7 RSP+8, deref, stackval    Stack     r-value  Direct

     0, (plus_uconst, 8, deref, stackval)-> DW_OP_breg7 RSP+8, deref, stackval    Stack     r-value  Direct

$noreg, (),                              -> DW_OP_reg7 RSP                        Register  l-value  Direct

is this strictly an r-value?

     0, (),                              -> DW_OP_breg7 RSP+0                     Memory    l-value  Indirect

$noreg, (deref),                         -> DW_OP_breg7 RSP+0                     Memory    l-value  Indirect

     0, (deref),                         -> DW_OP_breg7 RSP+0, DW_OP_deref        Memory    l-value  Indirect

The point of using DW_OP_LLVM_direct is that it supplants the current directness flag, without the redundancy or unintuitive behaviour that the flag does.

Why do we need DW_OP_LLVM_direct when we already have DW_OP_LLVM_stack_value? Can you give an example of something that is definitely not a stack value, but direct?

I believe the only reason that the indirectness flag is necessary right now is to allow register locations to be emitted, by delineating the register location "$rsp, $noreg, ()" -> DW_OP_reg7 RSP" from the memory location "$rsp, 0, () -> DW_OP_breg7 RSP+0". Outside of this case the existing representation is sufficient for all other locations, and ideally the indirectness flag would have no effect (although unfortunately it does). The justification for DW_OP_LLVM_direct rests on the idea that we will generally choose to produce "DW_OP_reg7 RSP" instead of "DW_OP_breg7 RSP, DW_OP_stack_value". It doesn't prevent us from using DW_OP_stack_value instead if we have an exception, but I don't believe there are any. Using DW_OP_LLVM_direct instead of directness and stackval for the table above, we get this:

(plus_uconst, 8),                     -> DW_OP_breg7 RSP+8                     Memory    l-value  Indirect

(plus_uconst, 8, LLVM_direct),        -> DW_OP_breg7 RSP+8, stackval           Stack     r-value  Direct

(plus_uconst, 8, deref),              -> DW_OP_breg7 RSP+8, deref              Memory    l-value  Indirect

(plus_uconst, 8, deref, LLVM_direct), -> DW_OP_breg7 RSP+8, deref, stackval    Stack     r-value  Direct

(),                                   -> DW_OP_breg7 RSP+0                     Memory    l-value  Inirect

(LLVM_direct),                        -> DW_OP_reg7 RSP                        Register  l-value  Direct

(deref),                              -> DW_OP_breg7 RSP+0, deref              Memory    l-value  Indirect

(deref, LLVM_direct),                 -> DW_OP_breg7 RSP+0, deref, stackval    Stack     r-value  Direct

Two of the examples in this table should be excluded from actual use: the two rows that end with "deref, LLVM_direct" shouldn't be produced within LLVM, because we can cancel the two operators out to give a memory location rather than producing a "deref, stackval" expression. This can be done in LLVM itself through the DIExpression interface, so that we don't hold DIExpressions in an incorrect intermediate state. I'm currently operating under the belief that if a dbg.value can be an l-value, it always should be; if not, then we can use DW_OP_stack_value instead in all cases where we require a given dbg.value to be an r-value.

To give an example of why having this could be more useful than just applying stack value, consider a hypothetical "DIExpression optimizer" pass applied to the following code:

    // `int a` is live...

    int b = a + 5;

    int c = b - 5;

If both b and c are optimized out and salvaged, then we end up with the following dbg.values:

    @llvm.dbg.value(i32 %a, !"b", !DIExpression(DW_OP_plus_uconst, 5, DW_OP_LLVM_direct))

    @llvm.dbg.value(i32 %a, !"c", !DIExpression(DW_OP_plus_uconst, 5, DW_OP_constu, 5, DW_OP_minus, DW_OP_LLVM_direct))

    ; DIExpressions optimized...

    @llvm.dbg.value(i32 %a, !"b", !DIExpression(DW_OP_plus_uconst, 5, DW_OP_LLVM_direct))

    @llvm.dbg.value(i32 %a, !"c", !DIExpression(DW_OP_LLVM_direct))

In this admittedly strange case, we start with b and c as l-values (before they are optimized out), they then become r-values due to optimization, and finally c is a valid l-value again.

But is that safe? If we wrote to %a, yes we will modify "c", but wouldn't we also unintentionally modify "a", and "b"? It sounds more like we'd want to have an rvalue in this case.

If we instead applied DW_OP_stack_value when we salvage, then c would not be recovered as an l-value. If we had DW_OP_implicit_ptr instead of DW_OP_LLVM_direct, then the result would be an r-value either way; likewise if we were referencing a memory location, the result would be an l-value regardless of how we modified it.

I suspect there may be disagreements over whether c should share an l-value location with a, since this means that a user could write to either c or a, and that doing so would assign to both of them. My personal belief is that even if it seems confusing, we shouldn't arbitrarily restrict write-access to variables on criteria that will not always be clear to a debug user; whether or not to apply such a restriction should be left to the debugger, rather than being baked into the information we produce for it.

Ah — you've thought about this already :-)

This is actually really important to me: My take is that we must always err on the safe side. If the compiler cannot prove that it is safe to write to an l-value (ie., that the semantics are the same as if the write were inserted into the source code at that source location) it must downgrade the l-value to an r-value. That's one thing I'm feeling really strongly about.

The reason behind this is that if we allow debug info that is only maybe correct (or correct on some paths, or under some other circumstances that are opaque to the user) — if we allow potentially incorrect debug info to leak into the program, the user can not distinguish between information they can trust and information that is bogus. That means all information that they see in the debugger is potentially bogus. If the debugger is not trustworthy it's worthless. That is feedback I keep getting particularly from kernel developers, and I must say it resonates with me.

-- adrian

I don't think there's any issue with a variable being an r-value at some points in its live range and an l-value at others; in this case I think it's correct that the first dbg.value should be an l-value. Any write tox will not affect the code after the eliminated store, but even without optimizations x would be set to 0 (overriding any debugger assignment) at that point anyway. I do agree that the code produced may be slightly confusing to a user; this code likely maps to something along the lines of:

[Robinson, Paul via llvm-dev]

Hi Adrian & Stephen,

 

One thought here:

 

But — not all memory locations are l-values. If we have a DWARF location list for variable "x" which points to a memory address for the first n instructions and the switches to a constant for the remainder of the scope, the memory address is not guaranteed to be an l-value, because writing the the memory address cannot affect the later part of the function where the variable is a constant.

 

A very interesting point.  I believe this is a concept that LLVM does not yet understand?  that it needs to consider the l/r-value-ness of the variable throughout the function, before deciding on the form that the location-expressions will take.  I would not want to spend a lot of time parsing expressions in order to make this kind of decision.

 

And a second thought here:

 

I suspect there may be disagreements over whether c should share an l-value location with a, since this means that a user could write to either c or a, and that doing so would assign to both of them. My personal belief is that even if it seems confusing, we shouldn't arbitrarily restrict write-access to variables on criteria that will not always be clear to a debug user; whether or not to apply such a restriction should be left to the debugger, rather than being baked into the information we produce for it.

 

Ah — you've thought about this already :-)

 

This is actually really important to me: My take is that we must always err on the safe side. If the compiler cannot prove that it is safe to write to an l-value (ie., that the semantics are the same as if the write were inserted into the source code at that source location) it must downgrade the l-value to an r-value. That's one thing I'm feeling really strongly about.

The reason behind this is that if we allow debug info that is only maybe correct (or correct on some paths, or under some other circumstances that are opaque to the user) — if we allow potentially incorrect debug info to leak into the program, the user can not distinguish between information they can trust and information that is bogus. That means all information that they see in the debugger is potentially bogus. If the debugger is not trustworthy it's worthless. That is feedback I keep getting particularly from kernel developers, and I must say it resonates with me.

 

I should say that Stephen and I had a discussion a while back, where I said IMO the compiler should emit debug info that correctly reflects the code as-it-is, and not arbitrarily decide that ‘c’ and/or ‘a’ should not be writeable.  I believe I also said this was arguable, but that I felt pretty strongly about it.

 

I have to say, it did not occur to me to consider emitting info that showed *neither* ‘c’ nor ‘a’ to be writeable!  This is a kind of protect-the-debugger-from-the-user perspective that I’ve not encountered before.  I would actually argue that showing both variables as writeable is in fact correct (truthful), because it reflects the instructions as-produced; if you modify one, you necessarily modify the other.  That’s what the code as-produced does.

I accept that is not what the code as-originally-written does… and that it is unlikely that the debugger would go to the trouble to notice that two variables share a storage location and caution the user about that unexpected interaction.  (It’s hard enough to persuade the compiler to notice…)  Given that engineering practicality, and that producing debug info that helps the user without confusing/dismaying the user is a valuable goal, then I can agree that describing both variables as r-values is more helpful.

 

In short, I agree with your conclusion, although not for your reasons. 😊

--paulr

 

[Adrian Prantl via llvm-dev]

On Sep 15, 2020, at 3:07 PM, Robinson, Paul <paul.r...@sony.com> wrote:

Hi Adrian & Stephen,

 

One thought here:

 

But — not all memory locations are l-values. If we have a DWARF location list for variable "x" which points to a memory address for the first n instructions and the switches to a constant for the remainder of the scope, the memory address is not guaranteed to be an l-value, because writing the the memory address cannot affect the later part of the function where the variable is a constant.

 

A very interesting point.  I believe this is a concept that LLVM does not yet understand?  that it needs to consider the l/r-value-ness of the variable throughout the function, before deciding on the form that the location-expressions will take.  I would not want to spend a lot of time parsing expressions in order to make this kind of decision.

That's right. Stephen correctly identified that we need some new concept at the LLVM IR level. My hypothesis is that DW_OP_stack_value — if applied correctly — is sufficient to represent this. Stephen is making a case that we could get out more by having a separate flag but I'm not (yet?) convinced that that is in the end-user's best interest. So if we can avoid having two flags for confusingly similar semantics I would like to go with the simpler IR.

-- adrian

[Tozer, Stephen via llvm-dev]

> That makes sense, and I think for "direct" values in your definition it is true that all direct values are r-values.

> Why do we need DW_OP_LLVM_direct when we already have DW_OP_LLVM_stack_value? Can you give an example of something that is definitely not a stack value, but direct?

The difference in definition is the intention: DW_OP_LLVM_direct means "we'd like this to be an l-value if possible", DW_OP_stack_value means "this should never be an l-value". Because of this, an expression ending with DW_OP_LLVM_direct can be emitted as an l-value in any case where the value of the preceding expression is equal to an l-value. So for example:

  DBG_VALUE $rsp, !"x", !DIExpression(DW_OP_LLVM_direct) => DW_OP_reg7 RSP

  DBG_VALUE $rsp, !"x", !DIExpression(DW_OP_deref, DW_OP_LLVM_direct) => DW_OP_breg7 RSP+0

  DBG_VALUE $rsp, !"x", !DIExpression(DW_OP_plus_uconst, 4, DW_OP_LLVM_direct) => DW_OP_breg7 RSP+4, DW_OP_stack_value

Your point about the semantics of variable assignments in the debugger is useful, that clears up my misunderstandings. I believe that even with that in mind, LLVM_direct (or whatever name it takes) would be appropriate. If we recognize that a variable must be read-only to preserve those semantics, then we can use DW_OP_stack_value to ensure that it is always an r-value. If we don't have any reason to make a variable read-only other than that we can't *currently* find an l-value location for it, then we would use DW_OP_LLVM_direct. Right now we use DW_OP_stack_value whenever we make a complex expression, but that doesn't need to be the case.

The code below is an example program where we may eventually be able to generate a valid l-value for the variable "a" in foo(), but can't without an alternative to DW_OP_stack_value. At the end of the example, "a" is an r-value, but doesn't need to be: there is a single register that holds its exact value, and an assignment to that register would have the same semantics as an equivalent assignment to "a" in the source. The optimizations taking place in this code are analogous to if we had "a = bar() + 4 - 4;", but because we don't figure out that "a = bar()" in a single pass, we pre-emptively assume that "a" must be an r-value.

To be able to emit an l-value we would first need the ability to optimize/simplify DIExpressions so that the expression becomes just (DW_OP_stack_value) - this wouldn't be particularly difficult to implement for simple arithmetic. Even with this improvement, the definition of DW_OP_stack_value explicitly forbids the expression from being a register location. If we instead used DW_OP_LLVM_direct, then we would be free to emit the register location (DW_OP_reg0 RAX).

  // Compile with clang -O2 -g

  int baz();

  int bar2(int arg) {

    return arg * 4;

  }

  int bar() {

    return bar2(1);

  }

  int foo() {

    int a = baz() + bar() - 4;

    return a * 2;

  }

; Eventually becomes the IR...

  %call = call i32 @_Z3bazv(), !dbg !25

  %call1 = call i32 @_Z3barv(), !dbg !26

  %add = add nsw i32 %call, %call1, !dbg !27

  %sub = sub nsw i32 %add, 4, !dbg !28

  call void @llvm.dbg.value(metadata i32 %sub, metadata !24, metadata !DIExpression()), !dbg !29

  %mul = mul nsw i32 %sub, 2, !dbg !30

  ret i32 %mul, !dbg !31

; Combine redundant instructions, "a" is salvaged...

  %call = call i32 @_Z3bazv(), !dbg !25

  %call1 = call i32 @_Z3barv(), !dbg !26

  %add = add nsw i32 %call, %call1, !dbg !27

  call void @llvm.dbg.value(metadata i32 %add, metadata !24, metadata !DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_stack_value)), !dbg !28

  %sub = shl i32 %add, 1, !dbg !29

  %mul = add i32 %sub, -8, !dbg !29

  ret i32 %mul, !dbg !30

 

; bar() is found to always return 4

  %call = call i32 @_Z3bazv(), !dbg !14

  %add = add nsw i32 %call, 4, !dbg !15

  call void @llvm.dbg.value(metadata i32 %add, metadata !13, metadata !DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_stack_value)), !dbg !16

  %sub = shl i32 %add, 1, !dbg !17

  %mul = add i32 %sub, -8, !dbg !17

  ret i32 %mul, !dbg !18

; %add is unused, optimize out and salvage...

  %call = call i32 @_Z3bazv(), !dbg !24

  call void @llvm.dbg.value(metadata i32 %call, metadata !23, metadata !DIExpression(DW_OP_plus_uconst, 4, DW_OP_constu, 4, DW_OP_minus, DW_OP_stack_value)), !dbg !25

  %add = shl i32 %call, 1, !dbg !26

  ret i32 %add, !dbg !27

  ; Final DWARF location for "a":

  DW_AT_location        (0x00000000:

      [0x0000000000000029, 0x000000000000002b): DW_OP_breg0 RAX+4, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_lit4, DW_OP_minus, DW_OP_stack_value)

[Adrian Prantl via llvm-dev]

On Sep 16, 2020, at 9:55 AM, Tozer, Stephen <stephe...@sony.com> wrote:

> That makes sense, and I think for "direct" values in your definition it is true that all direct values are r-values.

> Why do we need DW_OP_LLVM_direct when we already have DW_OP_LLVM_stack_value? Can you give an example of something that is definitely not a stack value, but direct?

The difference in definition is the intention: DW_OP_LLVM_direct means "we'd like this to be an l-value if possible", DW_OP_stack_value means "this should never be an l-value". Because of this, an expression ending with DW_OP_LLVM_direct can be emitted as an l-value in any case where the value of the preceding expression is equal to an l-value. So for example:

  DBG_VALUE $rsp, !"x", !DIExpression(DW_OP_LLVM_direct) => DW_OP_reg7 RSP

  DBG_VALUE $rsp, !"x", !DIExpression(DW_OP_deref, DW_OP_LLVM_direct) => DW_OP_breg7 RSP+0

  DBG_VALUE $rsp, !"x", !DIExpression(DW_OP_plus_uconst, 4, DW_OP_LLVM_direct) => DW_OP_breg7 RSP+4, DW_OP_stack_value

Your point about the semantics of variable assignments in the debugger is useful, that clears up my misunderstandings. I believe that even with that in mind, LLVM_direct (or whatever name it takes) would be appropriate. If we recognize that a variable must be read-only to preserve those semantics, then we can use DW_OP_stack_value to ensure that it is always an r-value. If we don't have any reason to make a variable read-only other than that we can't *currently* find an l-value location for it, then we would use DW_OP_LLVM_direct. Right now we use DW_OP_stack_value whenever we make a complex expression, but that doesn't need to be the case.

Great! It sounds like we reached mutual understanding :-)

So in this example, if we had DW_OP_LLVM_direct, we would salvage "a" as

DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_LLVM_direct) ?

which would mean: "this is an l-value with some additional DWARF operations. The backend should either emit this as a DW_OP_stack_value, or if the DWARF expression turns out to be a no-op, drop the entire DIExpression and emit this as a register or memory location.".

I can see how that could potentially be useful. I'm not sure how often we could practically make use of a situation like this, but I understand your motivation.

If we had DW_OP_LLVM_direct: what would be the semantics of 

DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_LLVM_direct)

versus

DIExpression(DW_OP_constu, 4, DW_OP_minus) ?

thanks,

adrian

[Tozer, Stephen via llvm-dev]

> I can see how that could potentially be useful. I'm not sure how often we could practically make use of a situation like this, but I understand your motivation.

Indeed, I don't expect us to cancel out DWARF expressions like that very often. Although that edge case is likely to be very rare, the _direct operator itself will appear very frequently, as it would be used for every DBG_VALUE that represents a register location. This allows us to represent register locations in a way that doesn't rely on flags outside of the DIExpression, doesn't require changes to be made to the flag/DIExpression if the register is RAUWd by a constant or other value, and has a clear definition that doesn't clash with anything in the DWARF spec. Supporting the no-op DIExpression reduction is unlikely to have a huge impact in itself, but having a "stack_value that could be an l-value" nicely rounds out the LLVM representation for debug values.

>If we had DW_OP_LLVM_direct: what would be the semantics of 

>

>DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_LLVM_direct)

>

>versus

>

>DIExpression(DW_OP_constu, 4, DW_OP_minus) ?

Once we have the _direct operator, which will be used for all register locations and some implicit locations, we can safely say that any expression that isn't _direct, implicit, or empty will be a memory location. So for the first expression we would check to see if it could be emitted as a register location, and when that fails we emit a stack value:

DW_OP_breg7 RSP+0, DW_OP_constu 4, DW_OP_minus, DW_OP_stack_value

Since the second expression is not LLVM_direct, stack_value, implicit_ptr, or any other explicitly declared location type, then it must be a memory location, so we emit:

DW_OP_breg7 RSP+0, DW_OP_constu 4, DW_OP_minus

[Adrian Prantl via llvm-dev]

On Oct 6, 2020, at 5:13 AM, Tozer, Stephen <stephe...@sony.com> wrote:

> I can see how that could potentially be useful. I'm not sure how often we could practically make use of a situation like this, but I understand your motivation.

Indeed, I don't expect us to cancel out DWARF expressions like that very often. Although that edge case is likely to be very rare, the _direct operator itself will appear very frequently, as it would be used for every DBG_VALUE that represents a register location. This allows us to represent register locations in a way that doesn't rely on flags outside of the DIExpression, doesn't require changes to be made to the flag/DIExpression if the register is RAUWd by a constant or other value, and has a clear definition that doesn't clash with anything in the DWARF spec. Supporting the no-op DIExpression reduction is unlikely to have a huge impact in itself, but having a "stack_value that could be an l-value" nicely rounds out the LLVM representation for debug values.

That makes sense.

>If we had DW_OP_LLVM_direct: what would be the semantics of 

>

>DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_LLVM_direct)

>

>versus

>

>DIExpression(DW_OP_constu, 4, DW_OP_minus) ?

Once we have the _direct operator, which will be used for all register locations and some implicit locations, we can safely say that any expression that isn't _direct, implicit, or empty will be a memory location.

I don't see how this is a meaningful distinction in LLVM IR. In LLVM IR we only have SSA values. An SSA value could be an alloca, or a gep into an alloca, or spilled onto the stack at the MIR level, in which case the dbg.value should get lowered into a memory location (if it isn't explicitly a DW_OP_stack_value). Do you have an example of a a dbg.value that isn't a DW_OP_stack_value where it makes sense to distinguish between a memory and a register location?

Perhaps another way to phrase this question — is there a difference between

dbg.value(my_alloca, var, !DIExpression(DW_OP_deref, DW_OP_LLVM_direct))

and

dbg.value(my_alloca, var, !DIExpression(DW_OP_deref)) ?

thanks,

Adrian

[Tozer, Stephen via llvm-dev]

> I don't see how this is a meaningful distinction in LLVM IR. In LLVM IR we only have SSA values. An SSA value could be an alloca, or a gep into an alloca, or spilled onto the stack at the MIR level, in which case the dbg.value should get lowered into a memory location (if it isn't explicitly a DW_OP_stack_value).

I think the distinction is still important; even at the IR level, if we have a dbg.value that uses an alloca or something similar, we can still distinguish between "this alloca is the variable's location" versus "this alloca is the variable's value", i.e. the variable itself is a pointer to a local variable. In IR, we implicitly distinguish between these by using dbg.declare/dbg.addr for the former, and dbg.value for the latter. In MIR, we use the indirectness flag instead. DW_OP_LLVM_direct can supplant the latter.

Apologies for the somewhat confusing explanation thus far; in the IR stage, I think that actually we wouldn't need to produce DW_OP_LLVM_direct at all (although there's no harm in doing so) as long as we have the existing set of debug variable intrinsics, because "directness" is already made explicit by the choice of intrinsic. Every dbg.value would implicitly be LLVM_direct unless it has another implicit location specifier (such as stack_value or implicit_ptr). This would mean that we could have a debug value: dbg.value(%a, "a", (DW_OP_plus_uconst, 5)), with no stack_value necessary, as opposed to the current case where every dbg.value with a complex expression has stack_value (I believe).

As discussed, one of the key distinctions that DW_OP_LLVM_direct is used for is distinguishing between memory and register locations; this is exactly the same as the difference between dbg.addr(%a, "a", ()) and dbg.value(%a, "a", ()). The former would become DBG_VALUE %a, "a", () and the latter would become DBG_VALUE %a, "a", (DW_OP_LLVM_direct).

> Do you have an example of a a dbg.value that isn't a DW_OP_stack_value where it makes sense to distinguish between a memory and a register location?

>

> Perhaps another way to phrase this question — is there a difference between

>

> dbg.value(my_alloca, var, !DIExpression(DW_OP_deref, DW_OP_LLVM_direct))

>

> and

>

> dbg.value(my_alloca, var, !DIExpression(DW_OP_deref)) ?

So with that in mind, we wouldn't need to produce these, as in both cases the intent would be that the value of "var" is at the address given by "my_alloca". When we produce the corresponding DBG_VALUEs for these, both would end with DW_OP_LLVM_direct. This would change if we unified the IR debug intrinsics so that a single intrinsic could represent both memory locations and register/implicit locations, as opposed to the current state where the former can only be represented by dbg.declare/dbg.addr and the latter can only be represented by dbg.value.

[Adrian Prantl via llvm-dev]

On Oct 7, 2020, at 5:38 AM, Tozer, Stephen <stephe...@sony.com> wrote:

> I don't see how this is a meaningful distinction in LLVM IR. In LLVM IR we only have SSA values. An SSA value could be an alloca, or a gep into an alloca, or spilled onto the stack at the MIR level, in which case the dbg.value should get lowered into a memory location (if it isn't explicitly a DW_OP_stack_value).

I think the distinction is still important; even at the IR level, if we have a dbg.value that uses an alloca or something similar, we can still distinguish between "this alloca is the variable's location" versus "this alloca is the variable's value", i.e. the variable itself is a pointer to a local variable. In IR, we implicitly distinguish between these by using dbg.declare/dbg.addr for the former, and dbg.value for the latter. In MIR, we use the indirectness flag instead. DW_OP_LLVM_direct can supplant the latter.

Apologies for the somewhat confusing explanation thus far; in the IR stage, I think that actually we wouldn't need to produce DW_OP_LLVM_direct at all (although there's no harm in doing so) as long as we have the existing set of debug variable intrinsics, because "directness" is already made explicit by the choice of intrinsic.

That is exactly what I was trying figure out. My concern is that it would be confusing to add a DWARF expression extension at the LLVM IR level that has not semantic effect. If we don't add a Verifier check that it is consistently applied then inevitably various frontends will start producing either variant of DIExpression. If we do add a Verifier check, then we are effectively adding a pointless extra field to every DIExpression that has no effect. In either case it would be hard to explain to new LLVM developers why this operation exists in LLVM IR.

So I wonder if we should instead model this only at the MIR level, where this distinction actually makes sense. In MIR, we probably don't want to rewrite every DIExpression, so it would make sense to model it either as a flag on the intrinsic, or by have two kinds of intrinsics.

What do you think about making this a property of MIR instead?

-- adrian

[Tozer, Stephen via llvm-dev]

> So I wonder if we should instead model this only at the MIR level, where this distinction actually makes sense. In MIR, we probably don't want to rewrite every DIExpression, so it would make sense to model it either as a flag on the intrinsic, or by have two kinds of intrinsics.

That works for me - as long as we have the ability to represent these expressions, it should be fine. What will be slightly awkward is maintaining this at the same time as the old DBG_VALUE; having two flags on two different but related instructions with the same name and meanings that are almost the same but slightly different. Still, the old version will be deprecated so we shouldn't have to worry too much.

[Adrian Prantl via llvm-dev]

On Oct 8, 2020, at 4:07 AM, Tozer, Stephen <stephe...@sony.com> wrote:

> So I wonder if we should instead model this only at the MIR level, where this distinction actually makes sense. In MIR, we probably don't want to rewrite every DIExpression, so it would make sense to model it either as a flag on the intrinsic, or by have two kinds of intrinsics.

That works for me - as long as we have the ability to represent these expressions, it should be fine. What will be slightly awkward is maintaining this at the same time as the old DBG_VALUE; having two flags on two different but related instructions with the same name and meanings that are almost the same but slightly different. Still, the old version will be deprecated so we shouldn't have to worry too much.

My expectation is that DBG_VALUE_OLD is not going to stick around with us for very long. It's only used internally in LLVM and we should be able to move off it quickly. That the short transitional period may be confusing doesn't bother me.

Great — that sounds like a plan then!

thanks for your patience working through this,

adrian