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: -m output is missing escape information #32850

Open
FiloSottile opened this issue Jun 28, 2019 · 17 comments
Open

cmd/compile: -m output is missing escape information #32850

FiloSottile opened this issue Jun 28, 2019 · 17 comments

Comments

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Jun 28, 2019

In #32670 we are considering a new API for golang.org/x/crypto/curve25519. One of the changes I'd like to introduce is not to require the caller to pre-allocate the destination. OTOH, I'd like not to introduce a heap allocation in the process.

I was hoping to be able to use the inliner to inline the variable declaration in the caller, where it has a chance not to escape. I used -gcflags -m to verify if it would work.

package main

import (
	"crypto/subtle"
	"errors"

	"golang.org/x/crypto/curve25519"
)

func main() {
	scalar, point := make([]byte, 32), make([]byte, 32)
	res, err := X25519(scalar, point)
	if err != nil {
		panic(err)
	}
	println(res)
}

func X25519(scalar, point []byte) ([]byte, error) {
	var dst [32]byte
	return x25519(&dst, scalar, point)
}

func x25519(dst *[32]byte, scalar, point []byte) ([]byte, error) {
	var in, base, zero [32]byte
	copy(in[:], scalar)
	copy(base[:], point)
	curve25519.ScalarMult(dst, &in, &base)
	if subtle.ConstantTimeCompare(dst[:], zero[:]) == 1 {
		return nil, errors.New("bad input")
	}
	return dst[:], nil
}

In Go 1.12.6 this works as intended. Note main &dst does not escape.

# play
./inline.go:28:23: inlining call to curve25519.ScalarMult
./inline.go:30:25: inlining call to errors.New
./inline.go:19:6: can inline X25519
./inline.go:12:20: inlining call to X25519
/var/folders/df/mrk3bfz149n8zb5h5p1vp_1m00hbbm/T/go-build156101582/b001/_gomod_.go:6:6: can inline init.0
./inline.go:30:25: error(&errors.errorString literal) escapes to heap
./inline.go:30:25: &errors.errorString literal escapes to heap
./inline.go:24:13: leaking param: dst to result ~r3 level=0
./inline.go:24:28: x25519 scalar does not escape
./inline.go:24:36: x25519 point does not escape
./inline.go:26:9: x25519 in does not escape
./inline.go:27:11: x25519 base does not escape
./inline.go:28:29: x25519 &in does not escape
./inline.go:28:34: x25519 &base does not escape
./inline.go:29:44: x25519 zero does not escape
./inline.go:11:23: main make([]byte, 32) does not escape
./inline.go:11:41: main make([]byte, 32) does not escape
./inline.go:12:20: main &dst does not escape
./inline.go:21:16: &dst escapes to heap
./inline.go:20:6: moved to heap: dst
./inline.go:19:13: X25519 scalar does not escape
./inline.go:19:21: X25519 point does not escape

In Go +2f387ac1f3, however, a lot of information is missing from the output. Note that the &dst escapes to heap and main &dst does not escape lines are gone, along with others.

# play
./inline.go:28:23: inlining call to curve25519.ScalarMult
./inline.go:30:25: inlining call to errors.New
./inline.go:19:6: can inline X25519
./inline.go:12:20: inlining call to X25519
./inline.go:24:13: leaking param: dst to result ~r3 level=0
./inline.go:24:28: x25519 scalar does not escape
./inline.go:24:36: x25519 point does not escape
./inline.go:30:25: error(&errors.errorString literal) escapes to heap
./inline.go:30:25: &errors.errorString literal escapes to heap
./inline.go:11:23: main make([]byte, 32) does not escape
./inline.go:11:41: main make([]byte, 32) does not escape
./inline.go:19:13: X25519 scalar does not escape
./inline.go:19:21: X25519 point does not escape
./inline.go:20:6: moved to heap: dst

Looking at the SSA it seems the inlined value still doesn't escape, but it's impossible to tell from the -gcflags -m output now.

Marking as release-blocker to look into as a regression.

/cc @mdempsky

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 28, 2019

Loading

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jun 29, 2019

I think this is intentional. Some old messages are either redundant or misleading. As in the example, the presence of both

./inline.go:12:20: main &dst does not escape
./inline.go:21:16: &dst escapes to heap

