Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: run escape analysis on SSA form #31501

Open
mdempsky opened this issue Apr 16, 2019 · 4 comments
Open

cmd/compile: run escape analysis on SSA form #31501

mdempsky opened this issue Apr 16, 2019 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mdempsky
Copy link
Contributor

Placeholder issue for discussion about running escape analysis on SSA form instead of the Node AST. I don't expect us to get to this for quite a while, but I think it's useful to have a single issue for cross-referencing related issues.

@mdempsky mdempsky added this to the Unplanned milestone Apr 16, 2019
@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 28, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@Jorropo
Copy link
Member

Jorropo commented Aug 11, 2023

I was working on having json.Unmarshal not leak the any argument and I have this minimized example:

package A

var escape func (*string)

func foo(a, b *string) *string {
	escape(a)
	return b
}

func Foo(a, b *string) {
	a = foo(a, b)
}

Here it incorrectly conclude that b leaks, because a leaks, and b is being assigned to a, so b must also leak right ? 😭
Any non buggy SSA analyzer would correctly understand that b does not leak here.

(if anyone is curious this is in reflect.Value.Set)


# command-line-arguments
./a.go:5:6: can inline foo with cost 62 as: func(*string, *string) *string { escape(a); return b }
./a.go:10:6: can inline Foo with cost 68 as: func(*string, *string) { a = foo(a, b) }
./a.go:11:9: inlining call to foo
./a.go:5:10: parameter a leaks to {heap} with derefs=0:
./a.go:5:10:   flow: {heap} = a:
./a.go:5:10:     from escape(a) (call parameter) at ./a.go:6:8
./a.go:5:13: parameter b leaks to ~r0 with derefs=0:
./a.go:5:13:   flow: ~r0 = b:
./a.go:5:13:     from return b (return) at ./a.go:7:2
./a.go:5:10: leaking param: a
./a.go:5:13: leaking param: b to result ~r0 level=0
./a.go:10:10: parameter a leaks to {heap} with derefs=0:
./a.go:10:10:   flow: a = a:
./a.go:10:10:     from a, b := a, b (assign-pair) at ./a.go:11:9
./a.go:10:10:   flow: {heap} = a:
./a.go:10:10:     from escape(a) (call parameter) at ./a.go:11:9
./a.go:10:13: parameter b leaks to {heap} with derefs=0:
./a.go:10:13:   flow: b = b:
./a.go:10:13:     from a, b := a, b (assign-pair) at ./a.go:11:9
./a.go:10:13:   flow: ~R0 = b:
./a.go:10:13:     from ~R0 = b (assign-pair) at ./a.go:11:9
./a.go:10:13:   flow: a = ~R0:
./a.go:10:13:     from a = ~R0 (assign) at ./a.go:11:4
./a.go:10:13:   flow: a = a:
./a.go:10:13:     from a, b := a, b (assign-pair) at ./a.go:11:9
./a.go:10:13:   flow: {heap} = a:
./a.go:10:13:     from escape(a) (call parameter) at ./a.go:11:9
./a.go:10:10: leaking param: a
./a.go:10:13: leaking param: b

@randall77
Copy link
Contributor

Our escape analysis is currently flow-insensitive, so in the statement

a = foo(a, b)

It doesn't understand that the assignment happens after the call. It thinks the assignment might happen before the call, in which base b might flow into the first argument of foo.
Not sure if doing escape analysis on SSA form would help this. Maybe.

@Jorropo
Copy link
Member

Jorropo commented Aug 11, 2023

I'm pretty sure SSA would work since it would reach the Call value, lookup the arguments, figure out that .Args[0] leaks, mark the Arg {a} value as leaking and that all, stop here.
a after foo would be SelectN [0] (Call {Foo} (Arg {a}) (Arg {b})), as completely different values it wouldn't be affected.

There maybe are simpler ways to achieve this, rewriting into SSA would be a big task but it would also solve other control flow related issues.

@Jorropo
Copy link
Member

Jorropo commented Aug 11, 2023

For my problem I just did that which looks very stupid on the diff but hey it works (*shift the goal post to an other problem):

 func (v Value) Set(x Value) {
 	v.mustBeAssignable()
 	x.mustBeExported() // do not let unexported x leak
 	var target unsafe.Pointer
 	if v.kind() == Interface {
 		target = v.ptr
 	}
-	x = x.assignTo("reflect.Set", v.typ(), target)
-	if x.flag&flagIndir != 0 {
-		if x.ptr == unsafe.Pointer(&zeroVal[0]) {
+	z := x.assignTo("reflect.Set", v.typ(), target)
+	if z.flag&flagIndir != 0 {
+		if z.ptr == unsafe.Pointer(&zeroVal[0]) {
 			typedmemclr(v.typ(), v.ptr)
 		} else {
-			typedmemmove(v.typ(), v.ptr, x.ptr)
+			typedmemmove(v.typ(), v.ptr, z.ptr)
 		}
 	} else {
-		*(*unsafe.Pointer)(v.ptr) = x.ptr
+		*(*unsafe.Pointer)(v.ptr) = z.ptr
 	}
 }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants