<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 3, 2014 at 12:33 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 style="word-wrap:break-word"><br><div><div><div class="h5"><blockquote type="cite"><div>On Jun 1, 2014, at 11:23 AM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a>> wrote:</div>
<br><div><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 30, 2014 at 7:14 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><div><br>
> On May 30, 2014, at 4:55 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a>> wrote:<br>
><br>
><br>
> On Fri, May 30, 2014 at 4:17 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
><br>
> > On May 30, 2014, at 4:13 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
> ><br>
> ><br>
> >> On May 30, 2014, at 3:57 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">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" target="_blank">aprantl@apple.com</a>> wrote:<br>
> >><br>
> >>> On May 30, 2014, at 3:51 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">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" target="_blank">aprantl@apple.com</a>> wrote:<br>
> >>><br>
> >>>> On May 30, 2014, at 3:40 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> >>>><br>
> >>>> On Fri, May 30, 2014 at 3:37 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
> >>>>><br>
> >>>>>> On May 30, 2014, at 3:31 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> >>>>>><br>
> >>>>>> On Fri, May 30, 2014 at 3:12 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
> >>>>>>> On Fri, May 30, 2014 at 3:03 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> >>>>>>>> On Fri, May 30, 2014 at 3:00 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a>> wrote:<br>
> >>>>>>>>><br>
> >>>>>>>>> On Fri, May 30, 2014 at 12:45 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">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" target="_blank">aprantl@apple.com</a>> wrote:<br>
> >>>>>>>>>>><br>
> >>>>>>>>>>>> On May 29, 2014, at 10:03 PM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com" target="_blank">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>
> 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>


><br>
> Could you elaborate on line table solution?<br>
<br>
</div></div>Epilogue is everything that is dominated by the last instruction that has a DebugLoc attached to it. This incidentally also happens to be the debugger’s interpretation.<br></blockquote><div><br></div><div>But this is not true - I see that every instruction in the epilogue (including the ones that restore stack and frame pointer, and the return instruction itself) have the valid DebugLoc attached to them.</div>
</div></div></div></div></blockquote><div><br></div></div></div><div>You're right. We’d have to scan back from the last instruction to find the first instruction that has a different DebugLoc. While not an epitome of elegance, this seems to work fine:</div>
</div></div></blockquote><div><br></div><div>SG. I've implemented this suggestion and uploaded the new version of patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><br></div><div><div><font face="Menlo">/// Return an iterator pointing to the first epilogue instruction in</font></div><div><font face="Menlo">/// MBB by looking at the instructions' debug location. Returns</font></div>
<div><font face="Menlo">/// MBB->end() if there are no epilogue instructions.</font></div><div><font face="Menlo">static MachineBasicBlock::const_iterator</font></div><div><font face="Menlo">getFirstEpilogueInstr(const MachineBasicBlock &MBB) {</font></div>
<div><font face="Menlo">  const auto Last = MBB.getLastNonDebugInstr();</font></div><div><font face="Menlo">  bool isReturnBlock = MBB.succ_empty();</font></div></div></div></div></blockquote><div><br></div><div>^^</div><div>
I think we should check that Last is return instruction here. We can have unreachable basic blocks,</div><div>or basic blocks ending with noreturn calls. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div><font face="Menlo">  if (!isReturnBlock || Last == MBB.end())</font></div><div><font face="Menlo">    return MBB.end();</font></div><div><font face="Menlo"><br></font></div>
<div><font face="Menlo">  DebugLoc LastLoc = Last->getDebugLoc();</font></div><div><font face="Menlo">  MachineBasicBlock::const_reverse_iterator First(Last);</font></div><div><font face="Menlo">  for (auto MI = First; MI != MBB.instr_rend(); ++MI) {</font></div>
<div><font face="Menlo">    if (MI->getDebugLoc() != LastLoc)</font></div><div><font face="Menlo">      return MachineBasicBlock::const_iterator(&*First);</font></div><div><font face="Menlo">    First = MI;</font></div>
<div><font face="Menlo">  }</font></div><div><font face="Menlo">  return MBB.begin();</font></div><div><font face="Menlo">}</font></div><div><font face="Menlo"><br></font></div><div><font face="Menlo">void calculateDbgValueHistory(const MachineFunction *MF,</font></div>
<div><font face="Menlo">                              const TargetRegisterInfo *TRI,</font></div><div><font face="Menlo">                              DbgValueHistoryMap &Result) {</font></div><div><font face="Menlo">  // Collect registers that are modified in the function body (their contents</font></div>
<div><font face="Menlo">  // is changed after the frame setup and before the last basic block).</font></div><div><font face="Menlo">  std::set<unsigned> ChangingRegs;</font></div><div><font face="Menlo">  for (const auto &MBB : *MF) {</font></div>
<div><font face="Menlo">    for (auto MI = MBB.instr_begin(); MI != getFirstEpilogueInstr(MBB); ++MI) {</font></div><div><font face="Menlo">      if (!MI->getFlag(MachineInstr::FrameSetup) && &MBB != &MF->back())</font></div>
<div><font face="Menlo">        collectClobberedRegisters(*MI, TRI, ChangingRegs);</font></div><div><font face="Menlo">    }</font></div><div><font face="Menlo">  }</font></div><div>    ...</div></div><div><div><br></div>
<div>cheers,</div><div>Adrian</div></div><div><div class="h5"><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
><br>
><br>
> -- adrian<br>
><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" target="_blank">vonosmas@gmail.com</a><br>
> >>><br>
> >>><br>
> >>><br>
> >>><br>
> >>> --<br>
> >>> Alexey Samsonov<br>
> >>> <a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> --<br>
> >> Alexey Samsonov<br>
> >> <a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a><br>
> ><br>
><br>
><br>
><br>
><br>
> --<br>
> Alexey Samsonov<br>
> <a href="mailto:vonosmas@gmail.com" target="_blank">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>
</div></blockquote></div></div></div><br></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>