are confusing -- does it escape or not? (What it actually meant is that &dst does not escape at line 12, but does escape at line 21.) Also, ./inline.go:20:6: moved to heap: dst tells that it escapes, so the escapes to heap message isn't really necessary. Also, it is sometimes hard for the new escape analysis to match the exact message of the old one, as the implementation are quite different. So I think we decided to remove some of the messages.

The information is still there: ./inline.go:20:6: moved to heap: dst tells that it escapes. And the in the case of not escaping, the absence of moved to heap would indicate that.

Maybe the "fix" is to mention that in the release note? @aclements

Loading

@cherrymui cherrymui changed the title cmd/go: -gcflags -m output is missing escape information cmd/compile: -m output is missing escape information Jun 29, 2019
@FiloSottile
Copy link
Contributor Author

@FiloSottile FiloSottile commented Jun 30, 2019

I don't think main &dst does not escape vs &dst escapes to heap are contradictory. AFAICT main &dst is the dst instance that got inlined in main, which is a completely different variable. Indeed, that's exactly what I was going for: dst escapes in X25519, but not when inlined in main. The old output was confirming that, the new output is unclear.

I also don't think moved to heap and escapes to heap are redundant, one line tells me the location of the variable, the other the location of the escape. Finally, for readability it's nice to be guaranteed that every variable will have an associated explicit escapes or does not escape line.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 30, 2019

AFAICT main &dst is the dst instance that got inlined in main, which is a completely different variable.

No, esc.go's messages are just inconsistent and some prefix with the function name and others don't. (For regress compatibility, escape.go is similarly inconsistent, but I plan to fix this next cycle after removing esc.go.)

I also don't think moved to heap and escapes to heap are redundant, one line tells me the location of the variable, the other the location of the escape.

The current convention is that:

  • Named variables that are heap allocated are diagnosed with "moved to heap", while stack allocated get no diagnostic.
  • Allocating expressions (eg, make, new, interface boxing, etc) get "escapes to heap" or "does not escape to heap".

We used to print "escapes to heap" or "does not escape to heap" for &x expressions because they were imprecisely modeled as allocating expressions. The new algorithm models them more precisely/simply.

I admit knowing where the address leaked was nice, and I spent quite a while trying to figure out how to preserve that in the new algorithm without success.

Finally, for readability it's nice to be guaranteed that every variable will have an associated explicit escapes or does not escape line.

It would be easy to add additional messages to print when named variables are stack allocated. I think that would become very noisy though.

Loading

@FiloSottile
Copy link
Contributor Author

@FiloSottile FiloSottile commented Jun 30, 2019

Ok, sounds like the current output is more nuanced than I am qualified to judge.

AFAICT main &dst is the dst instance that got inlined in main, which is a completely different variable.

No, esc.go's messages are just inconsistent and some prefix with the function name and others don't. (For regress compatibility, escape.go is similarly inconsistent, but I plan to fix this next cycle after removing esc.go.)

Still, am I correct in saying that dst does not escape when inlined in main, but does escape in the stand-alone X25519?

If yes, there should be something in the output about that, and main &dst does not escape + &dst escapes to heap was close enough to give me the idea.

If no, I must have misread the SSA and I need to redesign my API.

Loading

@av86743
Copy link

@av86743 av86743 commented Jun 30, 2019

Ok, sounds like the current output is more nuanced than I am qualified to judge.

AFAICT main &dst is the dst instance that got inlined in main, which is a completely different variable.

No, esc.go's messages are just inconsistent and some prefix with the function name and others don't. (For regress compatibility, escape.go is similarly inconsistent, but I plan to fix this next cycle after removing esc.go.)

Still, am I correct in saying that dst does not escape when inlined in main, but does escape in the stand-alone X25519?

If yes, there should be something in the output about that, and main &dst does not escape + &dst escapes to heap was close enough to give me the idea.

If no, I must have misread the SSA and I need to redesign my API.

Does any of the above still make this issue a release-blocker for 1.13?

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 1, 2019

Still, am I correct in saying that dst does not escape when inlined in main, but does escape in the stand-alone X25519?

Yes. Sorry, I meant to clarify just that bare "&dst" does not mean uninlined; it just means those diagnostics happen to not include function name.

