[PATCH] Add a "probe-stack" attribute

Fri Jan 16 12:43:33 PST 2015

Reid, I'm not so sure.

We already have a probe-stack-size attribute, how does this attribute
interact with that one?  Which targets have a __probestack that can be
called?

On Fri, Jan 16, 2015 at 11:23 AM, Reid Kleckner <rnk at google.com> wrote:

> patch 1 lgtm
> patch 2 lgtm
>
> I can apply those.
>
> Patches 3 and 4 handle edge cases worth having tests, for, though.
>
> On Wed, Jan 7, 2015 at 6:37 PM, 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
>> >>>>>
>> >>>>
>> >>>>
>> >>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150116/e4778163/attachment.html>