code review 139380043: runtime: let stack copier update Panic structs for us

runtime: let stack copier update Panic structs for us

It already is updating parts of them; we're just getting lucky
retraversing them and not finding much to do.
Change argp to a pointer so that it will be updated too.
Existing tests break if you apply the change to adjustpanics
without also updating the type of argp.

[rsc, 2014-09-08 23:27:22]

Hello khr (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[khr1, 2014-09-09 00:45:24]

LGTM.


On Mon, Sep 8, 2014 at 4:27 PM, <rsc@golang.org> wrote:

> Reviewers: khr,
>
> Message:
> Hello khr (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: let stack copier update Panic structs for us
>
> It already is updating parts of them; we're just getting lucky
> retraversing them and not finding much to do.
> Change argp to a pointer so that it will be updated too.
> Existing tests break if you apply the change to adjustpanics
> without also updating the type of argp.
>
> Please review this at https://codereview.appspot.com/139380043/
>
> Affected files (+13, -15 lines):
>   M src/runtime/panic.go
>   M src/runtime/runtime.h
>   M src/runtime/stack.c
>
>
> Index: src/runtime/panic.go
> ===================================================================
> --- a/src/runtime/panic.go
> +++ b/src/runtime/panic.go
> @@ -287,9 +287,9 @@
>                 gp._defer = (*_defer)(noescape(unsafe.Pointer(&dabort)))
>                 p._defer = d
>
> -               p.argp = getargp(0)
> +               p.argp = unsafe.Pointer(getargp(0))
>                 reflectcall(unsafe.Pointer(d.fn),
> unsafe.Pointer(&d.args), uint32(d.siz), uint32(d.siz))
> -               p.argp = 0
> +               p.argp = nil
>
>                 // reflectcall did not panic. Remove dabort.
>                 if gp._defer != &dabort {
> @@ -362,7 +362,7 @@
>         // If they match, the caller is the one who can recover.
>         gp := getg()
>         p := gp._panic
> -       if p != nil && !p.recovered && argp == p.argp {
> +       if p != nil && !p.recovered && argp == uintptr(p.argp) {
>                 p.recovered = true
>                 return p.arg
>         }
> Index: src/runtime/runtime.h
> ===================================================================
> --- a/src/runtime/runtime.h
> +++ b/src/runtime/runtime.h
> @@ -653,7 +653,7 @@
>   */
>  struct Panic
>  {
> -       uintptr argp;   // pointer to arguments of deferred call run
> during panic; cannot move - known to liblink
> +       void*   argp;   // pointer to arguments of deferred call run
> during panic; cannot move - known to liblink
>         Eface   arg;            // argument to panic
>         Panic*  link;           // link to earlier panic
>         Defer*  defer;          // current executing defer
> Index: src/runtime/stack.c
> ===================================================================
> --- a/src/runtime/stack.c
> +++ b/src/runtime/stack.c
> @@ -712,6 +712,10 @@
>                         // get adjusted appropriately.
>                         // This only happens for runtime.main and
> runtime.gopanic now,
>                         // but a compiler optimization could do more of
> this.
> +                       // If such an optimization were introduced,
> Defer.argp should
> +                       // change to have pointer type so that it will be
> updated by
> +                       // the stack copying. Today both of those on-stack
> defers
> +                       // set argp = NoArgs, so no adjustment is
> necessary.
>                         *dp = (Defer*)((byte*)d + adjinfo->delta);
>                         continue;
>                 }
> @@ -751,17 +755,11 @@
>  static void
>  adjustpanics(G *gp, AdjustInfo *adjinfo)
>  {
> -       Panic *p, **l;
> -
> -       // only the topmost panic is on the current stack
> -       for(l = &gp->panic; (p = *l) != nil; ) {
> -               if(adjinfo->oldstk <= (byte*)p && (byte*)p <
> adjinfo->oldbase)
> -                       *l = (Panic*)((byte*)p + adjinfo->delta);
> -               l = &p->link;
> -
> -               if(adjinfo->oldstk <= (byte*)p->argp && (byte*)p->argp <
> adjinfo->oldbase)
> -                       p->argp += adjinfo->delta;
> -       }
> +       // Panic structs are all on the stack
> +       // and are adjusted by stack copying.
> +       // The only pointer we need to update is gp->panic, the head of
> the list.
> +       if(adjinfo->oldstk <= (byte*)gp->panic && (byte*)gp->panic <
> adjinfo->oldbase)
> +               gp->panic = (Panic*)((byte*)gp->panic + adjinfo->delta);
>  }
>
>  static void
>
>
>

[rsc, 2014-09-09 01:02:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=e2ec5172543b ***

runtime: let stack copier update Panic structs for us

It already is updating parts of them; we're just getting lucky
retraversing them and not finding much to do.
Change argp to a pointer so that it will be updated too.
Existing tests break if you apply the change to adjustpanics
without also updating the type of argp.

LGTM=khr
R=khr
CC=golang-codereviews
https://codereview.appspot.com/139380043

[gobot, 2014-09-09 04:15:01]

This CL appears to have broken the darwin-amd64-race-cheney builder.
See http://build.golang.org/log/953a8ac4edfa0aae226a279f27dfb8f571cc4949

[bradfitz, 2014-09-09 04:32:29]

Not really.


On Mon, Sep 8, 2014 at 9:15 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the darwin-amd64-race-cheney builder.
> See http://build.golang.org/log/953a8ac4edfa0aae226a279f27dfb8f571cc4949
>
>
> https://codereview.appspot.com/139380043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>