<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 30, 2014 at 3:54 PM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><br>
> On May 30, 2014, at 3:51 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> wrote:<br>
><br>
><br>
> On Fri, May 30, 2014 at 3:44 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
><br>
> > On May 30, 2014, at 3:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> ><br>
> > On Fri, May 30, 2014 at 3:37 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
> >><br>
> >>> On May 30, 2014, at 3:31 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> >>><br>
> >>> On Fri, May 30, 2014 at 3:12 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
> >>>> On Fri, May 30, 2014 at 3:03 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> >>>>> On Fri, May 30, 2014 at 3:00 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> wrote:<br>
> >>>>>><br>
> >>>>>> On Fri, May 30, 2014 at 12:45 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
> >>>>>> wrote:<br>
> >>>>>>><br>
> >>>>>>> On Fri, May 30, 2014 at 10:26 AM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
> >>>>>>>><br>
> >>>>>>>>> On May 29, 2014, at 10:03 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>><br>
> >>>>>>>>> wrote:<br>
> >>>>>>>>><br>
> >>>>>>>>> Yeah, well, we can add "FrameTeardown" machine instruction flag in<br>
> >>>>>>>>> addition to "FrameSetup", and annotate machine instructions added in frame<br>
> >>>>>>>>> lowering code. But do we expect many users of this flag (assuming that this<br>
> >>>>>>>>> usage is also somewhat questionable)?<br>
> >>>>>>>><br>
> >>>>>>>> The FrameSetup flag is also only used by the debug info, so at least it<br>
> >>>>>>>> wouldn’t be any more questionable than that :-)<br>
> >>>>>><br>
> >>>>>><br>
> >>>>>> OK. Why don't we land the patch as-is now (don't terminate the register at<br>
> >>>>>> the end of MBB if this register is only modified in frame setup and the last<br>
> >>>>>> MBB - it should be safe), and then replace "the last MBB" condition with<br>
> >>>>>> checking a "FrameTeardown" flag, attached to certain machine instructions?<br>
> >>>>>> In this way we won't have to special-case certain registers and make<br>
> >>>>>> assumptions about their constant-ness.<br>
> >>>>><br>
> >>>>> Given the limited nature of this fix (both in the fix's possible scope<br>
> >>>>> and intent/purpose) I'm personally sort of inclined to do this by<br>
> >>>>> whitelisting certain registers instead, if that were/is possible...<br>
> >>>>> seems simpler and still sufficient for the required purpose here.<br>
> >>>>><br>
> >>>>> Can we easily just test whether a register is the stack/frame pointer?<br>
> >>>>> (I actually know so little about this that the difference between<br>
> >>>>> those two things isn't straight in my head and I'm not sure which, or<br>
> >>>>> both, we need for the common/intended case here)<br>
> >>>>><br>
> >>>><br>
> >>>> If we need to stack realign we might not be able to do this as easily.<br>
> >>>> I'm not a huge fan of either method, but I think that the assumption<br>
> >>>> that Alexey has might make more sense than whitelisting.<br>
> >>><br>
> >>> Fair enough.<br>
> >>><br>
> >>>> My logic:<br>
> >>>> Whitelisting involves assuming that various registers won't be used in<br>
> >>>> ways that surprise us and I can think of a few ways where it could<br>
> >>>> happen: stack realignment, inline asm, a smarter stack coloring<br>
> >>>> algorithm.<br>
> >>><br>
> >>> Can inline asm clobber the frame pointer & we actually tolerate that,<br>
> >>> find the right variables, etc, when that happens? (how?)<br>
> >>><br>
> >>> Can't quite picture the stack coloring you have in mind & Alexey<br>
> >>> commented on the stack realignment issue.<br>
> >>><br>
> >>> The only other idea I'd float would be another simpler solution,<br>
> >>> though a bit more expressive: just a check to see that the particular<br>
> >>> register(s) (stack/frame pointers, whichever things it is we actually<br>
> >>> use to describe the location of stack variables) don't change beyond<br>
> >>> the prologue, and if not, describe any variables that are relative to<br>
> >>> that register as having that location for the whole program.<br>
> >><br>
> >> That won’t work for values that are spilled to the stack, because they only live on the stack for part of the function.<br>
> ><br>
> > Do we get those right currently?<br>
><br>
> Partially. We get them right until the end of the current basic block.<br>
><br>
> The patch should improve this situation by the way:<br>
> <bbX>:<br>
>   DBG_VALUE $rsp, 32 !"var"  // "var is spilled on stack.<br>
> // ... more code ..<br>
> <bbY>:<br>
>   DBG_VALUE $r9, 0, !"var"  // "var" is now loaded to "r9".<br>
><br>
> Earlier we used to terminate the location of "var" at the end of bbX, but now we won't do<br>
> this, as we know "rsp" doesn't change in the function.<br>
<br>
</div></div>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.<br></blockquote><div><br></div><div>Yes. So, how do you feel about the plan suggested above:</div>
<div><br></div><div><div style="font-family:arial,sans-serif;font-size:13px"><span style="color:rgb(80,0,80);font-family:arial;font-size:small">OK. Why don't we land the patch as-is now (don't terminate the register at</span><br style="color:rgb(80,0,80);font-family:arial;font-size:small">
<span style="color:rgb(80,0,80);font-family:arial;font-size:small">the end of MBB if this register is only modified in frame setup and the last</span><br style="color:rgb(80,0,80);font-family:arial;font-size:small"><span style="color:rgb(80,0,80);font-family:arial;font-size:small">MBB - it should be safe), and then replace "the last MBB" condition with</span><br style="color:rgb(80,0,80);font-family:arial;font-size:small">
<span style="color:rgb(80,0,80);font-family:arial;font-size:small">checking a "FrameTeardown" flag, attached to certain machine instructions?</span><br style="color:rgb(80,0,80);font-family:arial;font-size:small">
<span style="color:rgb(80,0,80);font-family:arial;font-size:small">In this way we won't have to special-case certain registers and make</span><br style="color:rgb(80,0,80);font-family:arial;font-size:small"><span style="color:rgb(80,0,80);font-family:arial;font-size:small">assumptions about their constant-ness.</span><br>
</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><br>
><br>
> But there is no code that would insert DBG_VALUES after they are reloaded again.<br>
><br>
><br>
> ><br>
> >> [And for (-O0) allocas that are described in the MMI table, we already do what you described].<br>
> ><br>
> > Right - but the problem Alexey is trying to address is what ASan does,<br>
> > where it makes one big alloca and describes variables as being within<br>
> > that alloca - so the MMI sidetable handling doesn't fire…<br>
><br>
> Awright.<br>
><br>
> Yeah, MMI side table doesn't really work for indirect descriptions, when we need to deref the memory stored at reg+offset.<br>
><br>
><br>
> ><br>
> >><br>
> >> -- adrian<br>
> >><br>
> >>><br>
> >>> But, yeah, that's not /lots/ easier than finding any registers that<br>
> >>> happen to meet that property.<br>
> >>><br>
> >>> - David<br>
> >>><br>
> >>>><br>
> >>>> -eric<br>
> >>>><br>
> >>>>>><br>
> >>>>>>><br>
> >>>>>>>><br>
> >>>>>>><br>
> >>>>>>> :)<br>
> >>>>>>><br>
> >>>>>>>>> I can also make this patch even less general and just special-case the<br>
> >>>>>>>>> frame pointer and the stack pointer registers, so that the code wouldn't<br>
> >>>>>>>>> pretend to solve a general problem we're facing.<br>
> >>>>>>>><br>
> >>>>>>>> The frame pointer should be stable over the entire function, special<br>
> >>>>>>>> casing it seems to be appropriate. I don’t know whether, e.g, stack coloring<br>
> >>>>>>>> would cause the stack pointer to be modified in the middle of a function.<br>
> >>>>>>>><br>
> >>>>>>><br>
> >>>>>>> FWIW it may not _now_ but there's nothing from stopping it either. :\<br>
> >>>>>>><br>
> >>>>>>> -eric<br>
> >>>>>><br>
> >>>>>><br>
> >>>>>> --<br>
> >>>>>> Alexey Samsonov<br>
> >>>>>> <a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br>
><br>
><br>
><br>
><br>
> --<br>
> Alexey Samsonov<br>
> <a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div>
</div></div>