<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 30, 2014 at 4:17 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On May 30, 2014, at 4:13 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
><br>
><br>
>> On May 30, 2014, at 3:57 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> wrote:<br>
>><br>
>><br>
>> On Fri, May 30, 2014 at 3:54 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br>
>><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>
>> 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>
>><br>
>> Yes. So, how do you feel about the plan suggested above:<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>
> 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”?<br>

> Then again that condition would break on a backend that pops each callee-saved register from the stack one-by-one.<br>
><br>
> What about this definition: The epilogue is everything after the last instruction that has a DebugLoc attached to it?<br>
<br>
</div></div>Sorry, I realized didn’t actually answer you question :-)<br>
I agree that this is a big improvement, but I’m not convinced that the condition for detecting the epilogue you mention is safe, for the same reasons that Eric mentioned earlier. When we find a safe condition for the epilogue this is good to go. My current favorite is looking at the line table.<br>
</blockquote><div><br></div><div>Could you elaborate on line table solution?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-- adrian<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> -- adrian<br>
>><br>
>><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>
>><br>
>><br>
>><br>
>> --<br>
>> Alexey Samsonov<br>
>> <a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br>
><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>