From: Michael Pratt <mpratt@google.com>
Date: Thu, 16 Nov 2023 19:58:37 +0000 (-0500)
Subject: cmd/compile: don't devirtualize calls to runtime.memhash_varlen
X-Git-Tag: go1.22rc1~292
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=3fd5c357a3a763f70f3ed684caed878e732e6ebe;p=gostls13.git

cmd/compile: don't devirtualize calls to runtime.memhash_varlen

runtime.memhash_varlen is defined as a normal function, but it is
actually a closure. All references are generated by
cmd/compile/internal/reflectdata.genhash, which creates a closure
containing the size of the type, which memhash_varlen accesses with
runtime.getclosureptr.

Since this doesn't look like a normal closure, ir.Func.OClosure is not
set, thus PGO function value devirtualization is willing to devirtualize
it, generating a call that completely ignores the closure context. This
causes memhash_varlen to either crash or generate incorrect results.

Skip this function, which is the only caller of getclosureptr.
Unfortunately there isn't a good way to detect these ineligible
functions more generally.

Fixes #64209.

Change-Id: Ibf509406667c6d4e5d431f10e5b1d1f926ecd7dc
Reviewed-on: https://go-review.googlesource.com/c/go/+/543195
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
---

diff --git a/src/cmd/compile/internal/devirtualize/pgo.go b/src/cmd/compile/internal/devirtualize/pgo.go
index 0a34e7eb8d..7b6c8ba0c0 100644
--- a/src/cmd/compile/internal/devirtualize/pgo.go
+++ b/src/cmd/compile/internal/devirtualize/pgo.go
@@ -241,6 +241,15 @@ func maybeDevirtualizeFunctionCall(p *pgo.Profile, fn *ir.Func, call *ir.CallExp
 		}
 		return nil, nil, 0
 	}
+	// runtime.memhash_varlen does not look like a closure, but it uses
+	// runtime.getclosureptr to access data encoded by callers, which are
+	// are generated by cmd/compile/internal/reflectdata.genhash.
+	if callee.Sym().Pkg.Path == "runtime" && callee.Sym().Name == "memhash_varlen" {
+		if base.Debug.PGODebug >= 3 {
+			fmt.Printf("callee %s is a closure (runtime.memhash_varlen), skipping\n", ir.FuncName(callee))
+		}
+		return nil, nil, 0
+	}
 	// TODO(prattmic): We don't properly handle methods as callees in two
 	// different dimensions:
 	//
diff --git a/src/runtime/stubs.go b/src/runtime/stubs.go
index 9633d3d4a6..cf856e135f 100644
--- a/src/runtime/stubs.go
+++ b/src/runtime/stubs.go
@@ -374,6 +374,11 @@ func getcallersp() uintptr // implemented as an intrinsic on all platforms
 //
 // The compiler rewrites calls to this function into instructions that fetch the
 // pointer from a well-known register (DX on x86 architecture, etc.) directly.
+//
+// WARNING: PGO-based devirtualization cannot detect that caller of
+// getclosureptr require closure context, and thus must maintain a list of
+// these functions, which is in
+// cmd/compile/internal/devirtualize/pgo.maybeDevirtualizeFunctionCall.
 func getclosureptr() uintptr
 
 //go:noescape