[PATCH] D51976: [DebugInfo][Dexter] Speculated BB presents illegal variable value to debugger

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso marked 6 inline comments as done.
CarlosAlbertoEnciso added a comment.

In https://reviews.llvm.org/D51976#1235508, @aprantl wrote:

> Having read the real testcase now, I understand why you are dropping the debug info now. 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.


That is a very good idea and I am interested in implementing it. A good opportunity to improve my knowledge in that area. I can see the associated benefits.

I would suggest if that is OK with you, to finish with the current issue. And then as a general improvement, to revisit the 'select' case as an independent issue, to implement what you have described. If you feel otherwise, I would proceed with the changes proposed to the 'llvm.dbg.value'.

In the meantime, I am updating the patch.



================
Comment at: include/llvm/Transforms/Utils/Local.h:446
 
+/// Remove the debug intrinsic instructions for the given machine instruction.
+void dropDebugUsers(Instruction &I);
----------------
vsk wrote:
> Please omit 'machine' here, as MachineInstr and Instruction are distinct.
Removed 'machine'.


================
Comment at: test/CodeGen/X86/pr38763.ll:36
+; CHECK:  %sub = add nsw i32 %foo.0.4, -2, !dbg !32
+; CHECK:  %result.0 = select i1 %cmp, i32 %add, i32 %sub, !dbg !34
+; CHECK:  call void @llvm.dbg.value(metadata i32 %result.0, metadata !16, metadata !DIExpression()), !dbg !27
----------------
vsk wrote:
> For the test to be stringent, it needs to check that there these dbg.value's don't appear:
> 
> ```
> dbg.value(%add, !16, ...)
> dbg.value(%sub, !16, ...)
> ```
> 
> One way to do that is to require exact next-line matches. E.g:
> 
> ```
> CHECK: %add = ...
> CHECK-NOT: dbg.value(metadata i32 %add
> CHECK-NEXT: %sub = ...
> CHECK-NOT: dbg.value(metadata i32 %sub
> CHECK-NEXT: %result.0 = select ...
> CHECK-NEXT: d.v(%result.0, ...)
> ```
Thanks for showing me how to improve the test. Adding the 'CHECK-NOT' entries.


================
Comment at: test/CodeGen/X86/pr38763.ll:86
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture) #1
+
----------------
vsk wrote:
> Are these lifetime markers needed to reproduce the issue?
Those markers are not needed. Removing them.


================
Comment at: test/CodeGen/X86/pr38763.ll:93
+attributes #1 = { argmemonly nounwind }
+attributes #2 = { nounwind readnone speculatable }
+
----------------
vsk wrote:
> Is the test still effective if you strip out all these attributes (i.e, remove '#0' etc from the file)? If so, doing that is a good way to make the test more maintainable.
Able to reproduce the issue without those attributes.


================
Comment at: test/CodeGen/X86/pr38763.ll:113
+!13 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !10)
+!14 = !DILocalVariable(name: "read", scope: !7, file: !1, line: 3, type: !10)
+!15 = !DILocalVariable(name: "read1", scope: !7, file: !1, line: 4, type: !10)
----------------
aprantl wrote:
> You can strip out all variables that aren't necessary for the test, it makes it easier to read and maintain in the future.
Stripped out variables that are not necessary.


================
Comment at: test/CodeGen/X86/pr38763.ll:122
+!22 = !{!"Simple C++ TBAA"}
+!23 = !DILocation(line: 3, column: 14, scope: !7)
+!24 = !DILocation(line: 3, column: 7, scope: !7)
----------------
aprantl wrote:
> You can further simplify the testcase by just using single (or just fewer) DILocation for everything.
Remove unnecessary DILocation entries.


https://reviews.llvm.org/D51976