Merge remote-tracking branch 'upstream/master'

Author: actions-user

src/cmd/compile/internal/devirtualize/pgo.go

@@ -741,7 +741,7 @@ func findHotConcreteCallee(p *pgoir.Profile, caller *ir.Func, call *ir.CallExpr,
 		hottest = e
 	}
 
-	if hottest == nil {
+	if hottest == nil || hottest.Weight == 0 {
 		if base.Debug.PGODebug >= 2 {
 			fmt.Printf("%v: call %s:%d: no hot callee\n", ir.Line(call), callerName, callOffset)
 		}

src/cmd/compile/internal/inline/inl.go

@@ -786,7 +786,7 @@ func inlineCallCheck(callerfn *ir.Func, call *ir.CallExpr) (bool, bool) {
 	if call.Op() != ir.OCALLFUNC {
 		return false, false
 	}
-	if call.GoDefer || call.NoInline {
+	if call.GoDefer {
 		return false, false
 	}
 

src/cmd/compile/internal/ir/expr.go

@@ -191,7 +191,6 @@ type CallExpr struct {
 	KeepAlive []*Name // vars to be kept alive until call returns
 	IsDDD     bool
 	GoDefer   bool // whether this call is part of a go or defer statement
-	NoInline  bool // whether this call must not be inlined
 }
 
 func NewCallExpr(pos src.XPos, op Op, fun Node, args []Node) *CallExpr {

src/cmd/compile/internal/ir/scc.go

@@ -22,7 +22,7 @@ package ir
 // Second, each function becomes two virtual nodes in the graph,
 // with numbers n and n+1. We record the function's node number as n
 // but search from node n+1. If the search tells us that the component
-// number (min) is n+1, we know that this is a trivial component: one function
+// number (minVisitGen) is n+1, we know that this is a trivial component: one function
 // plus its closures. If the search tells us that the component number is
 // n, then there was a path from node n+1 back to node n, meaning that
 // the function set is mutually recursive. The escape analysis can be
@@ -70,13 +70,13 @@ func (v *bottomUpVisitor) visit(n *Func) uint32 {
 	id := v.visitgen
 	v.nodeID[n] = id
 	v.visitgen++
-	min := v.visitgen
+	minVisitGen := v.visitgen
 	v.stack = append(v.stack, n)
 
 	do := func(defn Node) {
 		if defn != nil {
-			if m := v.visit(defn.(*Func)); m < min {
-				min = m
+			if m := v.visit(defn.(*Func)); m < minVisitGen {
+				minVisitGen = m
 			}
 		}
 	}
@@ -97,13 +97,13 @@ func (v *bottomUpVisitor) visit(n *Func) uint32 {
 		}
 	})
 
-	if (min == id || min == id+1) && !n.IsClosure() {
+	if (minVisitGen == id || minVisitGen == id+1) && !n.IsClosure() {
 		// This node is the root of a strongly connected component.
 
-		// The original min was id+1. If the bottomUpVisitor found its way
+		// The original minVisitGen was id+1. If the bottomUpVisitor found its way
 		// back to id, then this block is a set of mutually recursive functions.
 		// Otherwise, it's just a lone function that does not recurse.
-		recursive := min == id
+		recursive := minVisitGen == id
 
 		// Remove connected component from stack and mark v.nodeID so that future
 		// visits return a large number, which will not affect the caller's min.
@@ -121,5 +121,5 @@ func (v *bottomUpVisitor) visit(n *Func) uint32 {
 		v.analyze(block, recursive)
 	}
 
-	return min
+	return minVisitGen
 }

src/cmd/compile/internal/test/pgo_devirtualize_test.go

@@ -23,7 +23,7 @@ const profFileName = "devirt.pprof"
 const preProfFileName = "devirt.pprof.node_map"
 
 // testPGODevirtualize tests that specific PGO devirtualize rewrites are performed.
-func testPGODevirtualize(t *testing.T, dir string, want []devirtualization, pgoProfileName string) {
+func testPGODevirtualize(t *testing.T, dir string, want, nowant []devirtualization, pgoProfileName string) {
 	testenv.MustHaveGoRun(t)
 	t.Parallel()
 
@@ -69,24 +69,32 @@ go 1.21
 	}
 
 	got := make(map[devirtualization]struct{})
+	gotNoHot := make(map[devirtualization]struct{})
 
 	devirtualizedLine := regexp.MustCompile(`(.*): PGO devirtualizing \w+ call .* to (.*)`)
+	noHotLine := regexp.MustCompile(`(.*): call .*: no hot callee`)
 
 	scanner := bufio.NewScanner(pr)
 	for scanner.Scan() {
 		line := scanner.Text()
 		t.Logf("child: %s", line)
 
 		m := devirtualizedLine.FindStringSubmatch(line)
-		if m == nil {
+		if m != nil {
+			d := devirtualization{
+				pos:    m[1],
+				callee: m[2],
+			}
+			got[d] = struct{}{}
 			continue
 		}
-
-		d := devirtualization{
-			pos:    m[1],
-			callee: m[2],
+		m = noHotLine.FindStringSubmatch(line)
+		if m != nil {
+			d := devirtualization{
+				pos: m[1],
+			}
+			gotNoHot[d] = struct{}{}
 		}
-		got[d] = struct{}{}
 	}
 	if err := cmd.Wait(); err != nil {
 		t.Fatalf("error running go test: %v", err)
@@ -104,6 +112,11 @@ go 1.21
 		}
 		t.Errorf("devirtualization %v missing; got %v", w, got)
 	}
+	for _, nw := range nowant {
+		if _, ok := gotNoHot[nw]; !ok {
+			t.Errorf("unwanted devirtualization %v; got %v", nw, got)
+		}
+	}
 
 	// Run test with PGO to ensure the assertions are still true.
 	cmd = testenv.CleanCmdEnv(testenv.Command(t, out))
@@ -174,8 +187,18 @@ func TestPGODevirtualize(t *testing.T) {
 		//	callee: "mult.MultClosure.func1",
 		//},
 	}
+	nowant := []devirtualization{
+		// ExerciseIfaceZeroWeight
+		{
+			pos: "./devirt.go:256:29",
+		},
+		// ExerciseIndirCallZeroWeight
+		{
+			pos: "./devirt.go:282:37",
+		},
+	}
 
-	testPGODevirtualize(t, dir, want, profFileName)
+	testPGODevirtualize(t, dir, want, nowant, profFileName)
 }
 
 // TestPGOPreprocessDevirtualize tests that specific functions are devirtualized when PGO
@@ -237,8 +260,18 @@ func TestPGOPreprocessDevirtualize(t *testing.T) {
 		//	callee: "mult.MultClosure.func1",
 		//},
 	}
+	nowant := []devirtualization{
+		// ExerciseIfaceZeroWeight
+		{
+			pos: "./devirt.go:256:29",
+		},
+		// ExerciseIndirCallZeroWeight
+		{
+			pos: "./devirt.go:282:37",
+		},
+	}
 
-	testPGODevirtualize(t, dir, want, preProfFileName)
+	testPGODevirtualize(t, dir, want, nowant, preProfFileName)
 }
 
 // Regression test for https://go.dev/issue/65615. If a target function changes
@@ -303,8 +336,18 @@ func TestLookupFuncGeneric(t *testing.T) {
 		//	callee: "mult.MultClosure.func1",
 		//},
 	}
+	nowant := []devirtualization{
+		// ExerciseIfaceZeroWeight
+		{
+			pos: "./devirt.go:256:29",
+		},
+		// ExerciseIndirCallZeroWeight
+		{
+			pos: "./devirt.go:282:37",
+		},
+	}
 
-	testPGODevirtualize(t, dir, want, profFileName)
+	testPGODevirtualize(t, dir, want, nowant, profFileName)
 }
 
 var multFnRe = regexp.MustCompile(`func MultFn\(a, b int64\) int64`)

src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go

@@ -250,3 +250,45 @@ func ExerciseFuncClosure(iter int, a1, a2 AddFunc, m1, m2 mult.MultFunc) int {
 	}
 	return val
 }
+
+//go:noinline
+func IfaceZeroWeight(a *Add, b Adder) bool {
+	return a.Add(1, 2) == b.Add(3, 4) // unwanted devirtualization
+}
+
+// ExerciseIfaceZeroWeight never calls IfaceZeroWeight, so the callee
+// is not expected to appear in the profile.
+//
+//go:noinline
+func ExerciseIfaceZeroWeight() {
+	if false {
+		a := &Add{}
+		b := &Sub{}
+		// Unreachable call
+		IfaceZeroWeight(a, b)
+	}
+}
+
+func DirectCall() bool {
+	return true
+}
+
+func IndirectCall() bool {
+	return false
+}
+
+//go:noinline
+func IndirCallZeroWeight(indirectCall func() bool) bool {
+	return DirectCall() && indirectCall() // unwanted devirtualization
+}
+
+// ExerciseIndirCallZeroWeight never calls IndirCallZeroWeight, so the
+// callee is not expected to appear in the profile.
+//
+//go:noinline
+func ExerciseIndirCallZeroWeight() {
+	if false {
+		// Unreachable call
+		IndirCallZeroWeight(IndirectCall)
+	}
+}

src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go

@@ -71,3 +71,19 @@ func TestDevirtFuncClosure(t *testing.T) {
 		t.Errorf("ExerciseFuncClosure(10) got %d want 1176", v)
 	}
 }
+
+func BenchmarkDevirtIfaceZeroWeight(t *testing.B) {
+	ExerciseIfaceZeroWeight()
+}
+
+func TestDevirtIfaceZeroWeight(t *testing.T) {
+	ExerciseIfaceZeroWeight()
+}
+
+func BenchmarkDevirtIndirCallZeroWeight(t *testing.B) {
+	ExerciseIndirCallZeroWeight()
+}
+
+func TestDevirtIndirCallZeroWeight(t *testing.T) {
+	ExerciseIndirCallZeroWeight()
+}

src/crypto/tls/handshake_client_tls13.go

@@ -8,8 +8,8 @@ import (
 	"bytes"
 	"context"
 	"crypto"
+	"crypto/hkdf"
 	"crypto/hmac"
-	"crypto/internal/fips140/hkdf"
 	"crypto/internal/fips140/mlkem"
 	"crypto/internal/fips140/tls13"
 	"crypto/rsa"
@@ -90,12 +90,13 @@ func (hs *clientHandshakeStateTLS13) handshake() error {
 		confTranscript.Write(hs.serverHello.original[:30])
 		confTranscript.Write(make([]byte, 8))
 		confTranscript.Write(hs.serverHello.original[38:])
-		acceptConfirmation := tls13.ExpandLabel(hs.suite.hash.New,
-			hkdf.Extract(hs.suite.hash.New, hs.echContext.innerHello.random, nil),
-			"ech accept confirmation",
-			confTranscript.Sum(nil),
-			8,
-		)
+		h := hs.suite.hash.New
+		prk, err := hkdf.Extract(h, hs.echContext.innerHello.random, nil)
+		if err != nil {
+			c.sendAlert(alertInternalError)
+			return err
+		}
+		acceptConfirmation := tls13.ExpandLabel(h, prk, "ech accept confirmation", confTranscript.Sum(nil), 8)
 		if subtle.ConstantTimeCompare(acceptConfirmation, hs.serverHello.random[len(hs.serverHello.random)-8:]) == 1 {
 			hs.hello = hs.echContext.innerHello
 			c.serverName = c.config.ServerName
@@ -264,12 +265,13 @@ func (hs *clientHandshakeStateTLS13) processHelloRetryRequest() error {
 			copy(hrrHello, hs.serverHello.original)
 			hrrHello = bytes.Replace(hrrHello, hs.serverHello.encryptedClientHello, make([]byte, 8), 1)
 			confTranscript.Write(hrrHello)
-			acceptConfirmation := tls13.ExpandLabel(hs.suite.hash.New,
-				hkdf.Extract(hs.suite.hash.New, hs.echContext.innerHello.random, nil),
-				"hrr ech accept confirmation",
-				confTranscript.Sum(nil),
-				8,
-			)
+			h := hs.suite.hash.New
+			prk, err := hkdf.Extract(h, hs.echContext.innerHello.random, nil)
+			if err != nil {
+				c.sendAlert(alertInternalError)
+				return err
+			}
+			acceptConfirmation := tls13.ExpandLabel(h, prk, "hrr ech accept confirmation", confTranscript.Sum(nil), 8)
 			if subtle.ConstantTimeCompare(acceptConfirmation, hs.serverHello.encryptedClientHello) == 1 {
 				hello = hs.echContext.innerHello
 				c.serverName = c.config.ServerName

src/crypto/tls/handshake_server_tls13.go

@@ -8,8 +8,8 @@ import (
 	"bytes"
 	"context"
 	"crypto"
+	"crypto/hkdf"
 	"crypto/hmac"
-	"crypto/internal/fips140/hkdf"
 	"crypto/internal/fips140/mlkem"
 	"crypto/internal/fips140/tls13"
 	"crypto/internal/hpke"
@@ -572,12 +572,13 @@ func (hs *serverHandshakeStateTLS13) doHelloRetryRequest(selectedGroup CurveID)
 		if err := transcriptMsg(helloRetryRequest, confTranscript); err != nil {
 			return nil, err
 		}
-		acceptConfirmation := tls13.ExpandLabel(hs.suite.hash.New,
-			hkdf.Extract(hs.suite.hash.New, hs.clientHello.random, nil),
-			"hrr ech accept confirmation",
-			confTranscript.Sum(nil),
-			8,
-		)
+		h := hs.suite.hash.New
+		prf, err := hkdf.Extract(h, hs.clientHello.random, nil)
+		if err != nil {
+			c.sendAlert(alertInternalError)
+			return nil, err
+		}
+		acceptConfirmation := tls13.ExpandLabel(h, prf, "hrr ech accept confirmation", confTranscript.Sum(nil), 8)
 		helloRetryRequest.encryptedClientHello = acceptConfirmation
 	}
 
@@ -735,12 +736,13 @@ func (hs *serverHandshakeStateTLS13) sendServerParameters() error {
 			return err
 		}
 		// compute the acceptance message
-		acceptConfirmation := tls13.ExpandLabel(hs.suite.hash.New,
-			hkdf.Extract(hs.suite.hash.New, hs.clientHello.random, nil),
-			"ech accept confirmation",
-			echTranscript.Sum(nil),
-			8,
-		)
+		h := hs.suite.hash.New
+		prk, err := hkdf.Extract(h, hs.clientHello.random, nil)
+		if err != nil {
+			c.sendAlert(alertInternalError)
+			return err
+		}
+		acceptConfirmation := tls13.ExpandLabel(h, prk, "ech accept confirmation", echTranscript.Sum(nil), 8)
 		copy(hs.hello.random[32-8:], acceptConfirmation)
 	}