[PATCH] Generate better location ranges for some register-described variables.

[Adrian Prantl]

> On May 30, 2014, at 3:57 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
> 
> 
> On Fri, May 30, 2014 at 3:54 PM, Adrian Prantl <aprantl at apple.com> wrote:
> 
> > On May 30, 2014, at 3:51 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
> >
> >
> > On Fri, May 30, 2014 at 3:44 PM, Adrian Prantl <aprantl at apple.com> wrote:
> >
> > > On May 30, 2014, at 3:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > > On Fri, May 30, 2014 at 3:37 PM, Adrian Prantl <aprantl at apple.com> wrote:
> > >>
> > >>> On May 30, 2014, at 3:31 PM, David Blaikie <dblaikie at gmail.com> wrote:
> > >>>
> > >>> On Fri, May 30, 2014 at 3:12 PM, Eric Christopher <echristo at gmail.com> wrote:
> > >>>> On Fri, May 30, 2014 at 3:03 PM, David Blaikie <dblaikie at gmail.com> wrote:
> > >>>>> On Fri, May 30, 2014 at 3:00 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
> > >>>>>>
> > >>>>>> On Fri, May 30, 2014 at 12:45 PM, Eric Christopher <echristo at gmail.com>
> > >>>>>> wrote:
> > >>>>>>>
> > >>>>>>> On Fri, May 30, 2014 at 10:26 AM, Adrian Prantl <aprantl at apple.com> wrote:
> > >>>>>>>>
> > >>>>>>>>> On May 29, 2014, at 10:03 PM, Alexey Samsonov <vonosmas at gmail.com>
> > >>>>>>>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>> Yeah, well, we can add "FrameTeardown" machine instruction flag in
> > >>>>>>>>> addition to "FrameSetup", and annotate machine instructions added in frame
> > >>>>>>>>> lowering code. But do we expect many users of this flag (assuming that this
> > >>>>>>>>> usage is also somewhat questionable)?
> > >>>>>>>>
> > >>>>>>>> The FrameSetup flag is also only used by the debug info, so at least it
> > >>>>>>>> wouldn’t be any more questionable than that :-)
> > >>>>>>
> > >>>>>>
> > >>>>>> OK. Why don't we land the patch as-is now (don't terminate the register at
> > >>>>>> the end of MBB if this register is only modified in frame setup and the last
> > >>>>>> MBB - it should be safe), and then replace "the last MBB" condition with
> > >>>>>> checking a "FrameTeardown" flag, attached to certain machine instructions?
> > >>>>>> In this way we won't have to special-case certain registers and make
> > >>>>>> assumptions about their constant-ness.
> > >>>>>
> > >>>>> Given the limited nature of this fix (both in the fix's possible scope
> > >>>>> and intent/purpose) I'm personally sort of inclined to do this by
> > >>>>> whitelisting certain registers instead, if that were/is possible...
> > >>>>> seems simpler and still sufficient for the required purpose here.
> > >>>>>
> > >>>>> Can we easily just test whether a register is the stack/frame pointer?
> > >>>>> (I actually know so little about this that the difference between
> > >>>>> those two things isn't straight in my head and I'm not sure which, or
> > >>>>> both, we need for the common/intended case here)
> > >>>>>
> > >>>>
> > >>>> If we need to stack realign we might not be able to do this as easily.
> > >>>> I'm not a huge fan of either method, but I think that the assumption
> > >>>> that Alexey has might make more sense than whitelisting.
> > >>>
> > >>> Fair enough.
> > >>>
> > >>>> My logic:
> > >>>> Whitelisting involves assuming that various registers won't be used in
> > >>>> ways that surprise us and I can think of a few ways where it could
> > >>>> happen: stack realignment, inline asm, a smarter stack coloring
> > >>>> algorithm.
> > >>>
> > >>> Can inline asm clobber the frame pointer & we actually tolerate that,
> > >>> find the right variables, etc, when that happens? (how?)
> > >>>
> > >>> Can't quite picture the stack coloring you have in mind & Alexey
> > >>> commented on the stack realignment issue.
> > >>>
> > >>> The only other idea I'd float would be another simpler solution,
> > >>> though a bit more expressive: just a check to see that the particular
> > >>> register(s) (stack/frame pointers, whichever things it is we actually
> > >>> use to describe the location of stack variables) don't change beyond
> > >>> the prologue, and if not, describe any variables that are relative to
> > >>> that register as having that location for the whole program.
> > >>
> > >> That won’t work for values that are spilled to the stack, because they only live on the stack for part of the function.
> > >
> > > Do we get those right currently?
> >
> > Partially. We get them right until the end of the current basic block.
> >
> > The patch should improve this situation by the way:
> > <bbX>:
> >   DBG_VALUE $rsp, 32 !"var"  // "var is spilled on stack.
> > // ... more code ..
> > <bbY>:
> >   DBG_VALUE $r9, 0, !"var"  // "var" is now loaded to "r9".
> >
> > Earlier we used to terminate the location of "var" at the end of bbX, but now we won't do
> > this, as we know "rsp" doesn't change in the function.
> 
> Yeah, but only if we can detect the Epilogue and ignore the def of $rsp that restores the register at the end of the function.
> 
> Yes. So, how do you feel about the plan suggested above:
> 
> OK. Why don't we land the patch as-is now (don't terminate the register at
> the end of MBB if this register is only modified in frame setup and the last
> MBB - it should be safe), and then replace "the last MBB" condition with
> checking a "FrameTeardown" flag, attached to certain machine instructions?
> In this way we won't have to special-case certain registers and make
> assumptions about their constant-ness.

