code review 80160044: cmd/gc: liveness-related bug fixes

cmd/gc: liveness-related bug fixes

1. On entry to a function, only zero the ambiguously live stack variables.
Before, we were zeroing all stack variables containing pointers.
The zeroing is pretty inefficient right now (issue 7624), but there are also
too many stack variables detected as ambiguously live (issue 7345),
and that must be addressed before deciding how to improve the zeroing code.
(Changes in 5g/ggen.c, 6g/ggen.c, 8g/ggen.c, gc/pgen.c)

Fixes issue 7647.

2. Make the regopt word-based liveness analysis preserve the
whole-variable liveness property expected by the garbage collection
bitmap liveness analysis. That is, if the regopt liveness decides that
one word in a struct needs to be preserved, make sure it preserves
the entire struct. This is particularly important for multiword values
such as strings, slices, and interfaces, in which all the words need
to be present in order to understand the meaning.
(Changes in 5g/reg.c, 6g/reg.c, 8g/reg.c.)

Fixes issue 7591.

3. Make the regopt word-based liveness analysis treat a variable
as having its address taken - which makes it preserved across
all future calls - whenever n->addrtaken is set, for consistency
with the gc bitmap liveness analysis, even if there is no machine
instruction actually taking the address. In this case n->addrtaken
is incorrect (a nicer way to put it is overconservative), and ideally
there would be no such cases, but they can happen and the two
analyses need to agree.
(Changes in 5g/reg.c, 6g/reg.c, 8g/reg.c; test in bug484.go.)

Fixes crashes found by turning off "zero everything" in step 1.

4. Remove spurious VARDEF annotations. As the comment in
gc/pgen.c explains, the VARDEF must immediately precede
the initialization. It cannot be too early, and it cannot be too late.
In particular, if a function call sits between the VARDEF and the
actual machine instructions doing the initialization, the variable
will be treated as live during that function call even though it is
uninitialized, leading to problems.
(Changes in gc/gen.c; test in live.go.)

Fixes crashes found by turning off "zero everything" in step 1.

5. Do not treat loading the address of a wide value as a signal
that the value must be initialized. Instead depend on the existence
of a VARDEF or the first actual read/write of a word in the value.
If the load is in order to pass the address to a function that does
the actual initialization, treating the load as an implicit VARDEF
causes the same problems as described in step 4.
The alternative is to arrange to zero every such value before
passing it to the real initialization function, but this is a much
easier and more efficient change.
(Changes in gc/plive.c.)

Fixes crashes found by turning off "zero everything" in step 1.

6. Treat wide input parameters with their address taken as
initialized on entry to the function. Otherwise they look
"ambiguously live" and we will try to emit code to zero them.
(Changes in gc/plive.c.)

Fixes crashes found by turning off "zero everything" in step 1.

7. An array of length 0 has no pointers, even if the element type does.
Without this change, the zeroing code complains when asked to
clear a 0-length array.
(Changes in gc/reflect.c.)

[rsc, 2014-03-27 01:08:27]

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

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

[khr, 2014-03-27 16:45:42]

LGTM.

Thanks for the verbose comments, it needs it.

https://codereview.appspot.com/80160044/diff/80001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (left):

https://codereview.appspot.com/80160044/diff/80001/src/cmd/gc/pgen.c#oldcode432
src/cmd/gc/pgen.c:432: stkptrsize = rnd(stkptrsize, widthreg);
What do we still use stkptrsize for?

[rsc, 2014-03-27 17:35:07]

On Thu, Mar 27, 2014 at 12:45 PM, <khr@golang.org> wrote:

> LGTM.
>
> Thanks for the verbose comments, it needs it.
>
> https://codereview.appspot.com/80160044/diff/80001/src/cmd/gc/pgen.c
> File src/cmd/gc/pgen.c (left):
>
> https://codereview.appspot.com/80160044/diff/80001/src/
> cmd/gc/pgen.c#oldcode432
> src/cmd/gc/pgen.c:432: stkptrsize = rnd(stkptrsize, widthreg);
> What do we still use stkptrsize for?
>

It tells us how much of the local stack frame contains pointers; the
liveness bitmaps are sized only to describe that area.

[rsc, 2014-03-27 18:05:56]

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

cmd/gc: liveness-related bug fixes

