From 9cbbf5e0f4fe5231a5ad425638800618839de0a5 Mon Sep 17 00:00:00 2001
From: Keith Randall <khr@golang.org>
Date: Fri, 10 Jan 2025 17:33:26 -0800
Subject: [PATCH] [release-branch.go1.23] cmd/compile: fix write barrier
 coalescing

We can't coalesce a non-WB store with a subsequent Move, as the
result of the store might be the source of the move.

There's a simple codegen test. Not sure how we might do a real test,
as all the repro's I've come up with are very expensive and unreliable.

Fixes #71230

Change-Id: If18bf181a266b9b90964e2591cd2e61a7168371c
Reviewed-on: https://go-review.googlesource.com/c/go/+/642197
Reviewed-by: Keith Randall <khr@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/642498
---
 src/cmd/compile/internal/ssa/writebarrier.go | 15 ++++++++++++
 test/codegen/writebarrier.go                 | 25 ++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/src/cmd/compile/internal/ssa/writebarrier.go b/src/cmd/compile/internal/ssa/writebarrier.go
index 1caccb7c18..71acefbf8a 100644
--- a/src/cmd/compile/internal/ssa/writebarrier.go
+++ b/src/cmd/compile/internal/ssa/writebarrier.go
@@ -252,6 +252,7 @@ func writebarrier(f *Func) {
 		var start, end int
 		var nonPtrStores int
 		values := b.Values
+		hasMove := false
 	FindSeq:
 		for i := len(values) - 1; i >= 0; i-- {
 			w := values[i]
@@ -263,6 +264,9 @@ func writebarrier(f *Func) {
 					end = i + 1
 				}
 				nonPtrStores = 0
+				if w.Op == OpMoveWB {
+					hasMove = true
+				}
 			case OpVarDef, OpVarLive:
 				continue
 			case OpStore:
@@ -273,6 +277,17 @@ func writebarrier(f *Func) {
 				if nonPtrStores > 2 {
 					break FindSeq
 				}
+				if hasMove {
+					// We need to ensure that this store happens
+					// before we issue a wbMove, as the wbMove might
+					// use the result of this store as its source.
+					// Even though this store is not write-barrier
+					// eligible, it might nevertheless be the store
+					// of a pointer to the stack, which is then the
+					// source of the move.
+					// See issue 71228.
+					break FindSeq
+				}
 			default:
 				if last == nil {
 					continue
diff --git a/test/codegen/writebarrier.go b/test/codegen/writebarrier.go
index cfcfe15a40..4e0da14433 100644
--- a/test/codegen/writebarrier.go
+++ b/test/codegen/writebarrier.go
@@ -53,3 +53,28 @@ func combine4slice(p *[4][]byte, a, b, c, d []byte) {
 	// arm64:-`.*runtime[.]gcWriteBarrier`
 	p[3] = d
 }
+
+type S struct {
+	a, b string
+	c    *int
+}
+
+var g1, g2 *int
+
+func issue71228(dst *S, ptr *int) {
+	// Make sure that the non-write-barrier write.
+	// "sp.c = ptr" happens before the large write
+	// barrier "*dst = *sp". We approximate testing
+	// that by ensuring that two global variable write
+	// barriers aren't combined.
+	_ = *dst
+	var s S
+	sp := &s
+	//amd64:`.*runtime[.]gcWriteBarrier1`
+	g1 = nil
+	sp.c = ptr // outside of any write barrier
+	//amd64:`.*runtime[.]gcWriteBarrier1`
+	g2 = nil
+	//amd64:`.*runtime[.]wbMove`
+	*dst = *sp
+}
-- 
2.48.1