[PATCH] Add a "probe-stack" attribute

[John Kåre Alsaker]

Bump.

On Thu, Jan 8, 2015 at 3:37 AM, John Kåre Alsaker
<john.mailinglists at gmail.com> wrote:
> 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
>>>>>>
>>>>>
>>>>>
>>>>