code review 83410044: cmd/gc, runtime: make GODEBUG=gcdead=1 mode work with l...

cmd/gc, runtime: make GODEBUG=gcdead=1 mode work with liveness

Trying to make GODEBUG=gcdead=1 work with liveness
and in particular ambiguously live variables.

1. In the liveness computation, mark all ambiguously live
variables as live for the entire function, except the entry.
They are zeroed directly after entry, and we need them not
to be poisoned thereafter.

2. In the liveness computation, compute liveness (and deadness)
for all parameters, not just pointer-containing parameters.
Otherwise gcdead poisons untracked scalar parameters and results.

3. Fix liveness debugging print for -live=2 to use correct bitmaps.
(Was not updated for compaction during compaction CL.)

4. Correct varkill during map literal initialization.
Was killing the map itself instead of the inserted value temp.

5. Disable aggressive varkill cleanup for call arguments if
the call appears in a defer or go statement.

6. In the garbage collector, avoid bug scanning empty
strings. An empty string is two zeros. The multiword
code only looked at the first zero and then interpreted
the next two bits in the bitmap as an ordinary word bitmap.
For a string the bits are 11 00, so if a live string was zero
length with a 0 base pointer, the poisoning code treated
the length as an ordinary word with code 00, meaning it
needed poisoning, turning the string into a poison-length
string with base pointer 0. By the same logic I believe that
a live nil slice (bits 11 01 00) will have its cap poisoned.
Always scan full multiword struct.

7. In the runtime, treat both poison words (PoisonGC and
PoisonStack) as invalid pointers that warrant crashes.

Manual testing as follows:

- Create a script called gcdead on your PATH containing:

        #!/bin/bash
        GODEBUG=gcdead=1 GOGC=10 GOTRACEBACK=2 exec "$@"
- Now you can build a test and then run 'gcdead ./foo.test'.
- More importantly, you can run 'go test -short -exec gcdead std'
   to run all the tests.

Fixes issue 7676.

While here, enable the precise scanning of slices, since that was
disabled due to bugs like these. That now works, both with and
without gcdead.

Fixes issue 7549.

[rsc, 2014-04-03 04:33:15]

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

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

[rsc, 2014-04-03 15:27:27]

I changed plive.c to enable recording slices as slices, not as
ptr+scalar+scalar. It works. Likely bug #6 was one of the reasons it didn't
before.

[khr, 2014-04-03 23:22:35]

LGTM

https://codereview.appspot.com/83410044/diff/90001/src/cmd/gc/plive.c
File src/cmd/gc/plive.c (right):

https://codereview.appspot.com/83410044/diff/90001/src/cmd/gc/plive.c#newcode...
src/cmd/gc/plive.c:1127: bvset(bv, (*xoffset / widthptr) * BitsPerPointer + 4); 
// 1 = live scalar
With your change in mgc0.c, you don't need this line here any more.
(or vice versa, the additional bit shift for slices is not needed if you leave
this here).

https://codereview.appspot.com/83410044/diff/90001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/83410044/diff/90001/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:1492: if(((String*)p)->len != 0)
   markonly(((String*)p)->str);

[rsc, 2014-04-04 00:26:23]

retested all.bash and 'go test -short -exec gcdead std'.
submitting.

modulo undiscovered bugs, the liveness stuff should be done for go 1.3 now.

https://codereview.appspot.com/83410044/diff/90001/src/cmd/gc/plive.c
File src/cmd/gc/plive.c (right):

https://codereview.appspot.com/83410044/diff/90001/src/cmd/gc/plive.c#newcode...
src/cmd/gc/plive.c:1127: bvset(bv, (*xoffset / widthptr) * BitsPerPointer + 4); 
// 1 = live scalar
On 2014/04/03 23:22:35, khr wrote:
> With your change in mgc0.c, you don't need this line here any more.
> (or vice versa, the additional bit shift for slices is not needed if you leave
> this here).

Thanks. Removed the bit here and the else above.

https://codereview.appspot.com/83410044/diff/90001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/83410044/diff/90001/src/pkg/runtime/mgc0.c#new...
src/pkg/runtime/mgc0.c:1492: if(((String*)p)->len != 0)
On 2014/04/03 23:22:35, khr wrote:
>    markonly(((String*)p)->str);

Done.

[rsc, 2014-04-04 00:33:30]

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

cmd/gc, runtime: make GODEBUG=gcdead=1 mode work with liveness

Trying to make GODEBUG=gcdead=1 work with liveness
and in particular ambiguously live variables.

