Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(507)

Issue 53940043: code review 53940043: cmd/gc: do not follow uintptr passed as function argument (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by rsc
Modified:
11 years, 1 month ago
Reviewers:
iant
CC:
golang-codereviews, iant
Visibility:
Public.

Description

cmd/gc: do not follow uintptr passed as function argument The escape analysis works by tracing assignment paths from variables that start with pointer type, or addresses of variables (addresses are always pointers). It does allow non-pointers in the path, so that in this code it sees x's value escape into y: var x *[10]int y := (*int)(unsafe.Pointer(uintptr(unsafe.Pointer(x))+32)) It must allow uintptr in order to see through this kind of "pointer arithmetic". It also traces such values if they end up as uintptrs passed to functions. This used to be important because packages like encoding/gob passed around uintptrs holding real pointers. The introduction of precise collection of stacks has forced code to be more honest about which declared stack variables hold pointers and which do not. In particular, the garbage collector no longer sees pointers stored in uintptr variables. Because of this, packages like encoding/gob have been fixed. There is not much point in the escape analysis accepting uintptrs as holding pointers at call boundaries if the garbage collector does not. Excluding uintptr-valued arguments brings the escape analysis in line with the garbage collector and has the useful side effect of making arguments to syscall.Syscall not appear to escape. That is, this CL should yield the same benefits as CL 45930043 (rolled back in CL 53870043), but it does so by making uintptrs less special, not more.

Patch Set 1 #

Patch Set 2 : diff -r 9c870196cb6b https://code.google.com/p/go/ #

Patch Set 3 : diff -r 9c870196cb6b https://code.google.com/p/go/ #

Patch Set 4 : diff -r 04b6a587b21a https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -3 lines) Patch
M src/cmd/gc/esc.c View 1 2 chunks +8 lines, -3 lines 0 comments Download

Messages

Total messages: 3
rsc
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
11 years, 1 month ago (2014-01-17 23:43:58 UTC) #1
iant
LGTM
11 years, 1 month ago (2014-01-18 00:41:00 UTC) #2
rsc
11 years, 1 month ago (2014-01-21 18:31:37 UTC) #3
*** Submitted as https://code.google.com/p/go/source/detail?r=619cbfc6f3e2 ***

cmd/gc: do not follow uintptr passed as function argument

The escape analysis works by tracing assignment paths from
variables that start with pointer type, or addresses of variables
(addresses are always pointers).  It does allow non-pointers
in the path, so that in this code it sees x's value escape into y:

        var x *[10]int
        y := (*int)(unsafe.Pointer(uintptr(unsafe.Pointer(x))+32))

It must allow uintptr in order to see through this kind of
"pointer arithmetic".

It also traces such values if they end up as uintptrs passed to
functions. This used to be important because packages like
encoding/gob passed around uintptrs holding real pointers.

The introduction of precise collection of stacks has forced
code to be more honest about which declared stack variables
hold pointers and which do not. In particular, the garbage
collector no longer sees pointers stored in uintptr variables.
Because of this, packages like encoding/gob have been fixed.

There is not much point in the escape analysis accepting
uintptrs as holding pointers at call boundaries if the garbage
collector does not.

Excluding uintptr-valued arguments brings the escape
analysis in line with the garbage collector and has the
useful side effect of making arguments to syscall.Syscall
not appear to escape.

That is, this CL should yield the same benefits as
CL 45930043 (rolled back in CL 53870043), but it does
so by making uintptrs less special, not more.

R=golang-codereviews, iant
CC=golang-codereviews
https://codereview.appspot.com/53940043
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b