runtime: replace stkframe.arglen/argmap with methods

Currently, stkframe.arglen and stkframe.argmap are populated by
gentraceback under a particular set of circumstances. But because they
can be constructed from other fields in stkframe, they don't need to
be computed eagerly at all. They're also rather misleading, as they're
only part of computing the actual argument map and most callers should
be using getStackMap, which does the rest of the work.

This CL drops these fields from stkframe. It shifts the functions that
used to compute them, getArgInfoFast and getArgInfo, into
corresponding methods stkframe.argBytes and stkframe.argMapInternal.
argBytes is expected to be used by callers that need to know only the
argument frame size, while argMapInternal is used only by argBytes and
getStackMap.

We also move some of the logic from getStackMap into argMapInternal
because the previous split of responsibilities didn't make much sense.
This lets us return just a bitvector from argMapInternal, rather than
both a bitvector, which carries a size, and an "actually use this
size".

The getArgInfoFast function was inlined before (and inl_test checked
this). We drop that requirement from stkframe.argBytes because the
uses of this have shifted and now it's only called from heap dumping
(which never happens) and conservative stack frame scanning (which
very, very rarely happens).

There will be a few follow-up clean-up CLs.

For #54466. This is a nice clean-up on its own, but it also serves to
remove pointers from the traceback state that would eventually become
troublesome write barriers once we stack-rip gentraceback.

Change-Id: I107f98ed8e7b00185c081de425bbf24af02a4163
Reviewed-on: https://go-review.googlesource.com/c/go/+/424514
Run-TryBot: Austin Clements <austin@google.com>
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

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