1. In the liveness computation, mark all ambiguously live
variables as live for the entire function, except the entry.
They are zeroed directly after entry, and we need them not
to be poisoned thereafter.

2. In the liveness computation, compute liveness (and deadness)
for all parameters, not just pointer-containing parameters.
Otherwise gcdead poisons untracked scalar parameters and results.

3. Fix liveness debugging print for -live=2 to use correct bitmaps.
(Was not updated for compaction during compaction CL.)

4. Correct varkill during map literal initialization.
Was killing the map itself instead of the inserted value temp.

5. Disable aggressive varkill cleanup for call arguments if
the call appears in a defer or go statement.

6. In the garbage collector, avoid bug scanning empty
strings. An empty string is two zeros. The multiword
code only looked at the first zero and then interpreted
the next two bits in the bitmap as an ordinary word bitmap.
For a string the bits are 11 00, so if a live string was zero
length with a 0 base pointer, the poisoning code treated
the length as an ordinary word with code 00, meaning it
needed poisoning, turning the string into a poison-length
string with base pointer 0. By the same logic I believe that
a live nil slice (bits 11 01 00) will have its cap poisoned.
Always scan full multiword struct.

7. In the runtime, treat both poison words (PoisonGC and
PoisonStack) as invalid pointers that warrant crashes.

Manual testing as follows:

- Create a script called gcdead on your PATH containing:

        #!/bin/bash
        GODEBUG=gcdead=1 GOGC=10 GOTRACEBACK=2 exec "$@"
- Now you can build a test and then run 'gcdead ./foo.test'.
- More importantly, you can run 'go test -short -exec gcdead std'
   to run all the tests.

Fixes issue 7676.

While here, enable the precise scanning of slices, since that was
disabled due to bugs like these. That now works, both with and
without gcdead.

Fixes issue 7549.

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

[gobot, 2014-04-04 00:34:44]

This CL appears to have broken the freebsd-amd64 builder.
See http://build.golang.org/log/2aae5bb946e02bc384584ebf2e7e44f4d2dd8d06

[0intro, 2014-04-04 04:30:58]

It seems this change broke the Plan 9 build.

src/pkg/math/abs.go:14: internal error: abs ~r1 (type float64) recorded as live
on entry
src/pkg/math/acosh.go:42: internal error: Acosh ~r1 (type float64) recorded as
live on entry
src/pkg/math/asin.go:21: internal error: asin ~r1 (type float64) recorded as
live on entry
src/pkg/math/asin.go:53: internal error: acos ~r1 (type float64) recorded as
live on entry

[0intro, 2014-04-04 04:52:28]

It seems to be related to this change in src/cmd/gc/plive.c:

 	switch(ll->n->class) {
 	case PAUTO:
+		if(haspointers(ll->n->type))
+			arrayadd(result, &ll->n);
+		break;
 	case PPARAM:
 	case PPARAMOUT:
-		if(haspointers(ll->n->type))
-			arrayadd(result, &ll->n);
+		arrayadd(result, &ll->n);
 		break;
 	}

[khr1, 2014-04-04 06:20:31]

I cannot reproduce this error.  Running:

env GOOS=plan9 ./make.bash
env GOOS=plan9 ../bin/go tool 6g -S pkg/math/abs.go

Works just fine.  Adding some -live directives prints the right thing.  ~r1
is not live at entry, as expected.

Can you try to resync the builder?  I have the feeling it is in a bad
state.  Or accurate cross-compilation to plan9 is broken, which would also
be a bad state.


On Thu, Apr 3, 2014 at 9:52 PM, <0intro@gmail.com> wrote:

> It seems to be related to this change in src/cmd/gc/plive.c:
>
>         switch(ll->n->class) {
>         case PAUTO:
> +               if(haspointers(ll->n->type))
> +                       arrayadd(result, &ll->n);
> +               break;
>         case PPARAM:
>         case PPARAMOUT:
> -               if(haspointers(ll->n->type))
> -                       arrayadd(result, &ll->n);
> +               arrayadd(result, &ll->n);
>                 break;
>         }
>
>
> https://codereview.appspot.com/83410044/
>

[0intro, 2014-04-04 06:27:37]

> Can you try to resync the builder?  I have the feeling it is in a bad
state.

Building on another Plan 9 machine lead to the same failure.

-- 
David du Colombier

[0intro, 2014-04-04 06:31:16]

> I cannot reproduce this error.  Running:
>
> env GOOS=plan9 ./make.bash
> env GOOS=plan9 ../bin/go tool 6g -S pkg/math/abs.go
>
> Works just fine.  Adding some -live directives prints the right thing.
 ~r1 is not live at entry, as expected.

