code review 5278048: gc: treat uintptr as potentially containing a pointer

gc: treat uintptr as potentially containing a pointer
Fixes issue 2376

[dvyukov, 2011-10-17 07:19:20]

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

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

[lvd, 2011-10-17 10:43:26]

On Mon, Oct 17, 2011 at 09:19, <dvyukov@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> gc: treat uintptr as potentially containing a pointer
>
> Please review this at
http://codereview.appspot.com/**5278048/<http://codereview.appspot.com/5278048/>
>
> Affected files:
>  M src/cmd/gc/reflect.c
>
>
this negatively affects the precision of escape analysis.


>
> Index: src/cmd/gc/reflect.c
> ==============================**==============================**=======
> --- a/src/cmd/gc/reflect.c
> +++ b/src/cmd/gc/reflect.c
> @@ -516,7 +516,6 @@
>        case TUINT32:
>        case TINT64:
>        case TUINT64:
> -       case TUINTPTR:
>        case TFLOAT32:
>        case TFLOAT64:
>        case TBOOL:
> @@ -534,6 +533,7 @@
>        case TPTR32:
>        case TPTR64:
>        case TUNSAFEPTR:
> +       case TUINTPTR:
>        case TINTER:
>        case TCHAN:
>        case TMAP:
>
>
>

[lvd, 2011-10-17 10:46:11]

On Mon, Oct 17, 2011 at 12:43, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Mon, Oct 17, 2011 at 09:19, <dvyukov@google.com> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> gc: treat uintptr as potentially containing a pointer
>>
>> Please review this at
http://codereview.appspot.com/**5278048/<http://codereview.appspot.com/5278048/>
>>
>> Affected files:
>>  M src/cmd/gc/reflect.c
>>
>>
> this negatively affects the precision of escape analysis.
>

correction, it just causes some unnecessary work and parameter tagging.
 still i think it's undesirable.


>
>
>>
>> Index: src/cmd/gc/reflect.c
>> ==============================**==============================**=======
>> --- a/src/cmd/gc/reflect.c
>> +++ b/src/cmd/gc/reflect.c
>> @@ -516,7 +516,6 @@
>>        case TUINT32:
>>        case TINT64:
>>        case TUINT64:
>> -       case TUINTPTR:
>>        case TFLOAT32:
>>        case TFLOAT64:
>>        case TBOOL:
>> @@ -534,6 +533,7 @@
>>        case TPTR32:
>>        case TPTR64:
>>        case TUNSAFEPTR:
>> +       case TUINTPTR:
>>        case TINTER:
>>        case TCHAN:
>>        case TMAP:
>>
>>
>>
>

[dvyukov, 2011-10-17 11:24:49]

On 2011/10/17 10:46:11, lvd wrote:
> correction, it just causes some unnecessary work and parameter tagging.
>  still i think it's undesirable.

Hummm... but uintptr effectively haspointers. Can't dirty tricks with uintptr
break esc analysis as well? If not, then I am curious what condition do you need
in esc analysis? If you simply consider any cast to unsafe.Pointer as escape,
then you don't need to consider unsafe.Pointer as haspointers as well.

[dvyukov, 2011-10-17 11:28:16]

On 2011/10/17 11:24:49, dvyukov wrote:
> On 2011/10/17 10:46:11, lvd wrote:
> > correction, it just causes some unnecessary work and parameter tagging.
> >  still i think it's undesirable.
> 
> Hummm... but uintptr effectively haspointers. Can't dirty tricks with uintptr
> break esc analysis as well? If not, then I am curious what condition do you
need
> in esc analysis? If you simply consider any cast to unsafe.Pointer as escape,
> then you don't need to consider unsafe.Pointer as haspointers as well.

I see, you ignore all conversions. Then I would expect that you either need or
don't need both unsafe.Pointer and uintptr.

[dvyukov, 2011-10-17 11:30:06]

On 2011/10/17 11:28:16, dvyukov wrote:
> On 2011/10/17 11:24:49, dvyukov wrote:
> > On 2011/10/17 10:46:11, lvd wrote:
> > > correction, it just causes some unnecessary work and parameter tagging.
> > >  still i think it's undesirable.
> > 
> > Hummm... but uintptr effectively haspointers. Can't dirty tricks with
uintptr
> > break esc analysis as well? If not, then I am curious what condition do you
> need
> > in esc analysis? If you simply consider any cast to unsafe.Pointer as
escape,
> > then you don't need to consider unsafe.Pointer as haspointers as well.
> 
> I see, you ignore all conversions. Then I would expect that you either need or
> don't need both unsafe.Pointer and uintptr.

