[dev.regabi] cmd/compile: replace Node.HasCall with walk.mayCall

After CL 284220, we now only need to detect expressions that contain
function calls in the arguments list of further function calls. So we
can simplify Node.HasCall/fncall/etc a lot.

Instead of incrementally tracking whether an expression contains
function calls all throughout walk, simply check once at the point of
using an expression as a function call argument. Since any expression
checked here will itself become a function call argument, it won't be
checked again because we'll short circuit at the enclosing function
call.

Also, restructure the recursive walk code to use mayCall, and trim
down the list of acceptable expressions. It should be okay to be
stricter, since we'll now only see function call arguments and after
they've already been walked.

It's possible I was overly aggressive removing Ops here. But if so,
we'll get an ICE, and it'll be easy to re-add them. I think this is
better than the alternative of accidentally allowing expressions
through that risk silently clobbering the stack.

Passes toolstash -cmp.

Change-Id: I585ef35dcccd9f4018e4bf2c3f9ccb1514a826f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/284223
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>

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

@@ -32,8 +32,7 @@ type miniExpr struct {
 }
 
 const (
-	miniExprHasCall = 1 << iota
+	miniExprNonNil = 1 << iota
-	miniExprNonNil
 	miniExprTransient
 	miniExprBounded
 	miniExprImplicit // for use by implementations; not supported by every Expr
@@ -44,8 +43,6 @@ func (*miniExpr) isExpr() {}
 
 func (n *miniExpr) Type() *types.Type     { return n.typ }
 func (n *miniExpr) SetType(x *types.Type) { n.typ = x }
-func (n *miniExpr) HasCall() bool         { return n.flags&miniExprHasCall != 0 }
-func (n *miniExpr) SetHasCall(b bool)     { n.flags.set(miniExprHasCall, b) }
 func (n *miniExpr) NonNil() bool          { return n.flags&miniExprNonNil != 0 }
 func (n *miniExpr) MarkNonNil()           { n.flags |= miniExprNonNil }
 func (n *miniExpr) Transient() bool       { return n.flags&miniExprTransient != 0 }

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

@@ -57,8 +57,7 @@ const (
 	miniWalkdefShift   = 0 // TODO(mdempsky): Move to Name.flags.
 	miniTypecheckShift = 2
 	miniDiag           = 1 << 4
-	miniHasCall        = 1 << 5 // for miniStmt
+	miniWalked         = 1 << 5 // to prevent/catch re-walking
-	miniWalked         = 1 << 6 // to prevent/catch re-walking
 )
 
 func (n *miniNode) Typecheck() uint8 { return n.bits.get2(miniTypecheckShift) }
@@ -89,7 +88,5 @@ func (n *miniNode) Name() *Name             { return nil }
 func (n *miniNode) Sym() *types.Sym         { return nil }
 func (n *miniNode) Val() constant.Value     { panic(n.no("Val")) }
 func (n *miniNode) SetVal(v constant.Value) { panic(n.no("SetVal")) }
-func (n *miniNode) HasCall() bool           { return false }
-func (n *miniNode) SetHasCall(bool)         { panic(n.no("SetHasCall")) }
 func (n *miniNode) NonNil() bool            { return false }
 func (n *miniNode) MarkNonNil()             { panic(n.no("MarkNonNil")) }

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

@@ -52,8 +52,6 @@ type Node interface {
 	SetTypecheck(x uint8)
 	NonNil() bool
 	MarkNonNil()
-	HasCall() bool
-	SetHasCall(x bool)
 }
 
 // Line returns n's position as a string. If n has been inlined,
@@ -544,7 +542,6 @@ func InitExpr(init []Node, expr Node) Node {
 	}
 
 	n.PtrInit().Prepend(init...)
-	n.SetHasCall(true)
 	return n
 }
 

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

@@ -53,8 +53,6 @@ func (*miniStmt) isStmt() {}
 func (n *miniStmt) Init() Nodes     { return n.init }
 func (n *miniStmt) SetInit(x Nodes) { n.init = x }
 func (n *miniStmt) PtrInit() *Nodes { return &n.init }
