[PATCH] Add a "probe-stack" attribute

Fri Jan 16 11:23:15 PST 2015

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150116/d399cd3f/attachment.html>