1. On entry to a function, only zero the ambiguously live stack variables.
Before, we were zeroing all stack variables containing pointers.
The zeroing is pretty inefficient right now (issue 7624), but there are also
too many stack variables detected as ambiguously live (issue 7345),
and that must be addressed before deciding how to improve the zeroing code.
(Changes in 5g/ggen.c, 6g/ggen.c, 8g/ggen.c, gc/pgen.c)

Fixes issue 7647.

2. Make the regopt word-based liveness analysis preserve the
whole-variable liveness property expected by the garbage collection
bitmap liveness analysis. That is, if the regopt liveness decides that
one word in a struct needs to be preserved, make sure it preserves
the entire struct. This is particularly important for multiword values
such as strings, slices, and interfaces, in which all the words need
to be present in order to understand the meaning.
(Changes in 5g/reg.c, 6g/reg.c, 8g/reg.c.)

Fixes issue 7591.

3. Make the regopt word-based liveness analysis treat a variable
as having its address taken - which makes it preserved across
all future calls - whenever n->addrtaken is set, for consistency
with the gc bitmap liveness analysis, even if there is no machine
instruction actually taking the address. In this case n->addrtaken
is incorrect (a nicer way to put it is overconservative), and ideally
there would be no such cases, but they can happen and the two
analyses need to agree.
(Changes in 5g/reg.c, 6g/reg.c, 8g/reg.c; test in bug484.go.)

Fixes crashes found by turning off "zero everything" in step 1.

4. Remove spurious VARDEF annotations. As the comment in
gc/pgen.c explains, the VARDEF must immediately precede
the initialization. It cannot be too early, and it cannot be too late.
In particular, if a function call sits between the VARDEF and the
actual machine instructions doing the initialization, the variable
will be treated as live during that function call even though it is
uninitialized, leading to problems.
(Changes in gc/gen.c; test in live.go.)

Fixes crashes found by turning off "zero everything" in step 1.

5. Do not treat loading the address of a wide value as a signal
that the value must be initialized. Instead depend on the existence
of a VARDEF or the first actual read/write of a word in the value.
If the load is in order to pass the address to a function that does
the actual initialization, treating the load as an implicit VARDEF
causes the same problems as described in step 4.
The alternative is to arrange to zero every such value before
passing it to the real initialization function, but this is a much
easier and more efficient change.
(Changes in gc/plive.c.)

Fixes crashes found by turning off "zero everything" in step 1.

6. Treat wide input parameters with their address taken as
initialized on entry to the function. Otherwise they look
"ambiguously live" and we will try to emit code to zero them.
(Changes in gc/plive.c.)

Fixes crashes found by turning off "zero everything" in step 1.

7. An array of length 0 has no pointers, even if the element type does.
Without this change, the zeroing code complains when asked to
clear a 0-length array.
(Changes in gc/reflect.c.)

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

[gobot, 2014-03-27 19:16:46]

This CL appears to have broken the windows-386-ec2 builder.
See http://build.golang.org/log/392be6308d6bd452f199013b1cdc12261e3d06b3

[rsc, 2014-03-28 02:21:26]

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

https://codereview.appspot.com/80160044/diff/100001/src/cmd/gc/plive.c#newcod...
src/cmd/gc/plive.c:723: if(info.flags & (LeftRead|LeftWrite))
This change was #5 in the long CL description.
It avoided treating taking the address of a wide variable as a signal that the
variable is live.
The idea was that if you had:

MOVL chanvar, AX
MOVL AX, 0(SP)
LEAL bigvar, AX
MOVL AX, 4(SP)
CALL chanrecv(SB) // bigvar not live here
USE bigvar // bigvar live when chanrecv returns

Unfortunately, this logic is bogus. bigvar is indeed not live (because not
initialized) at the start of the call to chanrecv, but it becomes live (and
initialized, and something worth preserving) during the call, before the call
returns. The garbage collector, because it uses the program counter, cannot
distinguish these two cases. In particular, if the sequence is:

- call chanrecv
- chanrecv copies into bigvar, marks goroutine runnable
- some other goroutine runs, garbage collects
- original goroutine becomes runnable, returns from chanrecv

Then the garbage collection in the middle treats bigvar as not live even though
it is holding the result from chanrecv. If it is the only reference to that
result, the result will be garbage collected prematurely. I believe this is what
is going on in the powser2 failures.

If I run powser2 with GODEBUG=gcdead=1 then I get a failure due to this bug
nearly immediately.

We will have to zero these blocks before making the call to chanrecv etc.