[PATCH] Add a "probe-stack" attribute

[Philip Reames]

On 08/15/2014 01:55 PM, John Kåre Alsaker wrote:
> On Fri, Aug 15, 2014 at 6:47 PM, Philip Reames 
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
>     Sorry for the delay.
>
>     Patch 0002 LGTM.  Please submit
>     Patch 0003 LGTM.  Please submit.
>
> Yes, please submit.
Do you have commit access?  Or do you want me to submit on your behalf?
>
>
>     Patch 0001 LGTM, but doesn't do anything by itself. Please hold it
>     until either 0004 or 0006 are ready to land.
>
>     Patch 0004
>     - It seems like you're strill mixing several functional changes in
>     0004.  Can you enumerate (in email) what this patch is trying to
>     accomplish?  The trivial implementation I expected is to add
>     MF.shouldProbeStack in a couple places and call it done.  Why is
>     the rest needed?
>
> It's just generalizing the code for Win64 stack probes to x86-32 and 
> x86-64. The __probestack calling convention on both x86-32 and x86-64 
> is the same as __chkstk on Win64.
Why?  Is there a good reason now to use the x86-32 chkstk convention on 
x86-32 probestack?  What's the motivation for being different?
> In addition it doesn't assume RAX never is live in.
Why?  Is this a bug fix?
> It also disables the redzone (which never is used on Windows) since 
> the probing code doesn't account for that.
Ok.  I see the value in this.

This sounds like at least three distinct changes. Please separate them 
and we'll review each in turn.

Sorry for being so pedantic about separation here.  I'm trying to review 
code I don't really know well.  As a result, I'm trying to be extremely 
carefully about making sure I understand each piece.  If you can find 
another reviewer who knows this code better, I'd be happy to step aside.
>
>
>
>     Patch 0005
>     - 0x5000 - Please write this as " 5 * PageSize".  I won't ask you
>     to make page size configurable right now, but it should be clear
>     to someone who might want to extend it what this is coming from. 
>     Also, *why* 5?
>
> 5 is what GCC uses before turning the code into a loop.
Document that motivation please.
>
>     - "0x1000" should be replaced with "PageSize" to make it more
>     understandable what's happening here.
>
> Ok
>
>     - I don't think your addressing is quite right.  If RSP before
>     adjustment was exactly on a page boundary, wouldn't you miss
>     touching the first page entirely?
>
> No, we'd write to the first word of the page.
Assume the original RSP was page aligned (after pushing the RetPC), 
NumBytes is exactly one page.  Your code would compute an offset which 
is exactly one page off from the new RSP.  This would be in the same 
page as the old RSP, not the new one.  In theory, you could then push 
another frame and miss touching the page.

0x1000 - ret PC, touch location
0x2000 - new RSP

I may be wrong here, but if so, why?

>     - To make the loop more readable, I might restructure it as:
>     for(int Offset = NumBytes - PageSize + 1; Offset >= 0; Offset -=
>     PageSize) { touch RSP+Offset }
>
> I like the clarity on the amount of probes done.
>
>     - After reading up a bit about what chkstk does internally on
>     windows (i.e. internal probe stack), the optimization seems
>     clearly correct.  It would be nice to explain that in a comment. 
>     Don't assume everyone who reads the code knows how chkstk manages
>     stack expansion.
>
> Sure
>
>     - With these changes, LGTM.   I believe this is independent of
>     0004 right?
>
> It is.
>
>
>     Patch 0006 LGTM.
>
>     Patch 0007 LGTM, but you should get someone who's familiar with
>     the ARM backend to review.
>
>     Patch 0008 I don't understand.  Why is this correct?
>
> chkstk_ms on Cygwin/MinGW is the same as chkstk on MSVC, so we can 
> reuse the code for the call to the MSVC version.
>
>
>
>     Philip
>
>
>     On 08/07/2014 02:23 AM, John Kåre Alsaker wrote:
>
>         This is a continuation of http://reviews.llvm.org/D4717
>
>         It seems Phabricator is useless with multiple patches.
>
>         I've fixed the points raised in D4717 and separated out
>         whitespaces changes to make review easier.
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140818/63fd2451/attachment.html>