[PATCH] D52614: [DebugInfo][Dexter] Divide-before-return displays wrong value in debugger

[Carlos Alberto Enciso via Phabricator via llvm-commits]

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

In https://reviews.llvm.org/D52614#1248058, @dstenb wrote:

> It would be good to change the title to reflect what is done in MCP (at least before submitting this).


We are using the tags: [DebugInfo][Dexter] and identify the issues detected by the DexTer tool.

For the title, may be something like:

`Incorrect DBG_VALUE after MCP dead copy instruction removal.`

> Can the test be reduced to only contain the bb.2.sw.bb1 block?

Yes. I have reduced the test case, just to contain `bb.2.sw.bb1` block.



================
Comment at: include/llvm/CodeGen/MachineInstr.h:1547
 
+  /// Update any matching DBG_VALUEs with the given register.
+  void updateDebugValues(unsigned Reg);
----------------
aprantl wrote:
> How about:
> 
> `/// Find all DBG_VALUEs immediately following this instruction that point to a register def in this instruction and point them to \p Reg instead.`
> 
> Looking at the implementation of collectDebugValues() I'm somewhat puzzled how it works. Perhaps I'm misunderstanding what it is supposed to do, please let me know. It's scanning for DBG_VALUEs that point to Op0 of the instruction. Is Op0 always going to be a def? Or should it be improved?
> /// Find all DBG_VALUEs immediately following this instruction that point to a register def in this instruction and point them to \p Reg instead.

That is a better description.


================
Comment at: test/CodeGen/MIR/X86/pr38773.mir:46
+    %foo.0.foo.0..sroa_cast = bitcast i32* %foo to i8*, !dbg !16
+    call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %foo.0.foo.0..sroa_cast), !dbg !16
+    call void @llvm.dbg.declare(metadata i32* %foo, metadata !12, metadata !DIExpression()), !dbg !17
----------------
dstenb wrote:
> Can the lifetime markers be removed?
I have removed all the not required markers.


================
Comment at: test/CodeGen/MIR/X86/pr38773.mir:50
+    %foo.0.foo.0. = load volatile i32, i32* %foo, align 4, !dbg !22, !tbaa !18
+    call void @llvm.dbg.value(metadata i32 %foo.0.foo.0., metadata !14, metadata !DIExpression()), !dbg !23
+    %foo.0.foo.0.6 = load volatile i32, i32* %foo, align 4, !dbg !24, !tbaa !18
----------------
dstenb wrote:
> Should unrelated debug values for other variables be removed to minimize the test?
I have removed all the not required debug values. I have left only the ones for 'read1'.


================
Comment at: test/CodeGen/MIR/X86/pr38773.mir:153
+    ; CHECK-NEXT:   DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !23
+    ; CHECK-NEXT:   DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !23
+    DBG_VALUE debug-use $ecx, debug-use $noreg, !14, !DIExpression(), debug-location !23
----------------
aprantl wrote:
> This line is an exact duplicate of the previous line. While this won't break anything, it's inefficient.
> Looking at the definition of collectDebugValues it doesn't look like this patch is responsible, but it would still be good to fix. Whoever is inserting these DBG_VALUEs in the first place should be doing a check for duplicates? Doing a linear search of any DBG_VALUEs immediately following the insertion point (like collectDebugValues does) should be sufficient.
The duplicated line has been removed. Modified the IR just to include only a single DBG_VALUE.


================
Comment at: test/CodeGen/MIR/X86/pr38773.mir:153
+    ; CHECK-NEXT:   DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !23
+    ; CHECK-NEXT:   DBG_VALUE debug-use $eax, debug-use $noreg, !14, !DIExpression(), debug-location !23
+    DBG_VALUE debug-use $ecx, debug-use $noreg, !14, !DIExpression(), debug-location !23
----------------
CarlosAlbertoEnciso wrote:
> aprantl wrote:
> > This line is an exact duplicate of the previous line. While this won't break anything, it's inefficient.
> > Looking at the definition of collectDebugValues it doesn't look like this patch is responsible, but it would still be good to fix. Whoever is inserting these DBG_VALUEs in the first place should be doing a check for duplicates? Doing a linear search of any DBG_VALUEs immediately following the insertion point (like collectDebugValues does) should be sufficient.
> The duplicated line has been removed. Modified the IR just to include only a single DBG_VALUE.
> Looking at the definition of collectDebugValues it doesn't look like this patch is responsible, but it would still be good to fix.

If you are OK, I can leave that issue to be fix in another patch. 

> Whoever is inserting these DBG_VALUEs in the first place should be doing a check for duplicates? Doing a linear search of any DBG_VALUEs immediately following the insertion point (like collectDebugValues does) should be sufficient.

That is a very good point.

During the review [[ https://reviews.llvm.org/D50887 | D50887 ]], there were some discussions about the `collectDebugValues` implementation. The idea was to use the list of USEs instead of linearly scanning.

Another interesting modification, was the one you raised on [[ https://bugs.llvm.org/show_bug.cgi?id=38763#c8 | PR38763#c8 ]] to extended llvm.dbg.value to take more than one LLVM SSA Value.

I would suggest if we can create specific bugzillas to address the following issues:

* `collectDebugValues` implementation (to use the list of USEs).
* Check for duplicates when inserting DBG_VALUEs.
* Extended llvm.dbg.value to take more than one LLVM SSA Value.

I am happy to create those bugzillas, so we can have something in concrete to work on.


Repository:
  rL LLVM

https://reviews.llvm.org/D52614