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.

Sign up for GitHub

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

Jump to bottom

crypto/internal/boring: noescape and nocallback cgo directives result in suspected miscompilation #63739

Closed
stapelberg opened this issue Oct 25, 2023 · 6 comments
Closed

crypto/internal/boring: noescape and nocallback cgo directives result in suspected miscompilation #63739

stapelberg opened this issue Oct 25, 2023 · 6 comments
Assignees
@mdempsky
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.24

Comments

@stapelberg
Copy link
Contributor

stapelberg commented Oct 25, 2023 •
edited by bcmills
Loading 

#!watchflakes
default <- builder == "linux-arm64-boringcrypto" && date > "2023-10-22" && date < "2023-10-26"

We identified https://go.googlesource.com/go/+/e46e861 to be the culprit for breaking many Google-internal tests (internal bug reference: b/307728752).

I’m sending a revert for now, but this should be investigated in more detail.
@stapelberg stapelberg added release-blocker compiler/runtime Issues related to the Go compiler and/or runtime. labels Oct 25, 2023
@bcmills bcmills added this to the Go1.22 milestone Oct 25, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 25, 2023 •
edited
Loading

Marked as release-blocker for Go 1.22 because the noescape and nocallback cgo directives are new in Go 1.22. If they result in miscompilation or a buggy interaction with the runtime, that feature should be rolled back so that it isn't released in a broken state.

(attn @golang/compiler, @golang/runtime)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/537598 mentions this issue: Revert "crypto/internal/boring: use noescape and nocallback cgo directives"

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 25, 2023
gopherbot pushed a commit that referenced this issue Oct 25, 2023
@stapelberg @gopherbot
Revert "crypto/internal/boring: use noescape and nocallback cgo direc…
067f28a 
…tives"

This reverts CL 525035.

Reason for revert: breaks many Google-internal tests (#63739), suspected miscompilation

Change-Id: I8cbebca0a187d12e16c405b2373c754e4a397ef4
Reviewed-on: https://go-review.googlesource.com/c/go/+/537598
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
This was referenced Oct 25, 2023
Closed
Closed
Open
@gopherbot
Copy link
Contributor

Found new dashboard test flakes for:

#!watchflakes
default <- builder == "linux-arm64-boringcrypto" && date > "2023-10-22" && date < "2023-10-26"
2023-10-23 20:47 linux-arm64-boringcrypto go@f9c54f9c net/http.TestMaxBytesHandler (log) 
--- FAIL: TestMaxBytesHandler (0.08s)
    --- FAIL: TestMaxBytesHandler/max_size_1000000_request_size_1000000 (0.02s)
        --- FAIL: TestMaxBytesHandler/max_size_1000000_request_size_1000000/h2 (0.01s)
            serve_test.go:6713: set RST avoidance delay to 1ms
panic: boringcrypto: EVP_AEAD_CTX_seal failed [recovered]
	panic: boringcrypto: EVP_AEAD_CTX_seal failed

goroutine 1485 [running]:
panic({0x51bd20?, 0x67a530?})
	/tmp/workdir/go/src/runtime/panic.go:772 +0x140 fp=0x40005ff0e0 sp=0x40005ff030 pc=0x1306a0
...
net/http_test.run[...].func1()
	/tmp/workdir/go/src/net/http/clientserver_test.go:90 +0x104 fp=0x40005fff60 sp=0x40005fff10 pc=0x4c0b34
testing.tRunner(0x40007d6820, 0x400034c030)
	/tmp/workdir/go/src/testing/testing.go:1593 +0xe8 fp=0x40005fffb0 sp=0x40005fff60 pc=0x1fb198
testing.(*T).Run.gowrap1()
	/tmp/workdir/go/src/testing/testing.go:1646 +0x2c fp=0x40005fffd0 sp=0x40005fffb0 pc=0x1fbf7c
runtime.goexit({})
	/tmp/workdir/go/src/runtime/asm_arm64.s:1197 +0x4 fp=0x40005fffd0 sp=0x40005fffd0 pc=0x1676e4
created by testing.(*T).Run in goroutine 1145
	/tmp/workdir/go/src/testing/testing.go:1646 +0x318

— watchflakes

@rsc rsc modified the milestones: Go1.22, Go1.23 Nov 2, 2023
@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

With the feature being rolled back, this is now a blocker for Go 1.23.

@rsc rsc mentioned this issue Nov 2, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539235 mentions this issue: cmd/cgo: disable #cgo noescape/nocallback until Go 1.23

doujiang24 added a commit to doujiang24/go that referenced this issue Apr 18, 2024
@doujiang24
Revert "cmd/cgo: disable #cgo noescape/nocallback until Go 1.23"
cdc4e9c 
This reverts commit 607e020.

Reason for revert: Go1.22 is released.

It's aggressive to introdcue #cgo noescape/nocallback in Go1.22, as in golang#63739
And it won't be a problem again while upgrading from Go1.22 to Go1.23.

fix golang#56378

Signed-off-by: doujiang24 <doujiang24@gmail.com>
@doujiang24 doujiang24 mentioned this issue Apr 18, 2024
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/579955 mentions this issue: Revert "cmd/cgo: disable #cgo noescape/nocallback until Go 1.23"

doujiang24 added a commit to doujiang24/go that referenced this issue Jul 5, 2024
@doujiang24
Revert "cmd/cgo: disable #cgo noescape/nocallback until Go 1.23"
3f400c4 
This reverts commit 607e020.

Reason for revert: Go1.22 is released.

It's aggressive to introdcue #cgo noescape/nocallback in Go1.22, as in golang#63739
And it won't be a problem again while upgrading from Go1.22 to Go1.23.

fix golang#56378

Signed-off-by: doujiang24 <doujiang24@gmail.com>
@gopherbot gopherbot modified the milestones: Go1.23, Go1.24 Aug 13, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mdempsky mdempsky

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Compiler / Runtime
Status: Done
Test Flakes
Status: Done
Milestone
Go1.24
Development

Successfully merging a pull request may close this issue.

cmd/cgo: enable #cgo noescape/nocallback doujiang24/go
7 participants
@mdempsky @stapelberg @rsc @mknyszek @dmitshur @bcmills @gopherbot