code review 5489128: gc: avoid false positives when using scalar struct fields.

gc: avoid false positives when using scalar struct fields.

The escape analysis code does not make a distinction between
scalar and pointers fields in structs. Non-pointer fields
that escape should not make the whole struct escape.

[remyoudompheng, 2011-12-31 11:15:40]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
remy@archlinux.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[remyoudompheng, 2011-12-31 11:17:55]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
remy@archlinux.org),

Please take another look.

[lvd, 2012-01-04 08:38:15]

thanks!  LGTM but wait for rsc to get back from holiday.

http://codereview.appspot.com/5489128/diff/2006/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (right):

http://codereview.appspot.com/5489128/diff/2006/src/cmd/gc/esc.c#newcode475
src/cmd/gc/esc.c:475: return;
please add //fallthrough so future maintainers understand it's intentional.

[remyoudompheng, 2012-01-04 10:09:01]

Hello golang-dev@googlegroups.com, lvd@google.com (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[rsc, 2012-01-10 04:31:16]

very nice, thanks.

http://codereview.appspot.com/5489128/diff/7001/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (right):

http://codereview.appspot.com/5489128/diff/7001/src/cmd/gc/esc.c#newcode473
src/cmd/gc/esc.c:473: if(src->type && !haspointers(src->type))
This is great, thanks for finding this.

Please move the // up and make the case break
instead of returning.  THere is some cleanup after
the switch that needs to run.

// A non-pointer escaping from a struct does not concern us.
if(src->type && !haspointers(src->type))
    break;

[lvd, 2012-01-10 07:17:14]

i think the return is correct. and the comment should s/from/to
On Jan 10, 2012 5:31 AM, <rsc@golang.org> wrote:

> very nice, thanks.
>
>
>
>
http://codereview.appspot.com/**5489128/diff/7001/src/cmd/gc/**esc.c<http://c...
> File src/cmd/gc/esc.c (right):
>
> http://codereview.appspot.com/**5489128/diff/7001/src/cmd/gc/**
>
esc.c#newcode473<http://codereview.appspot.com/5489128/diff/7001/src/cmd/gc/esc.c#newcode473>
> src/cmd/gc/esc.c:473: if(src->type && !haspointers(src->type))
> This is great, thanks for finding this.
>
> Please move the // up and make the case break
> instead of returning.  THere is some cleanup after
> the switch that needs to run.
>
> // A non-pointer escaping from a struct does not concern us.
> if(src->type && !haspointers(src->type))
>    break;
>
>
http://codereview.appspot.com/**5489128/<http://codereview.appspot.com/5489128/>
>

[rsc, 2012-01-10 07:51:26]

On Mon, Jan 9, 2012 at 23:17, Luuk van Dijk <lvd@google.com> wrote:
> i think the return is correct. and the comment should s/from/to

you think this case does not need to do pdepth-- and lineno = lno?

[lvd, 2012-01-10 08:53:44]

i shouldnt try to review code on my phone.
On Jan 10, 2012 8:51 AM, "Russ Cox" <rsc@golang.org> wrote:

> On Mon, Jan 9, 2012 at 23:17, Luuk van Dijk <lvd@google.com> wrote:
> > i think the return is correct. and the comment should s/from/to
>
> you think this case does not need to do pdepth-- and lineno = lno?
>

[lvd, 2012-01-10 13:30:57]

On Tue, Jan 10, 2012 at 09:53, Luuk van Dijk <lvd@google.com> wrote:
> i shouldnt try to review code on my phone.

i was looking at the surroundings of the wrong ODOT case.  LGTM anyway then.

for my peace of mind, would you mind adding

// under AlsoNoLeak
func (b Bar) AlsoLeak() *int { // ERROR "leaking param: b"
	return b.ii
}

func (b Bar) LeaksToo() *int { // ERROR "leaking param: b"
	v := 0	// ERROR "moved to heap: v"
	b.ii = &v // ERROR "&v escapes"
	return b.ii
}

func (b *Bar) LeaksABit() *int { // ERROR "b does not escape"
	v := 0	// ERROR "moved to heap: v"
	b.ii = &v // ERROR "&v escapes"
	return b.ii
}

thanks.

[remyoudompheng, 2012-01-10 20:16:57]

and added tests suggested by Luuk.

http://codereview.appspot.com/5489128/diff/7001/src/cmd/gc/esc.c
File src/cmd/gc/esc.c (right):

http://codereview.appspot.com/5489128/diff/7001/src/cmd/gc/esc.c#newcode473
src/cmd/gc/esc.c:473: if(src->type && !haspointers(src->type))
On 2012/01/10 04:31:17, rsc wrote:
> This is great, thanks for finding this.
> 
> Please move the // up and make the case break
> instead of returning.  THere is some cleanup after
> the switch that needs to run.
> 
> // A non-pointer escaping from a struct does not concern us.
> if(src->type && !haspointers(src->type))
>     break;

Done.

[rsc, 2012-01-10 20:54:47]

hg mail?

[remyoudompheng, 2012-01-10 21:10:19]

Hello lvd@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com,
remy@archlinux.org),

Please take another look.

[remyoudompheng, 2012-01-11 21:49:51]

On 2012/01/10 21:10:19, remyoudompheng wrote:
> Please take another look.

Does it look OK now?

Rémy.

[lvd, 2012-01-12 11:05:56]

On Wed, Jan 11, 2012 at 22:49,  <remyoudompheng@gmail.com> wrote:
> On 2012/01/10 21:10:19, remyoudompheng wrote:
>>
>> Please take another look.
>
>
> Does it look OK now?

yes! thanks a lot.  I'll submit.

>
> Rémy.
>
> http://codereview.appspot.com/5489128/

[lvd, 2012-01-12 11:08:45]

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

gc: avoid false positives when using scalar struct fields.

The escape analysis code does not make a distinction between
scalar and pointers fields in structs. Non-pointer fields
that escape should not make the whole struct escape.

R=lvd, rsc
CC=golang-dev, remy
http://codereview.appspot.com/5489128

Committer: Luuk van Dijk <lvd@golang.org>