<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Hi Stephen,</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Thanks for working on this.</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<div style="margin: 0px">>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.</div>
<div style="margin: 0px"><br style="font-family: Calibri, Arial, Helvetica, sans-serif; background-color: rgb(255, 255, 255)">
</div>
<div style="margin: 0px">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?</div>
<div style="margin: 0px"><br>
</div>
Best regards,</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Djordje</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div id="appendonsend"></div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> llvm-dev <llvm-dev-bounces@lists.llvm.org> on behalf of Tozer, Stephen via llvm-dev <llvm-dev@lists.llvm.org><br>
<b>Sent:</b> Tuesday, August 25, 2020 8:09 PM<br>
<b>To:</b> llvm-dev@lists.llvm.org <llvm-dev@lists.llvm.org><br>
<b>Subject:</b> [llvm-dev] [Debuginfo] Changing llvm.dbg.value and DBG_VALUE to support multiple location operands</font>
<div> </div>
</div>
<style type="text/css" style="display:none">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
</style>
<div dir="ltr">
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
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.</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)">
<div><br>
</div>
<div>There are a few differences between the MIR instructions:</div>
<div><br>
</div>
<div><span style="font-family:Consolas,Courier,monospace">Old: </span><span style="font-family:Consolas,Courier,monospace">DBG_VALUE %x, $noreg, !DILocalVariable("x"), !DIExpression()</span></div>
<div><span style="font-family:Consolas,Courier,monospace">New: </span><span style="font-family:Consolas,Courier,monospace">DBG_VALUE !DILocalVariable("x"), !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_stack_value), %x</span></div>
<div><br>
</div>
<div>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.</div>
<div>2) The second operand which currently represents "Indirectness" has been removed entirely, because this is now explicitly specified in the DIExpression (see 4).</div>
<div>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 `<span style="font-family:Consolas,Courier,monospace">DW_OP_LLVM_arg,
N</span>` for the Nth location operand.</div>
<div>4) The DIExpression itself must be explicit about whether it evaluates to the location of a variable or its literal value, by using
<span style="font-family:Consolas,Courier,monospace">DW_OP_stack_value</span> in the latter case (instead of relying on the Indirectness flag, which is both confusing and redundant[1]).</div>
<div><br>
</div>
<div>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
<span style="font-family:Consolas,Courier,monospace">DW_OP_stack_value</span> for a simple single-register DBG_VALUE (as in the example above) would currently lose information, as it would output the DWARF expression `<span style="font-family:Consolas,Courier,monospace">DW_OP_breg0
RSP+0, DW_OP_stack_value</span>` instead of the current output `<span style="font-family:Consolas,Courier,monospace">DW_OP_reg0 RSP</span>`. 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.</div>
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>Described here are the differences in the IR intrinsics:</div>
<div><br>
</div>
<div><span style="font-family:Consolas,Courier,monospace">Old: </span><span style="font-family:Consolas,Courier,monospace">@llvm.dbg.value(metadata i32 %x, metadata !DILocalVariable("x"), metadata !DIExpression())</span></div>
<div><span style="font-family:Consolas,Courier,monospace">New: </span><span style="font-family:Consolas,Courier,monospace">@llvm.dbg.value(metadata !DIValueList(i32 %x), metadata !DILocalVariable("x"), metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_stack_value))</span></div>
<div><br>
</div>
<div>1) The location operand is changed from a single Value to a list of 0 or more Values.</div>
<div>2) The DIExpression is modified in the same manner as in the MIR instruction (see above).</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>[0] https://reviews.llvm.org/D82363</div>
<div>[1] https://bugs.llvm.org/show_bug.cgi?id=41675#c8</div>
[2] <a href="http://lists.llvm.org/pipermail/llvm-dev/2020-February/139441.html" id="LPlnk201276">
http://lists.llvm.org/pipermail/llvm-dev/2020-February/139441.html</a><br>
</div>
</div>
</body>
</html>