If yes, there should be something in the output about that,

If the inlined copy of dst had been moved to heap (e.g., by using fmt.Println instead of println), then there would have been a line:

./x.go:13:20: moved to heap: dst

Your point that this isn't very intuitive though is well received. The current output isn't really designed with inlining in mind: -m output is mostly used for regress tests, and most regress tests disable inlining. It could certainly be improved.

That said, I don't think this is a release blocking issue. The change in behavior was intentional.

Loading

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jul 1, 2019

Still, am I correct in saying that dst does not escape when inlined in main, but does escape in the stand-alone X25519?

Yes, this is correct.

But I wanted to say that I'm a little concerned that an API design relies on inlining. The inliner works heuristically, which changes over time, and it makes no guarantee about what is inlined (although we try to avoid regression in performance). Other implementations of the compiler (e.g. gccgo) may have very different inlining model. So I'm not sure it is best to have the API tied to the implementation detail of a particular compiler, or even a particular version of it.

Loading

@renthraysk
Copy link

@renthraysk renthraysk commented Jul 7, 2019

Seems only slices get reported as not escaping... whereas arrays never get reported.

func AppendVarint32(b []byte, x uint32) []byte {
	if x < 0x80 {
		return append(b, uint8(x))
	}
	t := []byte{
		uint8(x)|0x80,
		uint8(x>>7)|0x80,
		uint8(x>>14)|0x80,
		uint8(x>>21)|0x80,
		uint8(x>>28),
	}
	i := (bits.Len32(x)+6)/7  	// x >= 0x80 so Len32() cannot return 0
	t[i-1] &= 0x7F
	return append(b, t[:i]...)
}

Here t is defined as a slice, "AppendVarint32 []byte literal does not escape"
But as t := [5]byte{...} or [...]byte{...} and get nothing. Would be nice to be consistent.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 7, 2019

@renthraysk That's because slice literals implicitly allocate a backing array that the compiler needs to decide whether to heap or stack allocate, whereas array literals are passed by value.

Loading

@FiloSottile
Copy link
Contributor Author

@FiloSottile FiloSottile commented Jul 7, 2019

But I wanted to say that I'm a little concerned that an API design relies on inlining. The inliner works heuristically, which changes over time, and it makes no guarantee about what is inlined (although we try to avoid regression in performance). Other implementations of the compiler (e.g. gccgo) may have very different inlining model. So I'm not sure it is best to have the API tied to the implementation detail of a particular compiler, or even a particular version of it.

The API works correctly regardless of the inliner. What changes are its performance properties, and in a sense every API's performance changes based on the behavior of the inliner. I don't feel like we should ignore the capabilities of the inliner in deciding on API performance tradeoffs.

(If you want to continue this discussion, I suggest moving to #32670.)

Loading

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Jul 8, 2019

I was going to file this as an issue as well. All this information is now missing from 1.13

./stream.go:83:26: &bytes.Buffer literal escapes to heap
./stream.go:83:26: 	from ~R0 (assign-pair) at ./stream.go:83:26
./stream.go:83:26: 	from input (assigned) at ./stream.go:83:8
./stream.go:83:26: 	from input (interface-converted) at ./stream.go:93:26
./stream.go:83:26: 	from io.r (assign-pair) at ./stream.go:93:26
./stream.go:83:26: 	from io.r (passed to call[argument escapes]) at ./stream.go:93:26

This information was essential for not having to guess why an escpae occurred. All we are left with in 1.13 is this

./stream.go:83:26: &bytes.Buffer literal escapes to heap

Then are are some disconnected lines of information about where the variable allocated. There is no single picture of it anymore. This makes reading the report much harder.

Loading

@andybons andybons removed this from the Go1.13 milestone Jul 8, 2019
@andybons andybons added this to the Go1.14 milestone Jul 8, 2019
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 8, 2019

@ardan-bkennedy FWIW, the lack of detailed diagnostics (e.g., -m=2) is #31489. This issue is about the -m=1 diagnostics in the presence of inlining.

Loading

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Jul 8, 2019

Ok, I will post a new issue then. Sorry.

Loading

@rsc rsc removed this from the Go1.14 milestone Oct 9, 2019
@rsc rsc added this to the Backlog milestone Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants