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

Jump to bottom

runtime/internal/atomic: StorepNoWB needs test #40975

Closed
cuonglm opened this issue Aug 22, 2020 · 3 comments
Closed

runtime/internal/atomic: StorepNoWB needs test #40975

cuonglm opened this issue Aug 22, 2020 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.16

Comments

@cuonglm
Copy link
Member

cuonglm commented Aug 22, 2020 •
edited

From https://go-review.googlesource.com/c/go/+/249917, @mdempsky said:

That's pretty brittle though (e.g., if/when we move escape analysis to SSA, it won't work anymore). We'd probably want a test that runs "GOOS=js GOARCH=wasm go build -gcflags=-m runtime/internal/atomic" and looks for the "leaking param: val" line.

In general, StorepNoWB needs a test that its second argument must be force to escape.
@gopherbot
Copy link

Change https://golang.org/cl/249761 mentions this issue: test: add test for StorepNoWB param leaking on wasm/js

@gopherbot
Copy link

Change https://golang.org/cl/249962 mentions this issue: runtime: implement StorepNoWB for wasm in assembly

gopherbot pushed a commit that referenced this issue Aug 23, 2020
@randall77
runtime: implement StorepNoWB for wasm in assembly
c060260 
The second argument of StorepNoWB must be forced to escape.
The current Go code does not explicitly enforce that property.
By implementing in assembly, and not using go:noescape, we
force the issue.

Test is in CL 249761. Issue #40975.

This CL is needed for CL 249917, which changes how go:notinheap
works and breaks the previous StorepNoWB wasm code.

I checked for other possible errors like this. This is the only
go:notinheap that isn't in the runtime itself.

Change-Id: I43400a806662655727c4a3baa8902b63bdc9fa57
Reviewed-on: https://go-review.googlesource.com/c/go/+/249962
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@cuonglm cuonglm changed the title runtime/internal/atomic: wasm StorepNoWB needs test runtime/internal/atomic: StorepNoWB needs test Aug 24, 2020
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. labels Oct 9, 2020
@dmitshur dmitshur added this to the Go1.16 milestone Oct 9, 2020
@gopherbot
Copy link

Change https://golang.org/cl/260878 mentions this issue: runtime: implement StorepNoWB for wasm in assembly

gopherbot pushed a commit that referenced this issue Oct 9, 2020
@randall77 @dmitshur
[release-branch.go1.15] runtime: implement StorepNoWB for wasm in ass…
0b80775 
…embly

The second argument of StorepNoWB must be forced to escape.
The current Go code does not explicitly enforce that property.
By implementing in assembly, and not using go:noescape, we
force the issue.

Test is in CL 249761. Issue #40975.

This CL is needed for CL 249917, which changes how go:notinheap
works and breaks the previous StorepNoWB wasm code.

I checked for other possible errors like this. This is the only
go:notinheap that isn't in the runtime itself.

Included test from CL 249761.

Update #41432

Change-Id: I43400a806662655727c4a3baa8902b63bdc9fa57
Reviewed-on: https://go-review.googlesource.com/c/go/+/249962
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
(cherry picked from commit c060260)
Reviewed-on: https://go-review.googlesource.com/c/go/+/260878
Trust: Keith Randall <khr@golang.org>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
@randall77 @claucece
[release-branch.go1.15] runtime: implement StorepNoWB for wasm in ass…
d2b0a7d 
…embly

The second argument of StorepNoWB must be forced to escape.
The current Go code does not explicitly enforce that property.
By implementing in assembly, and not using go:noescape, we
force the issue.

Test is in CL 249761. Issue golang#40975.

This CL is needed for CL 249917, which changes how go:notinheap
works and breaks the previous StorepNoWB wasm code.

I checked for other possible errors like this. This is the only
go:notinheap that isn't in the runtime itself.

Included test from CL 249761.

Update golang#41432

Change-Id: I43400a806662655727c4a3baa8902b63bdc9fa57
Reviewed-on: https://go-review.googlesource.com/c/go/+/249962
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
(cherry picked from commit c060260)
Reviewed-on: https://go-review.googlesource.com/c/go/+/260878
Trust: Keith Randall <khr@golang.org>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@golang golang locked and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
3 participants
@dmitshur @cuonglm @gopherbot