Skip to content

Commit

Permalink
cmd/compile: change mustHeapAlloc to return a reason why
Browse files Browse the repository at this point in the history
This change renames mustHeapAlloc to heapAllocReason, and changes it
to return the reason why the argument must escape, so we don't have to
re-deduce it in its callers just to print the escape reason. It also
embeds isSmallMakeSlice body in heapAllocReason, since the former was
only used by the latter, and deletes isSmallMakeSlice.

An outdated TODO to remove smallintconst, which the TODO claimed was
only used in one place, was also removed, since grepping shows we
currently call smallintconst in 11 different places.

Change-Id: I0bd11bf29b92c4126f5bb455877ff73217d5a155
Reviewed-on: https://go-review.googlesource.com/c/go/+/258678
Run-TryBot: Alberto Donizetti <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Trust: Alberto Donizetti <[email protected]>
Trust: Cuong Manh Le <[email protected]>
Reviewed-by: Cuong Manh Le <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
  • Loading branch information
ALTree committed Oct 3, 2020
1 parent f89d05e commit 095e0f4
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 37 deletions.
1 change: 0 additions & 1 deletion src/cmd/compile/internal/gc/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -1134,7 +1134,6 @@ func strlit(n *Node) string {
return n.Val().U.(string)
}

// TODO(gri) smallintconst is only used in one place - can we used indexconst?
func smallintconst(n *Node) bool {
if n.Op == OLITERAL && Isconst(n, CTINT) && n.Type != nil {
switch simtype[n.Type.Etype] {
Expand Down
31 changes: 21 additions & 10 deletions src/cmd/compile/internal/gc/esc.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,36 +169,47 @@ func mayAffectMemory(n *Node) bool {
}
}

func mustHeapAlloc(n *Node) bool {
// heapAllocReason returns the reason the given Node must be heap
// allocated, or the empty string if it doesn't.
func heapAllocReason(n *Node) string {
if n.Type == nil {
return false
return ""
}

// Parameters are always passed via the stack.
if n.Op == ONAME && (n.Class() == PPARAM || n.Class() == PPARAMOUT) {
return false
return ""
}

if n.Type.Width > maxStackVarSize {
return true
return "too large for stack"
}

if (n.Op == ONEW || n.Op == OPTRLIT) && n.Type.Elem().Width >= maxImplicitStackVarSize {
return true
return "too large for stack"
}

if n.Op == OCLOSURE && closureType(n).Size() >= maxImplicitStackVarSize {
return true
return "too large for stack"
}
if n.Op == OCALLPART && partialCallType(n).Size() >= maxImplicitStackVarSize {
return true
return "too large for stack"
}

if n.Op == OMAKESLICE && !isSmallMakeSlice(n) {
return true
if n.Op == OMAKESLICE {
r := n.Right
if r == nil {
r = n.Left
}
if !smallintconst(r) {
return "non-constant size"
}
if t := n.Type; t.Elem().Width != 0 && r.Int64() >= maxImplicitStackVarSize/t.Elem().Width {
return "too large for stack"
}
}

return false
return ""
}

// addrescapes tags node n as having had its address taken
Expand Down
6 changes: 1 addition & 5 deletions src/cmd/compile/internal/gc/escape.go
Original file line number Diff line number Diff line change
Expand Up @@ -1051,11 +1051,7 @@ func (e *Escape) newLoc(n *Node, transient bool) *EscLocation {
}
n.SetOpt(loc)

if mustHeapAlloc(n) {
why := "too large for stack"
if n.Op == OMAKESLICE && (!Isconst(n.Left, CTINT) || (n.Right != nil && !Isconst(n.Right, CTINT))) {
why = "non-constant size"
}
if why := heapAllocReason(n); why != "" {
e.flow(e.heapHole().addr(n, why), loc)
}
}
Expand Down
17 changes: 2 additions & 15 deletions src/cmd/compile/internal/gc/walk.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,19 +336,6 @@ func walkstmt(n *Node) *Node {
return n
}

func isSmallMakeSlice(n *Node) bool {
if n.Op != OMAKESLICE {
return false
}
r := n.Right
if r == nil {
r = n.Left
}
t := n.Type

return smallintconst(r) && (t.Elem().Width == 0 || r.Int64() < maxImplicitStackVarSize/t.Elem().Width)
}

// walk the whole tree of the body of an
// expression or simple statement.
// the types expressions are calculated.
Expand Down Expand Up @@ -1339,8 +1326,8 @@ opswitch:
yyerror("%v can't be allocated in Go; it is incomplete (or unallocatable)", t.Elem())
}
if n.Esc == EscNone {
if !isSmallMakeSlice(n) {
Fatalf("non-small OMAKESLICE with EscNone: %v", n)
if why := heapAllocReason(n); why != "" {
Fatalf("%v has EscNone, but %v", n, why)
}
// var arr [r]T
// n = arr[:l]
Expand Down
11 changes: 5 additions & 6 deletions test/fixedbugs/issue41635.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@
package p

func f() { // ERROR ""
b1 := make([]byte, 1<<17) // ERROR "too large for stack" ""
b2 := make([]byte, 100, 1<<17) // ERROR "too large for stack" ""

n, m := 100, 200
b1 = make([]byte, n) // ERROR "non-constant size" ""
b2 = make([]byte, 100, m) // ERROR "non-constant size" ""
_ = make([]byte, 1<<17) // ERROR "too large for stack" ""
_ = make([]byte, 100, 1<<17) // ERROR "too large for stack" ""
_ = make([]byte, n, 1<<17) // ERROR "too large for stack" ""

_, _ = b1, b2
_ = make([]byte, n) // ERROR "non-constant size" ""
_ = make([]byte, 100, m) // ERROR "non-constant size" ""
}

0 comments on commit 095e0f4

Please sign in to comment.