[DebugInfo@O2][Dexter] Speculated BB presents illegal variable value to debugger

[jmorse]

Bugzilla Link	38763
Resolution	FIXED
Resolved on	Oct 05, 2018 03:24
Version	trunk
OS	Linux
Blocks	#38116
CC	@adrian-prantl,@dwblaikie,@efriedma-quic,@gregbedwell,@CarlosAlbertoEnciso,@JDevlieghere,@pogo59,@vedantk

Extended Description

Speculating a basic block into a select IR insn causes an illegal variable value to be presented to the debugger. Taking the code below, compiled -O2 -g for x86_64 with trunk, on the line "if (read == 4)" both gdb and lldb report that the value of "result" is two, where it should be zero.

The value two isn't calculated in the execution of this program, and when compiled with -O0 "result" is correctly reported zero on that line. More interestingly, the illegal value doesn't actually seem to be read from a speculated instruction, "result" is reported as two from the moment the volatile loads are done.

I get the impression that something DWARFy is going wrong, as the DW_AT_location expression for "result" looks way more complicated that it needs to be.

Found using DExTer ( https://github.com/SNSystems/dexter ).

-------->8--------
int
main()
{
volatile int foo = 4;
int read = foo;
int read1 = foo;

int result = 0;
if (read == 4) {
result = read1 + 2;
} else {
result = read1 - 2;
}

return result;
}
--------8<--------

[jmorse]

assigned to @CarlosAlbertoEnciso

[adrian-prantl]

Could you add the version number of llvm/clang you were using? Also, it might be helpful to paste in the output of llvm-dwarfdump --name=result.

[adrian-prantl]

The testcase is so short that the output of llc -stop-after=livedebugvalues would even fit :-)

[efriedma-quic]

I think the bad IR is coming out of FoldTwoEntryPHINode in SimplifyCFG: instead of erasing or fixing the llvm.dbg.value calls when the transform triggers, it just unconditionally sinks them out of the if statement.

[jmorse]

Version of clang/llvm is trunk @ r340912 built yesterday, output of llvm-dwarfdump --name=result:

a.out: file format ELF64-x86-64

0x00000060: DW_TAG_variable
DW_AT_location (0x00000023
[0x00000000004004b0, 0x00000000004004b0): DW_OP_consts +0, DW_OP_stack_value
[0x00000000004004b0, 0x00000000004004b0): DW_OP_breg2 RCX+2, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_stack_value
[0x00000000004004b0, 0x00000000004004be): DW_OP_breg2 RCX+0, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_constu 0x2, DW_OP_minus, DW_OP_stack_value
[0x00000000004004be, 0x00000000004004bf): DW_OP_reg0 RAX)
DW_AT_name ("result")
DW_AT_decl_file ("/tmp/test.cpp")
DW_AT_decl_line (8)
DW_AT_type (0x0000007f "int")

[CarlosAlbertoEnciso]

I think the bad IR is coming out of FoldTwoEntryPHINode in SimplifyCFG:
instead of erasing or fixing the llvm.dbg.value calls when the transform
triggers, it just unconditionally sinks them out of the if statement.

I have followed your suggestion of erasing the 'llvm.dbg.value' calls when the transform triggers.

Created a general utility function that can be used, in other similar problems that have been reported by the use of DExTer.

[CarlosAlbertoEnciso]

Proposed fix:

[https://reviews.llvm.org/D51976 | D51976]

[adrian-prantl]

[Copying my comment from https://reviews.llvm.org/D51976]

Having read the testcase now, I understand why you are dropping the debug info.

I would like to point out that we could do better. If we extended llvm.dbg.value to take more than one LLVM SSA Value, then we could produce a DIExpression that selects either %add or %sub depending on the value of %cmp. Let me know if you are interested in implementing this, it should be relatively straight forward to do and would come in useful in all sorts of other situations, too.

[adrian-prantl]

Davide and I recently sketched an idea for how to extend dbg.value but then never implemented it.

The idea was to turn

@​llvm.dbg.value(metadata %val, metadata !DILocalVariable(), metadata !DIExpression())

into a varargs function

@​llvm.dbg.value(metadata %val0, metadata !DILocalVariable(), metadata !DIExpression(), [metadata %val1, ...])

and in DIExpression we add new operators DW_OP_LLVM_ARG1, DW_OP_LLVM_ARG2, ... that push the contents of %val1, %val2, ... onto the DIExpression stack. Argument0 would remain implicit, this way the scheme is fully backwards compatible.

[CarlosAlbertoEnciso]

The patch has been committed at:

https://reviews.llvm.org/rL342527

[jmorse]

mentioned in issue #38116