-func (n *miniStmt) HasCall() bool     { return n.bits&miniHasCall != 0 }
-func (n *miniStmt) SetHasCall(b bool) { n.bits.set(miniHasCall, b) }
 
 // An AssignListStmt is an assignment statement with
 // more than one item on at least one side: Lhs = Rhs.

src/cmd/compile/internal/walk/assign.go

@@ -248,18 +248,6 @@ func walkReturn(n *ir.ReturnStmt) ir.Node {
 	return n
 }
 
-// fncall reports whether assigning an rvalue of type rt to an lvalue l might involve a function call.
-func fncall(l ir.Node, rt *types.Type) bool {
-	if l.HasCall() || l.Op() == ir.OINDEXMAP {
-		return true
-	}
-	if types.Identical(l.Type(), rt) {
-		return false
-	}
-	// There might be a conversion required, which might involve a runtime call.
-	return true
-}
-
 // check assign type list to
 // an expression list. called in
 //	expr-list = func()
@@ -275,9 +263,9 @@ func ascompatet(nl ir.Nodes, nr *types.Type) []ir.Node {
 		}
 		r := nr.Field(i)
 
-		// Any assignment to an lvalue that might cause a function call must be
+		// Order should have created autotemps of the appropriate type for
-		// deferred until all the returned values have been read.
+		// us to store results into.
-		if fncall(l, r.Type) {
+		if tmp, ok := l.(*ir.Name); !ok || !tmp.AutoTemp() || !types.Identical(tmp.Type(), r.Type) {
 			base.FatalfAt(l.Pos(), "assigning %v to %+v", r.Type, l)
 		}
 
@@ -286,14 +274,7 @@ func ascompatet(nl ir.Nodes, nr *types.Type) []ir.Node {
 		res.SetType(r.Type)
 		res.SetTypecheck(1)
 
-		a := convas(ir.NewAssignStmt(base.Pos, l, res), &nn)
+		nn.Append(ir.NewAssignStmt(base.Pos, l, res))
-		updateHasCall(a)
-		if a.HasCall() {
-			ir.Dump("ascompatet ucount", a)
-			base.Fatalf("ascompatet: too many function calls evaluating parameters")
-		}
-
-		nn.Append(a)
 	}
 	return nn
 }

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

@@ -67,8 +67,6 @@ func walkExpr(n ir.Node, init *ir.Nodes) ir.Node {
 		_ = staticdata.StringSym(n.Pos(), constant.StringVal(n.Val()))
 	}
 
-	updateHasCall(n)
-
 	if base.Flag.LowerW != 0 && n != nil {
 		ir.Dump("after walk expr", n)
 	}