Instead of checking for it being the last basic block we could check for something like (BB.getLastNonDebugInstr()->isReturn()) then we would handle cases with multiple return blocks. The only problem that I see is that the last basic block may also contain non-epilogue code. In fact, the entire function may only be a single basic block. A safer condition might be “register is modified in the prologue && has precisely one non-prologue-def in each returning basic block”?
Then again that condition would break on a backend that pops each callee-saved register from the stack one-by-one.

What about this definition: The epilogue is everything after the last instruction that has a DebugLoc attached to it?

-- adrian
>  
> 
> >
> > But there is no code that would insert DBG_VALUES after they are reloaded again.
> >
> >
> > >
> > >> [And for (-O0) allocas that are described in the MMI table, we already do what you described].
> > >
> > > Right - but the problem Alexey is trying to address is what ASan does,
> > > where it makes one big alloca and describes variables as being within
> > > that alloca - so the MMI sidetable handling doesn't fire…
> >
> > Awright.
> >
> > Yeah, MMI side table doesn't really work for indirect descriptions, when we need to deref the memory stored at reg+offset.
> >
> >
> > >
> > >>
> > >> -- adrian
> > >>
> > >>>
> > >>> But, yeah, that's not /lots/ easier than finding any registers that
> > >>> happen to meet that property.
> > >>>
> > >>> - David
> > >>>
> > >>>>
> > >>>> -eric
> > >>>>
> > >>>>>>
> > >>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> :)
> > >>>>>>>
> > >>>>>>>>> I can also make this patch even less general and just special-case the
> > >>>>>>>>> frame pointer and the stack pointer registers, so that the code wouldn't
> > >>>>>>>>> pretend to solve a general problem we're facing.
> > >>>>>>>>
> > >>>>>>>> The frame pointer should be stable over the entire function, special
> > >>>>>>>> casing it seems to be appropriate. I don’t know whether, e.g, stack coloring
> > >>>>>>>> would cause the stack pointer to be modified in the middle of a function.
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> FWIW it may not _now_ but there's nothing from stopping it either. :\
> > >>>>>>>
> > >>>>>>> -eric
> > >>>>>>
> > >>>>>>
> > >>>>>> --
> > >>>>>> Alexey Samsonov
> > >>>>>> vonosmas at gmail.com
> >
> >
> >
> >
> > --
> > Alexey Samsonov
> > vonosmas at gmail.com
> 
> 
> 
> 
> -- 
> Alexey Samsonov
> vonosmas at gmail.com