[PATCH] Add a "probe-stack" attribute

Wed Jan 7 18:37:34 PST 2015

I've rebased the patches.

On Thu, Nov 6, 2014 at 5:02 AM, John Kåre Alsaker
<john.mailinglists at gmail.com> wrote:
> Poke.
>
> On Tue, Oct 14, 2014 at 10:41 PM, John Kåre Alsaker
> <john.mailinglists at gmail.com> wrote:
>> 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/20150108/d8f95a9b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-x86-Add-support-for-probe-stack.patch
Type: text/x-patch
Size: 6666 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150108/d8f95a9b/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/20150108/d8f95a9b/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: 2399 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150108/d8f95a9b/attachment-0003.bin>