<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?</div></div></blockquote><div><br></div><div>If guessing what an epilogue is is non-trivial (and it seems so), maybe MI flag would be better indeed?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><span style="color:rgb(34,34,34)"> </span></div></div></blockquote><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>
</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,</blockquote><div><br></div><div>It is. I considered the case when the last basic block contains regular instructions in addition to epilogue code. In the current version of patch we don't special case any registers and don't say</div>
<div>"okay, this is %rsp, it's vaild in the whole function". We just don't assume these registers must be clobbered at the end of the basic block. When these registers are modified in the last MBB (be it</div>
<div>the epilogue, or the regular instruction), we do break the location range for the corresponding variable. See the expectations for the test cases - e.g. we assume that variable described with %rbp</div><div>is accessible up to the "pop %rbp" instruction.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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><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>