[PATCH] Add a "probe-stack" attribute

Tue Oct 14 13:41:34 PDT 2014

I've CC'd Nadav Rotem. Hopefully he can find someone to review the x86 changes.

I'll leave out the optimizations for now because of the issue raised.
I've attached the patches I'd like to have merged.

- John Kåre

On Thu, Sep 18, 2014 at 1:53 AM, Philip Reames
<listmail at philipreames.com> wrote:
> John & Anton,
>
> I would be perfectly happy to have another reviewer take over here if you'd
> prefer and can find someone willing to do so.  So far, no one else has
> spoken up.  I'm not prepared to commit the code as of the last revision I
> saw.  I'm happy to work with you towards that goal, but recognize the
> (reasonable) frustration of working with a reviewer who is not familiar with
> the code being changed and is thus forced to be conservative about
> understanding each piece.
>
> My apologies if the process has been unnecessarily frustrating. It's
> entirely possible I could have handled this better.  If you have specific
> suggestions, feel free to send me a private email off list.  I'm always
> happy to hear constructive criticism and learn from past experiences.
>
> p.s. There was a potential issue brought up with the optimization change.
> I'm just calling your attention to the matter in case you missed it.
> http://reviews.llvm.org/D5018#inline-43027
>
> Philip
>
>
>
>
> On 09/16/2014 02:30 AM, Anton Korobeynikov wrote:
>>
>> I would suggest that someone from code owners review these changes.
>>
>> On Tue, Sep 16, 2014 at 4:44 AM, John Kåre Alsaker
>> <john.mailinglists at gmail.com> wrote:
>>>
>>> I would prefer to see the important feature land first (that is the first
>>> 4
>>> commits here
>>> https://github.com/Zoxc/llvm/compare/llvm-mirror:master...stprobe) since
>>> I
>>> actually need those and not the optimization.
>>>
>>> On Fri, Aug 22, 2014 at 7:20 PM, Philip Reames
>>> <listmail at philipreames.com>
>>> wrote:
>>>>
>>>>
>>>> On 08/22/2014 12:43 AM, John Kåre Alsaker wrote:
>>>>
>>>> On Fri, Aug 22, 2014 at 2:01 AM, Philip Reames
>>>> <listmail at philipreames.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 08/18/2014 12:40 PM, John Kåre Alsaker wrote:
>>>>>
>>>>> On Mon, Aug 18, 2014 at 8:31 PM, Philip Reames
>>>>> <listmail at philipreames.com> wrote:
>>>>>>
>>>>>>
>>>>>> 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> 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?
>>>>>
>>>>> I'd like you to submit them.
>>>>>
>>>>> I've landed the first couple of minor changes from the series.  Mostly
>>>>> as
>>>>> a show of good faith since the later items still need work.  If I
>>>>> missed
>>>>> something you think should have landed (i.e. got a LGTM), let me know.
>>>>>
>>>>> I split the 5 page optimization into it's own review and added a couple
>>>>> of folks with Windows experience.  I was all the way up to submitting
>>>>> it and
>>>>> realized I didn't feel comfortable doing so.  The thread is "Fast-path
>>>>> for
>>>>> stack probes on smaller frames" at http://reviews.llvm.org/D5018.
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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?
>>>>>
>>>>> I'm being consistent across x86 while MSVC is not.
>>>>>
>>>>> Breaking compatibility with the platform compiler is almost never
>>>>> acceptable and requires a substantial justification.  If you want to
>>>>> make
>>>>> the argument that that is the right direction to take, you'll need to
>>>>> find
>>>>> someone else to review your work.  I am nowhere *near* knowledgeable
>>>>> enough
>>>>> in this area to approve such a change.
>>>>
>>>> It does not break anything. __probestack is a new non-Windows function.
>>>> On
>>>> Windows the existing methods are called as before (and "probe-stack" is
>>>> a
>>>> no-op).
>>>>
>>>> Thank you for the clarification.  I had misunderstood your point.
>>>>
>>>> I'll take another look at submitting this once the 5-page opt goes in.
>>>>
>>>>
>>>>>
>>>>> What is your actual use case?  Are you trying to do stack probing on
>>>>> non-windows platforms?  Are you using a custom runtime on Windows?  I'm
>>>>> asking for background so I can make constructive suggestions.
>>>>>
>>>>>
>>>>>> In addition it doesn't assume RAX never is live in.
>>>>>>
>>>>>> Why?  Is this a bug fix?
>>>>>
>>>>> That depends on if it ever can be live in. There were already logic for
>>>>> EAX which I generalized.
>>>>>
>>>>> Is this change needed for anything else?  I would prefer not to change
>>>>> code without reason.  Doing so only increases risk.
>>>>
>>>> I don't know if the existing assumption that RAX is never live-in holds
>>>> now that this code can be used on non-Windows platforms.
>>>>
>>>> OK, that makes sense.
>>>>
>>>>
>>>>>> 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.
>>>>>
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-a-probe-stack-attribute.patch
Type: text/x-patch
Size: 3171 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141014/f8480f64/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-x86-Add-support-for-probe-stack.patch
Type: text/x-patch
Size: 6648 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141014/f8480f64/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-x86-Don-t-assume-that-RAX-is-never-live-in.patch
Type: text/x-patch
Size: 3602 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141014/f8480f64/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-x86-Ensure-the-redzone-is-not-used-with-stack-probes.patch
Type: text/x-patch
Size: 2393 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141014/f8480f64/attachment-0003.bin>