[DebugInfo@O2][Dexter]Stale debug variable value in optimised code

[jmorse]

Bugzilla Link	38753
Resolution	FIXED
Resolved on	Feb 24, 2020 10:04
Version	trunk
OS	Linux
Blocks	#38116
CC	@adrian-prantl,@bjope,@dstenb,@dwblaikie,@gregbedwell,@CarlosAlbertoEnciso,@JDevlieghere,@pogo59
Fixed by commit(s)	https://reviews.llvm.org/rG453dc4d7ec5a3c3d8f54fc358bc5673834516d48

Extended Description

With the (contrived) test below, the "cheese" variable has a stale value reported inside the "if" block. Using an up-to-date toolchain (r340912), and compiling with "clang++ test.cpp -g -O2 -o a.out" for x86_64, on the marked line gdb and lldb report the value of "cheese" to be four, wheras if compiled -O0 the correct value of eight is seen.

The likely cause is SimplifyCFG's tryCSEWithPredecessor replacing the duplicate "read1 + read2" expression, however the debug value for "cheese" is either lost or not updated, causing debuggers to report a state the program isn't in.

It should be possible to get this right or mark "cheese" optimised out, the true-if-block isn't merged with anything else.

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

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

int cheese = foo;
int a = read1 + read2;
a += cheese;

if (foo == 4) {
cheese = read1 + read2;
a -= cheese - 12;
a *= 20; // <------ stale value seen
a /= 3;
} else {
a = 0;
}

return a;
}
--------8<--------

[CarlosAlbertoEnciso]

When stopping in the debugger at line:

'if (foo == 4) {',

the value reported for the variable 'a' is 16. The correct value should be 12.

[jmorse]

bug 38754 appears to be related, in that the change from the second comment fixes this ticket. This could be marked duplicate, but I'll keep it open in case the solution to 38754 takes a complicated direction.

[jmorse]

NB, even with the placeDbgValues fix (added to r354569) this problem persists. It would appear that something (instcombine most likely) sink the read1+read2 expression into the conditional block and folds it with the other arithmetic there, but doesn't then issue an undef dbg.value or otherwise.

Plus, 'a' is unreadable on the return statement, which is a pain.

[jmorse]

Coming back to this with a fresh eye: it looks like I got highly confused in the past as I didn't see that "read1+read2" would get CSE'd, thus I misinterpreted the eventual assembly.

It's likely that the cause of the "stale" value is due to variable assignments and line numbers being two independent state machines that don't line up. This probably won't be addressed any time soon.

The matter of the return value is still relevant though, at the end of the compilation pipeline we get:

--------8<--------
; predecessors: %bb.0, %bb.1
liveins: $rax
DBG_VALUE $eax, $noreg, !"a", !DIExpression()
$eax = KILL renamable $eax, implicit killed $rax
RETQ $eax
-------->8--------

According to TargetOpcodes.def, the "KILL" instruction is a noop that adjusts the liveness of registers. DbgEntityHistoryCalculator sees the kill of $rax and (soundly) kills the previous DBG_VALUE, making it cover zero instructions. It's unfortunate that this leads to the return value never being printable.

CC David as he's worked on DbgEntityHistoryCalculator recently. There might not be a good way of fixing this without spraying special cases into DbgEntityHistoryCalculator :(.

[bjope]

CC David as he's worked on DbgEntityHistoryCalculator recently. There might
not be a good way of fixing this without spraying special cases into
DbgEntityHistoryCalculator :(.

@​dstenb: One thing here is that for our OOT target we remove KILL instructions before late scheduling (probably for historical reasons and our VLIW scheduler not handling KILL instructions very well). So I doubt that we have problem with KILL for our own target. But I believe the DbgValueHistoryCalculator could simply ignore KILL instructions.

[dstenb]

Sorry, I don't think I have anything valuable to add about that.

[llvmbot]

Taken for myself, going to try and get to the bottom of this.

[llvmbot]

so ignoring the KILL instruction and return value for now, I want to address the initially reported stale value for 'a' and for 'cheese'.

The problem was caused by a pass called ReassociateExpressions, found in llvm/lib/Transforms/Reassociate.cpp.

This pass was dropping instructions that were trivially deletable but wasn't then undeffing the debug intrinsics that were referencing them causing dbg.value intrinsics to have 'dangling' pointers that were fixed up to point at empty meta data.

A later pass, combine redundant instructions, would then drop these noop intrinsics completely from the block.

Then comes along the LiveDebugValues pass during the lowering stage in LLC. It's job is to propogate debug value intrinsics into as many blocks where the liveness of the location is preserved for as greater range as possible. Since we didn't have an undeffed intrinsic when the location was dropped by reassociate expressions the liveness of previous intrinsics are extended into subsequent blocks, thus extending the range of the "stale" values further down the function than is correct.

I've written a patch that attempts to address this issue, I'm currently working on a test and doing due-dilligence to ensure I've not messed anything else up in the process.

I'll take a look at the return value for A and the kill instruction afterwards.

[llvmbot]

patch up for your reviewing pleasure

https://reviews.llvm.org/D69943

[llvmbot]

first patch, fix for reassociate pass is now in master.

[llvmbot]

can comfirm that the return value for a is still missing as of TOT, starting investigation.

[llvmbot]

diff up for review on Phab for fixing the return value.

https://reviews.llvm.org/D70497

[llvmbot]

patch up and accepted for KILL instructions here:

https://reviews.llvm.org/D70497

[llvmbot]

https://reviews.llvm.org/rG453dc4d7ec5a3c3d8f54fc358bc5673834516d48

[jmorse]

mentioned in issue #38116