@@ -47,7 +47,6 @@ func TestIntendedInlining(t *testing.T) {
 			"fastrand",
 			"float64bits",
 			"funcspdelta",
-			"getArgInfoFast",
 			"getm",
 			"getMCache",
 			"isDirectIface",

src/runtime/heapdump.go

@@ -327,7 +327,7 @@ func dumpframe(s *stkframe, arg unsafe.Pointer) bool {
 
 	// Record arg info for parent.
 	child.argoff = s.argp - s.fp
-	child.arglen = s.arglen
+	child.arglen = s.argBytes()
 	child.sp = (*uint8)(unsafe.Pointer(s.sp))
 	child.depth++
 	stkmap = (*stackmap)(funcdata(f, _FUNCDATA_ArgsPointerMaps))

src/runtime/mgcmark.go

@@ -945,10 +945,10 @@ func scanframeworker(frame *stkframe, state *stackScanState, gcw *gcWork) {
 		}
 
 		// Scan arguments to this frame.
-		if frame.arglen != 0 {
+		if n := frame.argBytes(); n != 0 {
 			// TODO: We could pass the entry argument map
 			// to narrow this down further.
-			scanConservative(frame.argp, frame.arglen, nil, gcw, state)
+			scanConservative(frame.argp, n, nil, gcw, state)
 		}
 
 		if isAsyncPreempt || isDebugCall {

src/runtime/runtime2.go

@@ -1027,13 +1027,11 @@ type stkframe struct {
 	// This is the PC to use to look up GC liveness for this frame.
 	continpc uintptr
 
-	lr     uintptr    // program counter at caller aka link register
-	sp     uintptr    // stack pointer at pc
-	fp     uintptr    // stack pointer at caller aka frame pointer
-	varp   uintptr    // top of local variables
-	argp   uintptr    // pointer to function arguments
-	arglen uintptr    // number of bytes at argp
-	argmap *bitvector // force use of this argmap
+	lr   uintptr // program counter at caller aka link register
+	sp   uintptr // stack pointer at pc
+	fp   uintptr // stack pointer at caller aka frame pointer
+	varp uintptr // top of local variables
+	argp uintptr // pointer to function arguments
 }
 
 // ancestorInfo records details of where a goroutine was started.

src/runtime/stack.go

@@ -1305,40 +1305,35 @@ func getStackMap(frame *stkframe, cache *pcvalueCache, debug bool) (locals, args
 		}
 	}
 
-	// Arguments.
-	if frame.arglen > 0 {
-		if frame.argmap != nil {
-			// argmap is set when the function is reflect.makeFuncStub or reflect.methodValueCall.
-			// In this case, arglen specifies how much of the args section is actually live.
-			// (It could be either all the args + results, or just the args.)
-			args = *frame.argmap
-			n := int32(frame.arglen / goarch.PtrSize)
-			if n < args.n {
-				args.n = n // Don't use more of the arguments than arglen.
-			}
+	// Arguments. First fetch frame size and special-case argument maps.
+	var isReflect bool
+	args, isReflect = frame.argMapInternal()
+	if args.n > 0 && args.bytedata == nil {
+		// Non-empty argument frame, but not a special map.
+		// Fetch the argument map at pcdata.
+		stackmap := (*stackmap)(funcdata(f, _FUNCDATA_ArgsPointerMaps))
+		if stackmap == nil || stackmap.n <= 0 {
+			print("runtime: frame ", funcname(f), " untyped args ", hex(frame.argp), "+", hex(args.n*goarch.PtrSize), "\n")
+			throw("missing stackmap")
+		}
+		if pcdata < 0 || pcdata >= stackmap.n {
+			// don't know where we are
+			print("runtime: pcdata is ", pcdata, " and ", stackmap.n, " args stack map entries for ", funcname(f), " (targetpc=", hex(targetpc), ")\n")
+			throw("bad symbol table")
+		}
+		if stackmap.nbit == 0 {
+			args.n = 0
 		} else {
-			stackmap := (*stackmap)(funcdata(f, _FUNCDATA_ArgsPointerMaps))
-			if stackmap == nil || stackmap.n <= 0 {
-				print("runtime: frame ", funcname(f), " untyped args ", hex(frame.argp), "+", hex(frame.arglen), "\n")
-				throw("missing stackmap")
-			}
-			if pcdata < 0 || pcdata >= stackmap.n {
-				// don't know where we are
-				print("runtime: pcdata is ", pcdata, " and ", stackmap.n, " args stack map entries for ", funcname(f), " (targetpc=", hex(targetpc), ")\n")
-				throw("bad symbol table")
-			}
-			if stackmap.nbit > 0 {
-				args = stackmapdata(stackmap, pcdata)
-			}
+			args = stackmapdata(stackmap, pcdata)
 		}
 	}
 
 	// stack objects.
 	if (GOARCH == "amd64" || GOARCH == "arm64" || GOARCH == "ppc64" || GOARCH == "ppc64le" || GOARCH == "riscv64") &&
-		unsafe.Sizeof(abi.RegArgs{}) > 0 && frame.argmap != nil {
-		// argmap is set when the function is reflect.makeFuncStub or reflect.methodValueCall.
-		// We don't actually use argmap in this case, but we need to fake the stack object
-		// record for these frames which contain an internal/abi.RegArgs at a hard-coded offset.
+		unsafe.Sizeof(abi.RegArgs{}) > 0 && isReflect {
+		// For reflect.makeFuncStub and reflect.methodValueCall,
+		// we need to fake the stack object record.
+		// These frames contain an internal/abi.RegArgs at a hard-coded offset.
 		// This offset matches the assembly code on amd64 and arm64.
 		objs = methodValueCallFrameObjs[:]
 	} else {

src/runtime/traceback.go

@@ -285,20 +285,7 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
 			frame.varp -= goarch.PtrSize
 		}
 
-		// Derive size of arguments.
-		// Most functions have a fixed-size argument block,
-		// so we can use metadata about the function f.
-		// Not all, though: there are some variadic functions
-		// in package runtime and reflect, and for those we use call-specific
-		// metadata recorded by f's caller.
-		if callback != nil || printing {
-			frame.argp = frame.fp + sys.MinFrameSize
-			var ok bool
-			frame.arglen, frame.argmap, ok = getArgInfoFast(f, callback != nil)
-			if !ok {
-				frame.arglen, frame.argmap = getArgInfo(&frame, callback != nil)
-			}
-		}
+		frame.argp = frame.fp + sys.MinFrameSize
 
 		// Determine frame's 'continuation PC', where it can continue.
 		// Normally this is the return address on the stack, but if sigpanic
@@ -491,7 +478,6 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
 		frame.lr = 0
 		frame.sp = frame.fp
 		frame.fp = 0
-		frame.argmap = nil
 
 		// On link register architectures, sighandler saves the LR on stack
 		// before faking a call.
@@ -670,21 +656,33 @@ type reflectMethodValue struct {
 	argLen uintptr    // just args
 }
 
-// getArgInfoFast returns the argument frame information for a call to f.
-// It is short and inlineable. However, it does not handle all functions.
-// If ok reports false, you must call getArgInfo instead.
-// TODO(josharian): once we do mid-stack inlining,
-// call getArgInfo directly from getArgInfoFast and stop returning an ok bool.
-func getArgInfoFast(f funcInfo, needArgMap bool) (arglen uintptr, argmap *bitvector, ok bool) {
-	return uintptr(f.args), nil, !(needArgMap && f.args == _ArgsSizeUnknown)
+// argBytes returns the argument frame size for a call to frame.fn.
+func (frame *stkframe) argBytes() uintptr {
+	if frame.fn.args != _ArgsSizeUnknown {
+		return uintptr(frame.fn.args)
+	}
+	// This is an uncommon and complicated case. Fall back to fully
+	// fetching the argument map to compute its size.
+	argMap, _ := frame.argMapInternal()
+	return uintptr(argMap.n) * goarch.PtrSize
 }
 
-// getArgInfo returns the argument frame information for a call to f
-// with call frame frame.
-func getArgInfo(frame *stkframe, needArgMap bool) (arglen uintptr, argmap *bitvector) {
+// argMapInternal is used internally by stkframe to fetch special
+// argument maps.
+//
+// argMap.n is always populated with the size of the argument map.
+//
+// argMap.bytedata is only populated for dynamic argument maps (used
+// by reflect). If the caller requires the argument map, it should use
+// this if non-nil, and otherwise fetch the argument map using the
+// current PC.
+//
+// hasReflectStackObj indicates that this frame also has a reflect
+// function stack object, which the caller must synthesize.
+func (frame *stkframe) argMapInternal() (argMap bitvector, hasReflectStackObj bool) {
 	f := frame.fn
-	arglen = uintptr(f.args)
-	if needArgMap && f.args == _ArgsSizeUnknown {
+	argMap.n = f.args / goarch.PtrSize
+	if f.args == _ArgsSizeUnknown {
 		// Extract argument bitmaps for reflect stubs from the calls they made to reflect.
 		switch funcname(f) {
 		case "reflect.makeFuncStub", "reflect.methodValueCall":
@@ -715,8 +713,9 @@ func getArgInfo(frame *stkframe, needArgMap bool) (arglen uintptr, argmap *bitve
 					print("runtime: confused by ", funcname(f), ": no frame (sp=", hex(frame.sp), " fp=", hex(frame.fp), ") at entry+", hex(frame.pc-f.entry()), "\n")
 					throw("reflect mismatch")
 				}
-				return 0, nil
+				return bitvector{}, false // No locals, so also no stack objects
 			}
+			hasReflectStackObj = true
 			mv := *(**reflectMethodValue)(unsafe.Pointer(arg0))
 			// Figure out whether the return values are valid.
 			// Reflect will update this value after it copies
@@ -726,12 +725,15 @@ func getArgInfo(frame *stkframe, needArgMap bool) (arglen uintptr, argmap *bitve
 				print("runtime: confused by ", funcname(f), "\n")
 				throw("reflect mismatch")
 			}
-			bv := mv.stack
-			arglen = uintptr(bv.n * goarch.PtrSize)
+			argMap = *mv.stack
 			if !retValid {
-				arglen = uintptr(mv.argLen) &^ (goarch.PtrSize - 1)
+				// argMap.n includes the results, but
+				// those aren't valid, so drop them.
+				n := int32((uintptr(mv.argLen) &^ (goarch.PtrSize - 1)) / goarch.PtrSize)
+				if n < argMap.n {
+					argMap.n = n
+				}
 			}
-			argmap = bv
 		}
 	}
 	return