[PATCH] D61600: [DebugInfo] More precise variable range for stack locations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Just to note: the patch this is based on (r359426) was reverted in r360301 due to compile time issues. I've got a patch incoming (tomorrow) that relies on this patch to fix it: they'll need to be un-reverted / committed in a certain order and possibly all together. (We can work through that when this is approved though).



================
Comment at: lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:375
             unsigned RegNo = DbgValue->getOperand(0).getReg();
-            if (TRI->isVirtualRegister(RegNo) || ChangingRegs.test(RegNo))
+            if (TRI->isVirtualRegister(RegNo) || ChangingRegs.test(RegNo) ||
+                FrameReg == RegNo)
----------------
NikolaPrica wrote:
> jmorse wrote:
> > dstenb wrote:
> > > Perhaps this must not be done in this patch, but I wonder if we should close all register-described values here?
> > > 
> > > For example:
> > > 
> > > ```
> > > volatile int g;
> > > 
> > > int baz(int p) {
> > >   int local = 123;
> > > 
> > >   if (g > 1) {
> > >     local = p;
> > >     g += p;
> > >   }
> > > 
> > >   return 3;
> > > }
> > > ```
> > > 
> > > when compiling the above with `clang -O1 -g --target=x86_64-unknown-linux-gnu` we land in the following MIR after livedebugvalues:
> > > 
> > > ```
> > >   bb.0.entry:
> > >     successors: %bb.1(0x40000000), %bb.2(0x40000000)
> > >     liveins: $edi
> > >   
> > >     DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> > >     DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> > >     DBG_VALUE 123, $noreg, !17, !DIExpression(), debug-location !19
> > >     renamable $eax = MOV32rm $rip, 1, $noreg, @g, $noreg, debug-location !20 :: (volatile dereferenceable load 4 from @g, !tbaa !22)
> > >     CMP32ri8 killed renamable $eax, 2, implicit-def $eflags, debug-location !26
> > >     JCC_1 %bb.2, 12, implicit $eflags, debug-location !27
> > >   
> > >   bb.1.if.then:
> > >     successors: %bb.2(0x80000000)
> > >     liveins: $edi
> > >   
> > >     DBG_VALUE 123, $noreg, !17, !DIExpression(), debug-location !19
> > >     DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> > >     DBG_VALUE $edi, $noreg, !17, !DIExpression(), debug-location !19
> > >     ADD32mr $rip, 1, $noreg, @g, $noreg, killed renamable $edi, implicit-def dead $eflags, debug-location !28 :: (volatile store 4 into @g, !tbaa !22), (volatile dereferenceable load 4 from @g, !tbaa !22)
> > >   
> > >   bb.2.if.end:
> > >     DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> > >     $eax = MOV32ri 3, debug-location !31
> > >     RETQ $eax, debug-location !31
> > > ```
> > > 
> > > As seen, in the `g > 1` branch the `local` variable is described by edi. As edi is not modified, it will not be contained in `ChangingRegs`.
> > > 
> > > As we don't close the range here, this means that we'll incorrectly describe `local` using edi for the rest of the function:
> > > 
> > > ```
> > >                   DW_AT_location	(0x00000000
> > >                      [0x0000000000000000,  0x000000000000000b): DW_OP_consts +123, DW_OP_stack_value
> > >                      [0x000000000000000b,  0x0000000000000017): DW_OP_reg5 RDI)
> > > ```
> > > 
> > > ```
> > > 0000000000000000 baz:
> > >        0: 8b 05 00 00 00 00            	movl	(%rip), %eax
> > >        6: 83 f8 02                     	cmpl	$2, %eax
> > >        9: 7c 06                        	jl	6 <baz+0x11>
> > >        b: 01 3d 00 00 00 00            	addl	%edi, (%rip)
> > >       11: b8 03 00 00 00               	movl	$3, %eax
> > >       16: c3                           	retq
> > > ```
> > > 
> > > Perhaps this must not be done in this patch, but I wonder if we should close all register-described values here?
> > 
> > I'd say definite yes -- when I dug into the history [0] for PR41600 (which this patch fixes I think) it sounded like ChangingRegs was a mild hack to work around variable locations being unreliable ~5 years ago. IMHO (YMMV) we might be able to delete it in another patch as a now redundant feature. (Plus as you demonstrate, it can be broken).
> > 
> > [0] https://reviews.llvm.org/D3933
> This is similar problem that I am targeting. It would be solved by closing register-described values at the end of block. The only difference is that `local` is dead after the branch, but the idea is the same: Having BB1 that branches to BB2 and BB3, different DBG_VALUE from BB2 takes range across BB3 due to lack of variable's DBG_VALUE in it (variable might be dead or it just does lack location at that block).
> 
> I stumbled on a case when frame register is not in `ChangingRegs` but as you shown it can happen for other registers.  I needed to modify `DwarfDebug` part in order to preserve single value variable locations because of partitioning of variable's range. Current condition  for recognizing single value locations would not be recognized as single value location). 
NB: I think the ChangingRegs situation can be addressed separately. and this patch is fine to go ahead with the other tweaks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61600/new/

https://reviews.llvm.org/D61600