@@ -527,15 +525,17 @@ func walkCall1(n *ir.CallExpr, init *ir.Nodes) {
 	// For any argument whose evaluation might require a function call,
 	// store that argument into a temporary variable,
 	// to prevent that calls from clobbering arguments already on the stack.
-	// When instrumenting, all arguments might require function calls.
 	var tempAssigns []ir.Node
 	for i, arg := range args {
-		updateHasCall(arg)
+		// Validate argument and parameter types match.
-		// Determine param type.
+		param := params.Field(i)
-		t := params.Field(i).Type
+		if !types.Identical(arg.Type(), param.Type) {
-		if base.Flag.Cfg.Instrumenting || fncall(arg, t) {
+			base.FatalfAt(n.Pos(), "assigning %L to parameter %v (type %v)", arg, param.Sym, param.Type)
-			// make assignment of fncall to Temp
+		}
-			tmp := typecheck.Temp(t)
+
+		if mayCall(arg) {
+			// assignment of arg to Temp
+			tmp := typecheck.Temp(param.Type)
 			a := convas(ir.NewAssignStmt(base.Pos, tmp, arg), init)
 			tempAssigns = append(tempAssigns, a)
 			// replace arg with temp

src/cmd/compile/internal/walk/walk.go

@@ -67,8 +67,6 @@ func convas(n *ir.AssignStmt, init *ir.Nodes) *ir.AssignStmt {
 	if n.Op() != ir.OAS {
 		base.Fatalf("convas: not OAS %v", n.Op())
 	}
-	defer updateHasCall(n)
-
 	n.SetTypecheck(1)
 
 	if n.X == nil || n.Y == nil {
@@ -274,40 +272,33 @@ func backingArrayPtrLen(n ir.Node) (ptr, length ir.Node) {
 	return ptr, length
 }
 
-// updateHasCall checks whether expression n contains any function
+// mayCall reports whether evaluating expression n may require
-// calls and sets the n.HasCall flag if so.
+// function calls, which could clobber function call arguments/results
-func updateHasCall(n ir.Node) {
+// currently on the stack.
-	if n == nil {
+func mayCall(n ir.Node) bool {
-		return
+	// When instrumenting, any expression might require function calls.
-	}
+	if base.Flag.Cfg.Instrumenting {
-	n.SetHasCall(calcHasCall(n))
+		return true
 	}
 
-func calcHasCall(n ir.Node) bool {
+	isSoftFloat := func(typ *types.Type) bool {
+		return types.IsFloat[typ.Kind()] || types.IsComplex[typ.Kind()]
+	}
+
+	return ir.Any(n, func(n ir.Node) bool {
+		// walk should have already moved any Init blocks off of
+		// expressions.
 		if len(n.Init()) != 0 {
-		// TODO(mdempsky): This seems overly conservative.
+			base.FatalfAt(n.Pos(), "mayCall %+v", n)
-		return true
 		}
 
 		switch n.Op() {
 		default:
-		base.Fatalf("calcHasCall %+v", n)
+			base.FatalfAt(n.Pos(), "mayCall %+v", n)
-		panic("unreachable")
 
-	case ir.OLITERAL, ir.ONIL, ir.ONAME, ir.OTYPE, ir.ONAMEOFFSET:
+		case ir.OCALLFUNC, ir.OCALLMETH, ir.OCALLINTER:
-		if n.HasCall() {
-			base.Fatalf("OLITERAL/ONAME/OTYPE should never have calls: %+v", n)
-		}
-		return false
-	case ir.OCALL, ir.OCALLFUNC, ir.OCALLMETH, ir.OCALLINTER:
 			return true
-	case ir.OANDAND, ir.OOROR:
+
-		// hard with instrumented code
-		n := n.(*ir.LogicalExpr)
-		if base.Flag.Cfg.Instrumenting {
-			return true
-		}
-		return n.X.HasCall() || n.Y.HasCall()
 		case ir.OINDEX, ir.OSLICE, ir.OSLICEARR, ir.OSLICE3, ir.OSLICE3ARR, ir.OSLICESTR,
 			ir.ODEREF, ir.ODOTPTR, ir.ODOTTYPE, ir.ODIV, ir.OMOD:
 			// These ops might panic, make sure they are done
@@ -316,81 +307,29 @@ func calcHasCall(n ir.Node) bool {
 
 		// When using soft-float, these ops might be rewritten to function calls
 		// so we ensure they are evaluated first.
-	case ir.OADD, ir.OSUB, ir.OMUL:
+		case ir.OADD, ir.OSUB, ir.OMUL, ir.ONEG:
-		n := n.(*ir.BinaryExpr)
+			return ssagen.Arch.SoftFloat && isSoftFloat(n.Type())
-		if ssagen.Arch.SoftFloat && (types.IsFloat[n.Type().Kind()] || types.IsComplex[n.Type().Kind()]) {
-			return true
-		}
-		return n.X.HasCall() || n.Y.HasCall()
-	case ir.ONEG:
-		n := n.(*ir.UnaryExpr)
-		if ssagen.Arch.SoftFloat && (types.IsFloat[n.Type().Kind()] || types.IsComplex[n.Type().Kind()]) {
-			return true
-		}
-		return n.X.HasCall()
 		case ir.OLT, ir.OEQ, ir.ONE, ir.OLE, ir.OGE, ir.OGT:
 			n := n.(*ir.BinaryExpr)
-		if ssagen.Arch.SoftFloat && (types.IsFloat[n.X.Type().Kind()] || types.IsComplex[n.X.Type().Kind()]) {
+			return ssagen.Arch.SoftFloat && isSoftFloat(n.X.Type())
-			return true
-		}
-		return n.X.HasCall() || n.Y.HasCall()
 		case ir.OCONV:
 			n := n.(*ir.ConvExpr)
-		if ssagen.Arch.SoftFloat && ((types.IsFloat[n.Type().Kind()] || types.IsComplex[n.Type().Kind()]) || (types.IsFloat[n.X.Type().Kind()] || types.IsComplex[n.X.Type().Kind()])) {
+			return ssagen.Arch.SoftFloat && (isSoftFloat(n.Type()) || isSoftFloat(n.X.Type()))
-			return true
+
+		case ir.OLITERAL, ir.ONIL, ir.ONAME, ir.ONAMEOFFSET, ir.OMETHEXPR,
+			ir.OAND, ir.OANDNOT, ir.OLSH, ir.OOR, ir.ORSH, ir.OXOR, ir.OCOMPLEX, ir.OEFACE,
+			ir.OANDAND, ir.OOROR,
+			ir.OADDR, ir.OBITNOT, ir.ONOT, ir.OPLUS,
+			ir.OCAP, ir.OIMAG, ir.OLEN, ir.OREAL,
+			ir.OCONVNOP, ir.ODOT,
+			ir.OCFUNC, ir.OIDATA, ir.OITAB, ir.OSPTR,
+			ir.OBYTES2STRTMP, ir.OGETG, ir.OSLICEHEADER:
+			// ok: operations that don't require function calls.
+			// Expand as needed.
 		}
-		return n.X.HasCall()
 
-	case ir.OAND, ir.OANDNOT, ir.OLSH, ir.OOR, ir.ORSH, ir.OXOR, ir.OCOPY, ir.OCOMPLEX, ir.OEFACE:
-		n := n.(*ir.BinaryExpr)
-		return n.X.HasCall() || n.Y.HasCall()
-
-	case ir.OAS:
-		n := n.(*ir.AssignStmt)
-		return n.X.HasCall() || n.Y != nil && n.Y.HasCall()
-
-	case ir.OADDR:
-		n := n.(*ir.AddrExpr)
-		return n.X.HasCall()
-	case ir.OPAREN:
-		n := n.(*ir.ParenExpr)
-		return n.X.HasCall()
-	case ir.OBITNOT, ir.ONOT, ir.OPLUS, ir.ORECV,
-		ir.OALIGNOF, ir.OCAP, ir.OCLOSE, ir.OIMAG, ir.OLEN, ir.ONEW,
-		ir.OOFFSETOF, ir.OPANIC, ir.OREAL, ir.OSIZEOF,
-		ir.OCHECKNIL, ir.OCFUNC, ir.OIDATA, ir.OITAB, ir.OSPTR, ir.OVARDEF, ir.OVARKILL, ir.OVARLIVE:
-		n := n.(*ir.UnaryExpr)
-		return n.X.HasCall()
-	case ir.ODOT, ir.ODOTMETH, ir.ODOTINTER:
-		n := n.(*ir.SelectorExpr)
-		return n.X.HasCall()
-
-	case ir.OGETG, ir.OMETHEXPR:
 		return false
-
+	})
-	// TODO(rsc): These look wrong in various ways but are what calcHasCall has always done.
-	case ir.OADDSTR:
-		// TODO(rsc): This used to check left and right, which are not part of OADDSTR.
-		return false
-	case ir.OBLOCK:
-		// TODO(rsc): Surely the block's statements matter.
-		return false
-	case ir.OCONVIFACE, ir.OCONVNOP, ir.OBYTES2STR, ir.OBYTES2STRTMP, ir.ORUNES2STR, ir.OSTR2BYTES, ir.OSTR2BYTESTMP, ir.OSTR2RUNES, ir.ORUNESTR:
-		// TODO(rsc): Some conversions are themselves calls, no?
-		n := n.(*ir.ConvExpr)
-		return n.X.HasCall()
-	case ir.ODOTTYPE2:
-		// TODO(rsc): Shouldn't this be up with ODOTTYPE above?
-		n := n.(*ir.TypeAssertExpr)
-		return n.X.HasCall()
-	case ir.OSLICEHEADER:
-		// TODO(rsc): What about len and cap?
-		n := n.(*ir.SliceHeaderExpr)
-		return n.Ptr.HasCall()
-	case ir.OAS2DOTTYPE, ir.OAS2FUNC:
-		// TODO(rsc): Surely we need to check List and Rlist.
-		return false
-	}
 }
 
 // itabType loads the _type field from a runtime.itab struct.