code review 116940043: runtime: pass correct size to malloc

runtime: pass correct size to malloc
In both cases we lie to malloc about the actual size that we need.
In panic we ask for less memory than we are going to use.
In slice we ask for more memory than we are going to use
(potentially asking for a fractional number of elements).
This breaks the new GC.

[dvyukov, 2014-07-20 11:17:31]

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

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

[dave_cheney.net, 2014-07-21 10:26:04]

testing on linux/arm now.

https://codereview.appspot.com/116940043/diff/40001/src/pkg/runtime/slice.goc
File src/pkg/runtime/slice.goc (right):

https://codereview.appspot.com/116940043/diff/40001/src/pkg/runtime/slice.goc...
src/pkg/runtime/slice.goc:127: capmem = ret->cap*typ->size;
I think this needs a short comment to explain how capmem can be unequal to a
multiple of type->size and the repercussions on the GC of having this fractional
allocation.

[dvyukov, 2014-07-21 12:14:51]

https://codereview.appspot.com/116940043/diff/40001/src/pkg/runtime/slice.goc
File src/pkg/runtime/slice.goc (right):

https://codereview.appspot.com/116940043/diff/40001/src/pkg/runtime/slice.goc...
src/pkg/runtime/slice.goc:127: capmem = ret->cap*typ->size;
On 2014/07/21 10:26:04, dfc wrote:
> I think this needs a short comment to explain how capmem can be unequal to a
> multiple of type->size and the repercussions on the GC of having this
fractional
> allocation.

Done.

[khr, 2014-07-21 19:44:53]

https://codereview.appspot.com/116940043/diff/60001/src/pkg/runtime/slice.goc
File src/pkg/runtime/slice.goc (right):

https://codereview.appspot.com/116940043/diff/60001/src/pkg/runtime/slice.goc...
src/pkg/runtime/slice.goc:125: ret->len = x.len;
I worry about setting len & cap before array is set.  This means GC might see a
[nil, nonzero, nonzero] slice.  Probably won't break the current gc, but it
would be safer for future changes to set all 3 fields atomically with respect to
GC.

Just moving them to after the mallocgc call should do it.

[dvyukov, 2014-07-21 21:00:03]

https://codereview.appspot.com/116940043/diff/60001/src/pkg/runtime/slice.goc
File src/pkg/runtime/slice.goc (right):

https://codereview.appspot.com/116940043/diff/60001/src/pkg/runtime/slice.goc...
src/pkg/runtime/slice.goc:125: ret->len = x.len;
On 2014/07/21 19:44:52, khr wrote:
> I worry about setting len & cap before array is set.  This means GC might see
a
> [nil, nonzero, nonzero] slice.  Probably won't break the current gc, but it
> would be safer for future changes to set all 3 fields atomically with respect
to
> GC.
> 
> Just moving them to after the mallocgc call should do it.

This problem is unsolvable with shuffling order. Consider that you have an
already initialized slice and want to assign a new value to it, whatever way you
assign new values you have an inconsistent slice during the operation. And with
concurrent GC you can't assume that you can look at 3 words atomically at all
(unless you protect all slice operations with a mutex, which is not a good
idea).
I think we need to leave it as is for now. If/when we change the GC, there will
be lots of work to do, and we know that the compiler, string.goc and slice.goc
are the places to check.

[khr1, 2014-07-21 21:14:41]

I realize we'll have to fix it for concurrent GC.  That's still a ways
away.  Having a safe point halfway through initializing the slice is
dangerous with the current GC, i.e. the GC that will ship with 1.4.  I'd
rather play it safe for the current release cycle.


On Mon, Jul 21, 2014 at 2:00 PM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.com/116940043/diff/60001/src/
> pkg/runtime/slice.goc
> File src/pkg/runtime/slice.goc (right):
>
> https://codereview.appspot.com/116940043/diff/60001/src/
> pkg/runtime/slice.goc#newcode125
> src/pkg/runtime/slice.goc:125: ret->len = x.len;
> On 2014/07/21 19:44:52, khr wrote:
>
>> I worry about setting len & cap before array is set.  This means GC
>>
> might see a
>
>> [nil, nonzero, nonzero] slice.  Probably won't break the current gc,
>>
> but it
>
>> would be safer for future changes to set all 3 fields atomically with
>>
> respect to
>
>> GC.
>>
>
>  Just moving them to after the mallocgc call should do it.
>>
>
> This problem is unsolvable with shuffling order. Consider that you have
> an already initialized slice and want to assign a new value to it,
> whatever way you assign new values you have an inconsistent slice during
> the operation. And with concurrent GC you can't assume that you can look
> at 3 words atomically at all (unless you protect all slice operations
> with a mutex, which is not a good idea).
> I think we need to leave it as is for now. If/when we change the GC,
> there will be lots of work to do, and we know that the compiler,
> string.goc and slice.goc are the places to check.
>
> https://codereview.appspot.com/116940043/
>