Btw, why does it say

func foobar(p *int) *int {
	return p
}
func main() {
	i := 42
	foobar(&i)
	println(i)
}
tst.go:13: moved to heap: i
tst.go:6: leaking param: p
tst.go:15: &i escapes to heap
?

Why p escapes from foobar?

[lvd, 2011-10-17 11:34:28]

On Mon, Oct 17, 2011 at 13:30, <dvyukov@google.com> wrote:

> On 2011/10/17 11:28:16, dvyukov wrote:
>
>> On 2011/10/17 11:24:49, dvyukov wrote:
>> > On 2011/10/17 10:46:11, lvd wrote:
>> > > correction, it just causes some unnecessary work and parameter
>>
> tagging.
>
>> > >  still i think it's undesirable.
>> >
>> > Hummm... but uintptr effectively haspointers. Can't dirty tricks
>>
> with uintptr
>
>> > break esc analysis as well? If not, then I am curious what condition
>>
> do you
>
>> need
>> > in esc analysis? If you simply consider any cast to unsafe.Pointer
>>
> as escape,
>
>> > then you don't need to consider unsafe.Pointer as haspointers as
>>
> well.
>
>  I see, you ignore all conversions. Then I would expect that you either
>>
> need or
>
>> don't need both unsafe.Pointer and uintptr.
>>
>
> Btw, why does it say
>
> func foobar(p *int) *int {
>        return p
> }
> func main() {
>        i := 42
>        foobar(&i)
>        println(i)
> }
> tst.go:13: moved to heap: i
> tst.go:6: leaking param: p
> tst.go:15: &i escapes to heap
> ?
>
> Why p escapes from foobar?
>

because you use it as a return value.



>
>
http://codereview.appspot.com/**5278048/<http://codereview.appspot.com/5278048/>
>

[lvd, 2011-10-17 11:39:54]

to get back to the original issue, i don't think marking the type uintptr as
something that contains a pointer is the correct way of dealing with the
problem. somehow, keeping track of where pointers go has to happen when you
do the unsafe conversion from a real pointer to an integer value.  Go
promises 'no dangling pointers' as long as you have pointers, not as long as
you can unsafely concoct one.  you could store all bits of the uintptr in a
separate bool, discard the uintptr, have the memory gc'ed and reconstruct
that pointer from your 32 or 64 bools.  that doesn't mean we should now mark
bools as potentially having pointers either.

/L

On Mon, Oct 17, 2011 at 13:34, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Mon, Oct 17, 2011 at 13:30, <dvyukov@google.com> wrote:
>
>> On 2011/10/17 11:28:16, dvyukov wrote:
>>
>>> On 2011/10/17 11:24:49, dvyukov wrote:
>>> > On 2011/10/17 10:46:11, lvd wrote:
>>> > > correction, it just causes some unnecessary work and parameter
>>>
>> tagging.
>>
>>> > >  still i think it's undesirable.
>>> >
>>> > Hummm... but uintptr effectively haspointers. Can't dirty tricks
>>>
>> with uintptr
>>
>>> > break esc analysis as well? If not, then I am curious what condition
>>>
>> do you
>>
>>> need
>>> > in esc analysis? If you simply consider any cast to unsafe.Pointer
>>>
>> as escape,
>>
>>> > then you don't need to consider unsafe.Pointer as haspointers as
>>>
>> well.
>>
>>  I see, you ignore all conversions. Then I would expect that you either
>>>
>> need or
>>
>>> don't need both unsafe.Pointer and uintptr.
>>>
>>
>> Btw, why does it say
>>
>> func foobar(p *int) *int {
>>        return p
>> }
>> func main() {
>>        i := 42
>>        foobar(&i)
>>        println(i)
>> }
>> tst.go:13: moved to heap: i
>> tst.go:6: leaking param: p
>> tst.go:15: &i escapes to heap
>> ?
>>
>> Why p escapes from foobar?
>>
>
> because you use it as a return value.
>
>
>
>>
>>
http://codereview.appspot.com/**5278048/<http://codereview.appspot.com/5278048/>
>>
>
>

[rsc, 2011-10-17 19:00:15]

LGTM

While this may reduce the effectiveness of escape analysis
in hypothetical cases, it doesn't have any effect on our
actual test suite.

[rsc, 2011-10-17 19:14:10]

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

gc: treat uintptr as potentially containing a pointer
Fixes issue 2376

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

Committer: Russ Cox <rsc@golang.org>