[PATCH] [RFC] Rewrite the way we generate debug locations for variables.

[Adrian Prantl]

On Jan 31, 2014, at 6:41, Alexey Samsonov <samsonov at google.com> wrote:
> 
> On Fri, Jan 31, 2014 at 3:13 AM, Robinson, Paul <
> Paul_Robinson at playstation.sony.com> wrote:
> 
> > I really really really wish I had time to review this now.
> > For the moment some kibitzing based on prior experience.
> >
> > > -----Original Message-----
> > > From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > > bounces at cs.uiuc.edu] On Behalf Of Alexey Samsonov
> > > Sent: Thursday, January 30, 2014 8:20 AM
> > > To: dblaikie at gmail.com; samsonov at google.com
> > > Cc: llvm-commits at cs.uiuc.edu
> > > Subject: [PATCH] [RFC] Rewrite the way we generate debug locations for
> > > variables.
> > >
> > > Hi dblaikie,
> > >
> > > Completely change the way we turn DBG_VALUE machine instructions
> > > into location information for local variables. This is inspired by
> > > discussion
> > > in r128327 review thread.
> > >
> > > We used to terminate location range for each variable at the end of the
> > > basic
> > > block with DBG_VALUE for it. This is incorrect, e.g. the register used
> > > to hold
> > > the variable location may be never clobbered in the entire function, so
> > > one
> > > DBG_VALUE is enough to define the location of a variable in the whole
> > > function.
> >
> > Terminating at that point is the safe thing to do.  Working out
> > when it's safe not to terminate is the tricky part, yeah.
> >
> > >
> > > Now we actually analyze the control flow: calculate the presumed
> > > location of
> > > a variable at the end and the beginning of each basic block. For
> > > example,
> > > if the location of a variable is the same at the end of all predecessors
> > > of
> > > basic block B, we may assume that we know the location of a variable in
> > > B,
> >
> > Sure.
> >
> > > and avoid terminating the location range too early.
> >
> > Whoa.  Looking at predecessors can tell you where something is on
> > block entry, but that's not the same as being able to not terminate
> > a range.  Predecessors are a control-flow thing, ranges are a
> > physical-layout thing.  Maybe you account for this in your patch,
> > like I said I haven't looked at it yet, but it's a trap for the
> > unwary.
> >
> 
> Yes, that's what complicates things. I take into account the order in which
> machine
> basic blocks are emitted and do a "best effort". Suppose that BB3 has two
> predecessors
> in control-flow graph: BB1 and BB2, and location of variable "v" at the end
> of BB1 is known
> and is the same as its location at the end of BB2. Then we check if we know
> the location
> of "v" at the end of basic block, _preceding BB3 in the physical layout_
> (this may be BB1, BB2,
> or some other basic block). If, again, the location happens to be the same,
> then we can continue
> the location range for "v" to BB3. Otherwise we terminate the location
> range at the beginning of BB3.
> 

Sounds like a data flow analysis that computes the meet-over-all paths solution for the DBG_VALUE assignments for each DIVariable where the join only keeps DBG_VALUES that are identical for all predecessors.

I imagine that we could implement this as a pass on the MC level.

Before:
-------
BB1:
  put "x" in %rax
  DBG_VALUE: var "x" is in %rax
  clobber %rbx
  jmp BB4
BB2:
  put "x" in %rbx
  DBG_VALUE: var "x" is in %rbx
  clobber %rax
  jmp BB3
BB3:
  jmp BB4
BB4:

After:
------
BB1:
  put "x" in %rax
  DBG_VALUE: var "x" is in %rax
  clobber %rbx
  jmp BB4
BB2:
  put "x" in %rbx
  DBG_VALUE: var "x" is in %rbx
  clobber %rax
  jmp BB3
BB3:
  DBG_VALUE: var "x" is in %rbx
  jmp BB4
BB4:
  DBG_VALUE: var "x” is #undef

My point is that this would be more explicit and debuggable. And the naive location merging code in DwarfDebug::emitDebugLoc() would get rid of redundant DBG_VALUEs (BB3 in the example). I think it would be better to avoid introducing additional complexity into DwarfDebug and friends.


-- adrian