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

doc/go1.17: document closure inlining affects function PC comparison #45781

Closed
rski opened this issue Apr 26, 2021 · 10 comments
Closed

doc/go1.17: document closure inlining affects function PC comparison #45781

rski opened this issue Apr 26, 2021 · 10 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1
Milestone

Comments

@rski
Copy link

rski commented Apr 26, 2021

What version of Go are you using (go version)?

$ go version
go version devel go1.17-d5d24dbe41 Mon Apr 26 17:13:36 2021 +0000 linux/amd64

Does this issue reproduce with the latest release?

only master

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
~/go/src/playground/at-2021-04-26-204402 $ ~/Code/go/bin/go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/rski/.cache/go-build"
GOENV="/home/rski/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/rski/go/pkg/mod"
GONOPROXY=""
GONOSUMDB="="
GOOS="linux"
GOPATH="/home/rski/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/rski/Code/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/rski/Code/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.17-d5d24dbe41 Mon Apr 26 17:13:36 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/home/rski/go/src/playground/at-2021-04-26-204402/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2782832869=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/Yw1lgRph4Kd

What did you expect to see?

This should print main.WithFoo.func1, as with go 16.3

What did you see instead?

After the regabi branch merge, this prints

main.init.func1

This was temporarily fixed by 1129a60, which disabled inlining of functions that contain closures.

@rski rski changed the title dev.regabi merged changed reflect behaviour in the presense of inlined functions dev.regabi merge changed reflect behaviour in the presense of inlined functions Apr 26, 2021
@cherrymui
Copy link
Member

cc @danscales for inlining closures.

This is working as intended. We probably need to document it better.

@cherrymui cherrymui added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Apr 26, 2021
@cherrymui cherrymui added this to the Go1.17 milestone Apr 26, 2021
@rski
Copy link
Author

rski commented Apr 26, 2021

This is working as intended. interesting. The reason I figured out this happened is because it broke some code. It uses reflect like this to figure out if a test was passed a specific opt function, and the inlined version no longer compares equal with the non inlined version. Is this use pushing it and not covered by the compatibility guarantee?

@cherrymui
Copy link
Member

Comparing PCs of functions is never intended to work (e.g. func values are incomparable). It may happen to work in some cases, but not guaranteed to work.

@rski
Copy link
Author

rski commented Apr 26, 2021

heh, I'm surprised we got away with it for so many go versions then. Thanks for the quick response, I'll replace the code with something less brittle.

@cherrymui cherrymui changed the title dev.regabi merge changed reflect behaviour in the presense of inlined functions doc: document closure inlining affects function PC comparison Apr 26, 2021
@dmitshur
Copy link
Contributor

dmitshur commented May 21, 2021

@cherrymui Do you know where a good place to document this might be?

@dmitshur dmitshur added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label May 21, 2021
@cherrymui
Copy link
Member

I think it's better in the release notes. @danscales may already have a CL.

@dmitshur
Copy link
Contributor

@danscales, if you don't mind I'll make you an assignee on this issue, but please feel free to update it if you're not able to get to this.

@danscales
Copy link
Contributor

I already sent this text to Jeremy about a week ago:

Functions containing closures can now be inlined. One effect of this change is that a function returning a constant closure may actually return a different closure function for each place that it is inlined. Hence, this change could reveal bugs where Go functions are compared (incorrectly) by (unsafe) pointer value. Go functions are by definition not comparable.

Is there a place that I should put this in the release notes (I'm not sure how to do it), or @cherrymui do you want to just grab it (since I think you are collecting release notes).

@dmitshur dmitshur changed the title doc: document closure inlining affects function PC comparison doc/go1.17: document closure inlining affects function PC comparison May 21, 2021
@dmitshur
Copy link
Contributor

Thank you Dan.

We can probably place that as a paragraph in the Runtime section at https://tip.golang.org/doc/go1.17#runtime. The corresponding file is here:

go/doc/go1.17.html

Lines 214 to 222 in 217f5dd

<h2 id="runtime">Runtime</h2>
<p><!-- CL 304470 -->
TODO: <a href="https://golang.org/cl/304470">https://golang.org/cl/304470</a>: cmd/compile, runtime: add metadata for argument printing in traceback
</p>
<p>
TODO: complete the Runtime section
</p>

@gopherbot
Copy link

Change https://golang.org/cl/322089 mentions this issue: doc: add Go 1.17 release note about inlining functions with closures

@golang golang locked and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1
Projects
None yet
Development

No branches or pull requests

5 participants