Cross-compiling on Unix might lead to a different behavior, because the
toolchain is built using GCC. Also, you tried to build for plan9/amd64 but
not plan9/386.

-- 
David du Colombier

[khr1, 2014-04-04 07:31:47]

I also tried GOOS=plan9 GOARCH=386, same deal.



On Thu, Apr 3, 2014 at 11:31 PM, David du Colombier <0intro@gmail.com>wrote:

> > I cannot reproduce this error.  Running:
> >
> > env GOOS=plan9 ./make.bash
> > env GOOS=plan9 ../bin/go tool 6g -S pkg/math/abs.go
> >
> > Works just fine.  Adding some -live directives prints the right thing.
>  ~r1 is not live at entry, as expected.
>
> Cross-compiling on Unix might lead to a different behavior, because the
> toolchain is built using GCC. Also, you tried to build for plan9/amd64 but
> not plan9/386.
>
> --
> David du Colombier
>

[0intro, 2014-04-04 08:09:19]

> I also tried GOOS=plan9 GOARCH=386, same deal.

Yes, I did the same on my side. It's seems this issue only appears on Plan 9.

-- 
David du Colombier

[rsc, 2014-04-04 13:10:12]

This is going to be a bug in the Plan 9 C compiler miscompiling plive.c.

Russ

[aram2, 2014-04-04 13:19:54]

I'm not near any Plan 9 terminal right now, could you paste the
disassembly of getvariables?

-- 
Aram Hăvărneanu

[0intro, 2014-04-04 13:46:07]

> I'm not near any Plan 9 terminal right now, could you paste the
> disassembly of getvariables?

I've attached the disassembly of plive.c.

-- 
David du Colombier

[rsc, 2014-04-04 14:09:32]

The problem is not in getvariables. The change to getvariables is working
correctly: it enables tracking of the return values from these functions.
The problem is that the tracking of these variables - which is needed but
was not being done before this CL - is compiling incorrectly. The bug is
somewhere else. It's usually something about integer promotions, which are
just wrong in that compiler, but I can't think where there'd be a
problematic one in this code.

Russ

[0intro, 2014-04-04 20:50:39]

The build failure is still present after reverting everything,
except the getvariables change, so I suppose the bug was already
present in the code before.

-- 
David du Colombier

[0intro, 2014-04-05 09:50:23]

for(j = 0; j < liveout->n; j++) {
	if(!bvget(liveout, j))
		continue;
	n = *(Node**)arrayget(lv->vars, j);
	if(n->class != PPARAM)
		yyerrorl(p->lineno, "internal error: %N %lN recorded as live on entry",
curfn->nname, n);
}

When the issue appears, liveout->n is equal to 2 in this loop.

In the first iteration, n->class is PPARAM. In the second
iteration n->class is PPARAMOUT and bvget returns 0 on Linux,
while it returns 1 on Plan 9.

If I look more closely to bvget, on Linux I get:

i: 0, bv->b[0]: 1, mask: 1, word: 1, return: 1
i: 1, bv->b[0]: 1, mask: 2, word: 0, return: 0

While on Plan 9, I get:

i: 0, bv->b[0]: 3, mask: 1, word: 1, return: 1
i: 1, bv->b[0]: 3, mask: 2, word: 2, return: 1

So, it seems liveout->b got the wrong values somewhere.

-- 
David du Colombier

[0intro, 2014-04-06 09:41:15]

I've noticed info.flags contains the wrong value in
plive.c:/^progeffects, so it takes the wrong branch here
and bvset is called with the wrong instruction.

-- 
David du Colombier

[rsc, 2014-04-06 14:22:15]

This is not the Plan 9 C compiler's fault, at least not directly. The
problem is that the 386 make.bash decides whether to use SSE based on
whether the compiling system supports it. On Plan 9 cmd/dist is hard-wired
to say no it does not. So Plan 9 is basically the only system using
GO386=387 instead of GO386=sse. And some of the 387 instructions (for
example FMOVDP) are recorded in proginfo as RightAddr, which makes the
liveness analysis think the previous value is needed. I will fix this.

It sounds like we need a linux/387 builder. It would be nice to have
another 387 system.

Russ

[rsc, 2014-04-06 15:07:54]

I submitted CL 84570045, which did in fact fix the Plan 9 build. I am happy
it was not a Plan 9 compiler bug, and I am happy the Plan 9 port helped us
find a real bug. I also filed issue 7220 to remind us to get a 387-only
builder on another operating system.

Thanks.
Russ

[0intro, 2014-04-06 16:12:24]

Thanks Russ!

-- 
David du Colombier