From 256a605faa10bc5507597c4669cc5bc400bf487a Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Fri, 20 Jan 2017 09:54:10 -0800 Subject: [PATCH] cmd/compile: don't use nilcheck information until the next block When nilcheck runs, the values in a block are not in any particular order. So any facts derived from examining the blocks shouldn't be used until we reach the next block. This is suboptimal as it won't eliminate nil checks within a block. But it's probably a better fix for now as it is a much smaller change than other strategies for fixing this bug. nilptr3.go changes are mostly because for this pattern: _ = *p _ = *p either nil check is fine to keep, and this CL changes which one the compiler tends to keep. There are a few regressions from code like this: _ = *p f() _ = *p For this pattern, after this CL we issue 2 nil checks instead of one. (For the curious, this happens because intra-block nil check elimination now falls to CSE, not nilcheck proper. The former pattern has two nil checks with the same store argument. The latter pattern has two nil checks with different store arguments.) Fixes #18725 Change-Id: I3721b494c8bc9ba1142dc5c4361ea55c66920ac8 Reviewed-on: https://go-review.googlesource.com/35485 Reviewed-by: Cherry Zhang --- src/cmd/compile/internal/ssa/nilcheck.go | 22 +++++++++++---- test/fixedbugs/issue18725.go | 24 ++++++++++++++++ test/nilptr3.go | 36 ++++++++++++------------ 3 files changed, 59 insertions(+), 23 deletions(-) create mode 100644 test/fixedbugs/issue18725.go diff --git a/src/cmd/compile/internal/ssa/nilcheck.go b/src/cmd/compile/internal/ssa/nilcheck.go index 9f58db664b1..0a34cd1ae64 100644 --- a/src/cmd/compile/internal/ssa/nilcheck.go +++ b/src/cmd/compile/internal/ssa/nilcheck.go @@ -82,7 +82,7 @@ func nilcheckelim(f *Func) { } } - // Next, process values in the block. + // Next, eliminate any redundant nil checks in this block. i := 0 for _, v := range b.Values { b.Values[i] = v @@ -105,13 +105,10 @@ func nilcheckelim(f *Func) { f.Config.Warnl(v.Line, "removed nil check") } v.reset(OpUnknown) + // TODO: f.freeValue(v) i-- continue } - // Record the fact that we know ptr is non nil, and remember to - // undo that information when this dominator subtree is done. - nonNilValues[ptr.ID] = true - work = append(work, bp{op: ClearPtr, ptr: ptr}) } } for j := i; j < len(b.Values); j++ { @@ -119,6 +116,21 @@ func nilcheckelim(f *Func) { } b.Values = b.Values[:i] + // Finally, find redundant nil checks for subsequent blocks. + // Note that we can't add these until the loop above is done, as the + // values in the block are not ordered in any way when this pass runs. + // This was the cause of issue #18725. + for _, v := range b.Values { + if v.Op != OpNilCheck { + continue + } + ptr := v.Args[0] + // Record the fact that we know ptr is non nil, and remember to + // undo that information when this dominator subtree is done. + nonNilValues[ptr.ID] = true + work = append(work, bp{op: ClearPtr, ptr: ptr}) + } + // Add all dominated blocks to the work list. for w := sdom[node.block.ID].child; w != nil; w = sdom[w.ID].sibling { work = append(work, bp{op: Work, block: w}) diff --git a/test/fixedbugs/issue18725.go b/test/fixedbugs/issue18725.go new file mode 100644 index 00000000000..c632dbad639 --- /dev/null +++ b/test/fixedbugs/issue18725.go @@ -0,0 +1,24 @@ +// run + +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import "os" + +func panicWhenNot(cond bool) { + if cond { + os.Exit(0) + } else { + panic("nilcheck elim failed") + } +} + +func main() { + e := (*string)(nil) + panicWhenNot(e == e) + // Should never reach this line. + panicWhenNot(*e == *e) +} diff --git a/test/nilptr3.go b/test/nilptr3.go index 8fdae8c075f..c681cba50c4 100644 --- a/test/nilptr3.go +++ b/test/nilptr3.go @@ -40,23 +40,23 @@ var ( ) func f1() { - _ = *intp // ERROR "generated nil check" + _ = *intp // ERROR "removed nil check" // This one should be removed but the block copy needs // to be turned into its own pseudo-op in order to see // the indirect. - _ = *arrayp // ERROR "generated nil check" + _ = *arrayp // ERROR "removed nil check" // 0-byte indirect doesn't suffice. // we don't registerize globals, so there are no removed.* nil checks. - _ = *array0p // ERROR "generated nil check" _ = *array0p // ERROR "removed nil check" + _ = *array0p // ERROR "generated nil check" - _ = *intp // ERROR "removed nil check" + _ = *intp // ERROR "generated nil check" _ = *arrayp // ERROR "removed nil check" _ = *structp // ERROR "generated nil check" _ = *emptyp // ERROR "generated nil check" - _ = *arrayp // ERROR "removed nil check" + _ = *arrayp // ERROR "generated nil check" } func f2() { @@ -71,15 +71,15 @@ func f2() { empty1p *Empty1 ) - _ = *intp // ERROR "generated nil check" - _ = *arrayp // ERROR "generated nil check" - _ = *array0p // ERROR "generated nil check" - _ = *array0p // ERROR "removed.* nil check" _ = *intp // ERROR "removed.* nil check" _ = *arrayp // ERROR "removed.* nil check" + _ = *array0p // ERROR "removed.* nil check" + _ = *array0p // ERROR "generated nil check" + _ = *intp // ERROR "generated nil check" + _ = *arrayp // ERROR "removed.* nil check" _ = *structp // ERROR "generated nil check" _ = *emptyp // ERROR "generated nil check" - _ = *arrayp // ERROR "removed.* nil check" + _ = *arrayp // ERROR "generated nil check" _ = *bigarrayp // ERROR "generated nil check" ARM removed nil check before indirect!! _ = *bigstructp // ERROR "generated nil check" _ = *empty1p // ERROR "generated nil check" @@ -122,16 +122,16 @@ func f3(x *[10000]int) { // x wasn't going to change across the function call. // But it's a little complex to do and in practice doesn't // matter enough. - _ = x[9999] // ERROR "removed nil check" + _ = x[9999] // ERROR "generated nil check" // TODO: fix } func f3a() { x := fx10k() y := fx10k() z := fx10k() - _ = &x[9] // ERROR "generated nil check" - y = z _ = &x[9] // ERROR "removed.* nil check" + y = z + _ = &x[9] // ERROR "generated nil check" x = y _ = &x[9] // ERROR "generated nil check" } @@ -139,11 +139,11 @@ func f3a() { func f3b() { x := fx10k() y := fx10k() - _ = &x[9] // ERROR "generated nil check" + _ = &x[9] // ERROR "removed.* nil check" y = x _ = &x[9] // ERROR "removed.* nil check" x = y - _ = &x[9] // ERROR "removed.* nil check" + _ = &x[9] // ERROR "generated nil check" } func fx10() *[10]int @@ -179,15 +179,15 @@ func f4(x *[10]int) { _ = x[9] // ERROR "generated nil check" // bug would like to remove before indirect fx10() - _ = x[9] // ERROR "removed nil check" + _ = x[9] // ERROR "generated nil check" // TODO: fix x = fx10() y := fx10() - _ = &x[9] // ERROR "generated nil check" + _ = &x[9] // ERROR "removed[a-z ]* nil check" y = x _ = &x[9] // ERROR "removed[a-z ]* nil check" x = y - _ = &x[9] // ERROR "removed[a-z ]* nil check" + _ = &x[9] // ERROR "generated nil check" } func f5(p *float32, q *float64, r *float32, s *float64) float64 {