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

cmd/compile: write barrier code is sometimes preemptible when compiled with -N [1.21 backport] #61958

Closed
randall77 opened this issue Aug 11, 2023 · 4 comments
Closed

cmd/compile: write barrier code is sometimes preemptible when compiled with -N [1.21 backport] #61958

randall77 opened this issue Aug 11, 2023 · 4 comments
Assignees
@randall77
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. release-blocker
Milestone
Go1.21.1

Comments

@randall77
Copy link
Contributor 

package main

var g *int

func h() {
	g = nil
}

When compiled with -N (optimizations off), the empty block on the write-barrier-disabled branch is marked as preemptible when it shouldn't be.

	0x0018 00024 (/Users/khr/gowork/tmp7.go:6)	MOVWU	runtime.writeBarrier(SB), R0
	0x0020 00032 (/Users/khr/gowork/tmp7.go:6)	CBZW	R0, 40
	0x0024 00036 (/Users/khr/gowork/tmp7.go:6)	JMP	44
	0x0028 00040 (/Users/khr/gowork/tmp7.go:6)	PCDATA	$0, $-1
	0x0028 00040 (/Users/khr/gowork/tmp7.go:6)	JMP	64                     <= this one
	0x002c 00044 (/Users/khr/gowork/tmp7.go:6)	PCDATA	$0, $-2
	0x002c 00044 (/Users/khr/gowork/tmp7.go:6)	MOVD	main.g(SB), R0
	0x0034 00052 (/Users/khr/gowork/tmp7.go:6)	CALL	runtime.gcWriteBarrier1(SB)
	0x0038 00056 (/Users/khr/gowork/tmp7.go:6)	MOVD	R0, (R25)
	0x003c 00060 (/Users/khr/gowork/tmp7.go:6)	JMP	64
	0x0040 00064 (/Users/khr/gowork/tmp7.go:6)	MOVD	ZR, main.g(SB)

(PCDATA of -1 is preemptible, -2 is unpreemptible).

This could lead to very rare but nasty heap corruption when compiling with optimizations off (e.g. when running with Delve).
@randall77 randall77 added the CherryPickCandidate Used during the release process for point releases label Aug 11, 2023
@randall77 randall77 added this to the Go1.21.1 milestone Aug 11, 2023
@randall77 randall77 self-assigned this Aug 11, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 11, 2023
@gopherbot
Copy link

Change https://go.dev/cl/518755 mentions this issue: [release-branch.go1.21] cmd/compile: ensure empty blocks in write barriers are marked unpreemptible

@dmitshur
Copy link
Contributor

@randall77 Is there a corresponding upstream issue (i.e., for Go1.22 milestone) for this backport request? Does this apply to Go 1.20 too, or only Go 1.21?

Per https://go.dev/wiki/MinorReleases:

As soon as an interested party thinks an issue should be considered for backport, they open one or two “child” issues titled like package: title [1.17 backport]. The issue should include a link to the original issue and a short rationale about why the backport might be needed.

@dmitshur dmitshur changed the title cmd/compile: write barrier code is sometimes preemptible when compiled with -N cmd/compile: write barrier code is sometimes preemptible when compiled with -N [1.21 backport] Aug 16, 2023
@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Aug 16, 2023
@randall77
Copy link
Contributor Author

This applies only to 1.21, 1.20 does not have the bug in question.
There's no upstream (1.22) issue, but there are CLs (518055 and 518539). They aren't easily backportable as is because the code has changed in the meantime.
CL 518755 is a simpler, safer backport for just the version of the bug that manifests in 1.21.

gopherbot pushed a commit that referenced this issue Aug 17, 2023
@randall77 @gopherbot
[release-branch.go1.21] cmd/compile: ensure empty blocks in write bar…
25ec110 
…riers are marked unpreemptible

Fixes #61958

Change-Id: I242ab77ad2f1ea1dad2d14ef756fa92f9378429f
Reviewed-on: https://go-review.googlesource.com/c/go/+/518755
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

Closed by merging 25ec110 to release-branch.go1.21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@randall77 randall77

Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. release-blocker
Projects
None yet
Milestone
Go1.21.1
Development

No branches or pull requests
4 participants
@dmitshur @randall77 @gopherbot @heschi