[dvyukov, 2014-07-21 21:44:09]

As you wish
PTAL

On 2014/07/21 21:14:41, khr1 wrote:
> I realize we'll have to fix it for concurrent GC.  That's still a ways
> away.  Having a safe point halfway through initializing the slice is
> dangerous with the current GC, i.e. the GC that will ship with 1.4.  I'd
> rather play it safe for the current release cycle.
> 
> 
> On Mon, Jul 21, 2014 at 2:00 PM, <mailto:dvyukov@google.com> wrote:
> 
> >
> > https://codereview.appspot.com/116940043/diff/60001/src/
> > pkg/runtime/slice.goc
> > File src/pkg/runtime/slice.goc (right):
> >
> > https://codereview.appspot.com/116940043/diff/60001/src/
> > pkg/runtime/slice.goc#newcode125
> > src/pkg/runtime/slice.goc:125: ret->len = x.len;
> > On 2014/07/21 19:44:52, khr wrote:
> >
> >> I worry about setting len & cap before array is set.  This means GC
> >>
> > might see a
> >
> >> [nil, nonzero, nonzero] slice.  Probably won't break the current gc,
> >>
> > but it
> >
> >> would be safer for future changes to set all 3 fields atomically with
> >>
> > respect to
> >
> >> GC.
> >>
> >
> >  Just moving them to after the mallocgc call should do it.
> >>
> >
> > This problem is unsolvable with shuffling order. Consider that you have
> > an already initialized slice and want to assign a new value to it,
> > whatever way you assign new values you have an inconsistent slice during
> > the operation. And with concurrent GC you can't assume that you can look
> > at 3 words atomically at all (unless you protect all slice operations
> > with a mutex, which is not a good idea).
> > I think we need to leave it as is for now. If/when we change the GC,
> > there will be lots of work to do, and we know that the compiler,
> > string.goc and slice.goc are the places to check.
> >
> > https://codereview.appspot.com/116940043/

[khr, 2014-07-21 21:45:05]

On 2014/07/21 21:44:09, dvyukov wrote:
> As you wish
> PTAL
> 
> On 2014/07/21 21:14:41, khr1 wrote:
> > I realize we'll have to fix it for concurrent GC.  That's still a ways
> > away.  Having a safe point halfway through initializing the slice is
> > dangerous with the current GC, i.e. the GC that will ship with 1.4.  I'd
> > rather play it safe for the current release cycle.
> > 
> > 
> > On Mon, Jul 21, 2014 at 2:00 PM, <mailto:dvyukov@google.com> wrote:
> > 
> > >
> > > https://codereview.appspot.com/116940043/diff/60001/src/
> > > pkg/runtime/slice.goc
> > > File src/pkg/runtime/slice.goc (right):
> > >
> > > https://codereview.appspot.com/116940043/diff/60001/src/
> > > pkg/runtime/slice.goc#newcode125
> > > src/pkg/runtime/slice.goc:125: ret->len = x.len;
> > > On 2014/07/21 19:44:52, khr wrote:
> > >
> > >> I worry about setting len & cap before array is set.  This means GC
> > >>
> > > might see a
> > >
> > >> [nil, nonzero, nonzero] slice.  Probably won't break the current gc,
> > >>
> > > but it
> > >
> > >> would be safer for future changes to set all 3 fields atomically with
> > >>
> > > respect to
> > >
> > >> GC.
> > >>
> > >
> > >  Just moving them to after the mallocgc call should do it.
> > >>
> > >
> > > This problem is unsolvable with shuffling order. Consider that you have
> > > an already initialized slice and want to assign a new value to it,
> > > whatever way you assign new values you have an inconsistent slice during
> > > the operation. And with concurrent GC you can't assume that you can look
> > > at 3 words atomically at all (unless you protect all slice operations
> > > with a mutex, which is not a good idea).
> > > I think we need to leave it as is for now. If/when we change the GC,
> > > there will be lots of work to do, and we know that the compiler,
> > > string.goc and slice.goc are the places to check.
> > >
> > > https://codereview.appspot.com/116940043/

LGTM

[dvyukov, 2014-07-21 21:56:24]

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

runtime: pass correct size to malloc
In both cases we lie to malloc about the actual size that we need.
In panic we ask for less memory than we are going to use.
In slice we ask for more memory than we are going to use
(potentially asking for a fractional number of elements).
This breaks the new GC.

LGTM=khr
R=golang-codereviews, dave, khr
CC=golang-codereviews, rsc
https://codereview.appspot.com/116940043

[rsc, 2014-07-22 03:34:33]

FWIW I have a change pending that will make slice update order irrelevant. It is
very easy.