From 9fe9853ae5641eda4cfa58015bd0bcedb99c12cb Mon Sep 17 00:00:00 2001 From: =?utf8?q?Cl=C3=A9ment=20Chigot?= Date: Tue, 16 Oct 2018 15:59:43 +0200 Subject: [PATCH] cmd/compile: fix nilcheck for AIX This commit adapts compile tool to create correct nilchecks for AIX. AIX allows to load a nil pointer. Therefore, the default nilcheck which issues a load must be replaced by a CMP instruction followed by a store at 0x0 if the value is nil. The store will trigger a SIGSEGV as on others OS. The nilcheck algorithm must be adapted to do not remove nilcheck if it's only a read. Stores are detected with v.Type.IsMemory(). Tests related to nilptr must be adapted to the previous changements. nilptr.go cannot be used as it's because the AIX address space starts at 1<<32. Change-Id: I9f5aaf0b7e185d736a9b119c0ed2fe4e5bd1e7af Reviewed-on: https://go-review.googlesource.com/c/144538 Run-TryBot: Tobias Klauser TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall --- src/cmd/compile/internal/gc/ssa.go | 3 +- src/cmd/compile/internal/ppc64/ssa.go | 46 +++- src/cmd/compile/internal/ssa/nilcheck.go | 12 +- test/nilptr.go | 3 + test/nilptr3.go | 21 +- test/nilptr3_wasm.go | 270 ----------------------- test/nilptr5.go | 33 +++ test/nilptr5_aix.go | 32 +++ test/nilptr5_wasm.go | 32 +++ test/nilptr_aix.go | 185 ++++++++++++++++ 10 files changed, 337 insertions(+), 300 deletions(-) delete mode 100644 test/nilptr3_wasm.go create mode 100644 test/nilptr5.go create mode 100644 test/nilptr5_aix.go create mode 100644 test/nilptr5_wasm.go create mode 100644 test/nilptr_aix.go diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go index 27af607d6f..e0b4b40323 100644 --- a/src/cmd/compile/internal/gc/ssa.go +++ b/src/cmd/compile/internal/gc/ssa.go @@ -3651,7 +3651,8 @@ func (s *state) call(n *Node, k callKind) *ssa.Value { break } closure = s.expr(fn) - if thearch.LinkArch.Family == sys.Wasm { + if thearch.LinkArch.Family == sys.Wasm || objabi.GOOS == "aix" && k != callGo { + // On AIX, the closure needs to be verified as fn can be nil, except if it's a call go. This needs to be handled by the runtime to have the "go of nil func value" error. // TODO(neelance): On other architectures this should be eliminated by the optimization steps s.nilCheck(closure) } diff --git a/src/cmd/compile/internal/ppc64/ssa.go b/src/cmd/compile/internal/ppc64/ssa.go index a6dd8cab5f..3b37c797a9 100644 --- a/src/cmd/compile/internal/ppc64/ssa.go +++ b/src/cmd/compile/internal/ppc64/ssa.go @@ -10,6 +10,7 @@ import ( "cmd/compile/internal/types" "cmd/internal/obj" "cmd/internal/obj/ppc64" + "cmd/internal/objabi" "math" "strings" ) @@ -1183,13 +1184,44 @@ func ssaGenValue(s *gc.SSAGenState, v *ssa.Value) { p.To.Sym = v.Aux.(*obj.LSym) case ssa.OpPPC64LoweredNilCheck: - // Issue a load which will fault if arg is nil. - p := s.Prog(ppc64.AMOVBZ) - p.From.Type = obj.TYPE_MEM - p.From.Reg = v.Args[0].Reg() - gc.AddAux(&p.From, v) - p.To.Type = obj.TYPE_REG - p.To.Reg = ppc64.REGTMP + if objabi.GOOS == "aix" { + // CMP Rarg0, R0 + // BNE 2(PC) + // STW R0, 0(R0) + // NOP (so the BNE has somewhere to land) + + // CMP Rarg0, R0 + p := s.Prog(ppc64.ACMP) + p.From.Type = obj.TYPE_REG + p.From.Reg = v.Args[0].Reg() + p.To.Type = obj.TYPE_REG + p.To.Reg = ppc64.REG_R0 + + // BNE 2(PC) + p2 := s.Prog(ppc64.ABNE) + p2.To.Type = obj.TYPE_BRANCH + + // STW R0, 0(R0) + // Write at 0 is forbidden and will trigger a SIGSEGV + p = s.Prog(ppc64.AMOVW) + p.From.Type = obj.TYPE_REG + p.From.Reg = ppc64.REG_R0 + p.To.Type = obj.TYPE_MEM + p.To.Reg = ppc64.REG_R0 + + // NOP (so the BNE has somewhere to land) + nop := s.Prog(obj.ANOP) + gc.Patch(p2, nop) + + } else { + // Issue a load which will fault if arg is nil. + p := s.Prog(ppc64.AMOVBZ) + p.From.Type = obj.TYPE_MEM + p.From.Reg = v.Args[0].Reg() + gc.AddAux(&p.From, v) + p.To.Type = obj.TYPE_REG + p.To.Reg = ppc64.REGTMP + } if gc.Debug_checknil != 0 && v.Pos.Line() > 1 { // v.Pos.Line()==1 in generated wrappers gc.Warnl(v.Pos, "generated nil check") } diff --git a/src/cmd/compile/internal/ssa/nilcheck.go b/src/cmd/compile/internal/ssa/nilcheck.go index fca4f0bfc4..e0669cf80c 100644 --- a/src/cmd/compile/internal/ssa/nilcheck.go +++ b/src/cmd/compile/internal/ssa/nilcheck.go @@ -5,6 +5,7 @@ package ssa import ( + "cmd/internal/objabi" "cmd/internal/src" ) @@ -183,6 +184,9 @@ func nilcheckelim(f *Func) { // This should agree with minLegalPointer in the runtime. const minZeroPage = 4096 +// faultOnLoad is true if a load to an address below minZeroPage will trigger a SIGSEGV. +var faultOnLoad = objabi.GOOS != "aix" + // nilcheckelim2 eliminates unnecessary nil checks. // Runs after lowering and scheduling. func nilcheckelim2(f *Func) { @@ -225,12 +229,16 @@ func nilcheckelim2(f *Func) { // Find any pointers that this op is guaranteed to fault on if nil. var ptrstore [2]*Value ptrs := ptrstore[:0] - if opcodeTable[v.Op].faultOnNilArg0 { + if opcodeTable[v.Op].faultOnNilArg0 && (faultOnLoad || v.Type.IsMemory()) { + // On AIX, only writing will fault. ptrs = append(ptrs, v.Args[0]) } - if opcodeTable[v.Op].faultOnNilArg1 { + if opcodeTable[v.Op].faultOnNilArg1 && (faultOnLoad || (v.Type.IsMemory() && v.Op != OpPPC64LoweredMove)) { + // On AIX, only writing will fault. + // LoweredMove is a special case because it's considered as a "mem" as it stores on arg0 but arg1 is accessed as a load and should be checked. ptrs = append(ptrs, v.Args[1]) } + for _, ptr := range ptrs { // Check to make sure the offset is small. switch opcodeTable[v.Op].auxType { diff --git a/test/nilptr.go b/test/nilptr.go index 8d674a7098..90f57c54b6 100644 --- a/test/nilptr.go +++ b/test/nilptr.go @@ -7,6 +7,9 @@ // Test that the implementation catches nil ptr indirection // in a large address space. +// +build !aix +// Address space starts at 1<<32 on AIX, so dummy is too far. + package main import "unsafe" diff --git a/test/nilptr3.go b/test/nilptr3.go index 6aa718e027..e0f2ed9767 100644 --- a/test/nilptr3.go +++ b/test/nilptr3.go @@ -1,6 +1,7 @@ // errorcheck -0 -d=nil // +build !wasm +// +build !aix // Copyright 2013 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style @@ -192,21 +193,6 @@ func f4(x *[10]int) { _ = &x[9] // ERROR "removed[a-z ]* nil check" } -func f5(p *float32, q *float64, r *float32, s *float64) float64 { - x := float64(*p) // ERROR "removed nil check" - y := *q // ERROR "removed nil check" - *r = 7 // ERROR "removed nil check" - *s = 9 // ERROR "removed nil check" - return x + y -} - -type T [29]byte - -func f6(p, q *T) { - x := *p // ERROR "removed nil check" - *q = x // ERROR "removed nil check" -} - func m1(m map[int][80]byte) byte { v := m[3] // ERROR "removed nil check" return v[5] @@ -257,11 +243,6 @@ func f7() (*Struct, float64) { return t, *p // ERROR "removed nil check" } -// make sure to remove nil check for memory move (issue #18003) -func f8(t *[8]int) [8]int { - return *t // ERROR "removed nil check" -} - func f9() []int { x := new([1]int) x[0] = 1 // ERROR "removed nil check" diff --git a/test/nilptr3_wasm.go b/test/nilptr3_wasm.go deleted file mode 100644 index df29cdc5dc..0000000000 --- a/test/nilptr3_wasm.go +++ /dev/null @@ -1,270 +0,0 @@ -// errorcheck -0 -d=nil - -// +build wasm - -// Copyright 2013 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. - -// Test that nil checks are removed. -// Optimization is enabled. - -package p - -type Struct struct { - X int - Y float64 -} - -type BigStruct struct { - X int - Y float64 - A [1 << 20]int - Z string -} - -type Empty struct { -} - -type Empty1 struct { - Empty -} - -var ( - intp *int - arrayp *[10]int - array0p *[0]int - bigarrayp *[1 << 26]int - structp *Struct - bigstructp *BigStruct - emptyp *Empty - empty1p *Empty1 -) - -func f1() { - _ = *intp // ERROR "generated 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" - - // 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" - - _ = *intp // ERROR "removed nil check" - _ = *arrayp // ERROR "removed nil check" - _ = *structp // ERROR "generated nil check" - _ = *emptyp // ERROR "generated nil check" - _ = *arrayp // ERROR "removed nil check" -} - -func f2() { - var ( - intp *int - arrayp *[10]int - array0p *[0]int - bigarrayp *[1 << 20]int - structp *Struct - bigstructp *BigStruct - emptyp *Empty - 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" - _ = *structp // ERROR "generated nil check" - _ = *emptyp // ERROR "generated nil check" - _ = *arrayp // ERROR "removed.* nil check" - _ = *bigarrayp // ERROR "generated nil check" ARM removed nil check before indirect!! - _ = *bigstructp // ERROR "generated nil check" - _ = *empty1p // ERROR "generated nil check" -} - -func fx10k() *[10000]int - -var b bool - -func f3(x *[10000]int) { - // Using a huge type and huge offsets so the compiler - // does not expect the memory hardware to fault. - _ = x[9999] // ERROR "generated nil check" - - for { - if x[9999] != 0 { // ERROR "removed nil check" - break - } - } - - x = fx10k() - _ = x[9999] // ERROR "generated nil check" - if b { - _ = x[9999] // ERROR "removed.* nil check" - } else { - _ = x[9999] // ERROR "removed.* nil check" - } - _ = x[9999] // ERROR "removed nil check" - - x = fx10k() - if b { - _ = x[9999] // ERROR "generated nil check" - } else { - _ = x[9999] // ERROR "generated nil check" - } - _ = x[9999] // ERROR "generated nil check" - - fx10k() - // This one is a bit redundant, if we figured out that - // 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" -} - -func f3a() { - x := fx10k() - y := fx10k() - z := fx10k() - _ = &x[9] // ERROR "generated nil check" - y = z - _ = &x[9] // ERROR "removed.* nil check" - x = y - _ = &x[9] // ERROR "generated nil check" -} - -func f3b() { - x := fx10k() - y := fx10k() - _ = &x[9] // ERROR "generated nil check" - y = x - _ = &x[9] // ERROR "removed.* nil check" - x = y - _ = &x[9] // ERROR "removed.* nil check" -} - -func fx10() *[10]int - -func f4(x *[10]int) { - // Most of these have no checks because a real memory reference follows, - // and the offset is small enough that if x is nil, the address will still be - // in the first unmapped page of memory. - - _ = x[9] // ERROR "generated nil check" // bug: would like to remove this check (but nilcheck and load are in different blocks) - - for { - if x[9] != 0 { // ERROR "removed nil check" - break - } - } - - x = fx10() - _ = x[9] // ERROR "generated nil check" // bug would like to remove before indirect - if b { - _ = x[9] // ERROR "removed nil check" - } else { - _ = x[9] // ERROR "removed nil check" - } - _ = x[9] // ERROR "removed nil check" - - x = fx10() - if b { - _ = x[9] // ERROR "generated nil check" // bug would like to remove before indirect - } else { - _ = &x[9] // ERROR "generated nil check" - } - _ = x[9] // ERROR "generated nil check" // bug would like to remove before indirect - - fx10() - _ = x[9] // ERROR "removed nil check" - - x = fx10() - y := fx10() - _ = &x[9] // ERROR "generated nil check" - y = x - _ = &x[9] // ERROR "removed[a-z ]* nil check" - x = y - _ = &x[9] // ERROR "removed[a-z ]* nil check" -} - -func f5(p *float32, q *float64, r *float32, s *float64) float64 { - x := float64(*p) // ERROR "generated nil check" - y := *q // ERROR "generated nil check" - *r = 7 // ERROR "generated nil check" - *s = 9 // ERROR "generated nil check" - return x + y -} - -type T [29]byte - -func f6(p, q *T) { - x := *p // ERROR "generated nil check" - *q = x // ERROR "generated nil check" -} - -func m1(m map[int][80]byte) byte { - v := m[3] // ERROR "removed nil check" - return v[5] -} -func m2(m map[int][800]byte) byte { - v := m[3] // ERROR "removed nil check" - return v[5] -} -func m3(m map[int][80]byte) (byte, bool) { - v, ok := m[3] // ERROR "removed nil check" - return v[5], ok -} -func m4(m map[int][800]byte) (byte, bool) { - v, ok := m[3] // ERROR "removed nil check" - return v[5], ok -} -func p1() byte { - p := new([100]byte) - return p[5] // ERROR "removed nil check" -} - -// make sure not to do nil check for access of PAUTOHEAP -//go:noinline -func (p *Struct) m() {} -func c1() { - var x Struct - func() { x.m() }() // ERROR "removed nil check" -} - -type SS struct { - x byte -} - -type TT struct { - SS -} - -func f(t *TT) *byte { - // See issue 17242. - s := &t.SS // ERROR "generated nil check" - return &s.x // ERROR "removed nil check" -} - -// make sure not to do nil check for newobject -func f7() (*Struct, float64) { - t := new(Struct) - p := &t.Y // ERROR "removed nil check" - return t, *p // ERROR "removed nil check" -} - -// make sure to remove nil check for memory move (issue #18003) -func f8(t *[8]int) [8]int { - return *t // ERROR "generated nil check" -} - -func f9() []int { - x := new([1]int) - x[0] = 1 // ERROR "removed nil check" - y := x[:] // ERROR "removed nil check" - return y -} diff --git a/test/nilptr5.go b/test/nilptr5.go new file mode 100644 index 0000000000..2c48c0b261 --- /dev/null +++ b/test/nilptr5.go @@ -0,0 +1,33 @@ +// errorcheck -0 -d=nil + +// +build !wasm +// +build !aix + +// Copyright 2018 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. + +// Test that nil checks are removed. +// Optimization is enabled. + +package p + +func f5(p *float32, q *float64, r *float32, s *float64) float64 { + x := float64(*p) // ERROR "removed nil check" + y := *q // ERROR "removed nil check" + *r = 7 // ERROR "removed nil check" + *s = 9 // ERROR "removed nil check" + return x + y +} + +type T [29]byte + +func f6(p, q *T) { + x := *p // ERROR "removed nil check" + *q = x // ERROR "removed nil check" +} + +// make sure to remove nil check for memory move (issue #18003) +func f8(t *[8]int) [8]int { + return *t // ERROR "removed nil check" +} diff --git a/test/nilptr5_aix.go b/test/nilptr5_aix.go new file mode 100644 index 0000000000..ff6900593b --- /dev/null +++ b/test/nilptr5_aix.go @@ -0,0 +1,32 @@ +// errorcheck -0 -d=nil + +// +build aix + +// Copyright 2018 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. + +// Test that nil checks are removed. +// Optimization is enabled. + +package p + +func f5(p *float32, q *float64, r *float32, s *float64) float64 { + x := float64(*p) // ERROR "generated nil check" + y := *q // ERROR "generated nil check" + *r = 7 // ERROR "removed nil check" + *s = 9 // ERROR "removed nil check" + return x + y +} + +type T [29]byte + +func f6(p, q *T) { + x := *p // ERROR "generated nil check" + *q = x // ERROR "generated nil check" +} + +// make sure to remove nil check for memory move (issue #18003) +func f8(t *[8]int) [8]int { + return *t // ERROR "generated nil check" +} diff --git a/test/nilptr5_wasm.go b/test/nilptr5_wasm.go new file mode 100644 index 0000000000..6ef8a02e90 --- /dev/null +++ b/test/nilptr5_wasm.go @@ -0,0 +1,32 @@ +// errorcheck -0 -d=nil + +// +build wasm + +// Copyright 2018 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. + +// Test that nil checks are removed. +// Optimization is enabled. + +package p + +func f5(p *float32, q *float64, r *float32, s *float64) float64 { + x := float64(*p) // ERROR "generated nil check" + y := *q // ERROR "generated nil check" + *r = 7 // ERROR "generated nil check" + *s = 9 // ERROR "generated nil check" + return x + y +} + +type T [29]byte + +func f6(p, q *T) { + x := *p // ERROR "generated nil check" + *q = x // ERROR "generated nil check" +} + +// make sure to remove nil check for memory move (issue #18003) +func f8(t *[8]int) [8]int { + return *t // ERROR "generated nil check" +} diff --git a/test/nilptr_aix.go b/test/nilptr_aix.go new file mode 100644 index 0000000000..ea5fcc3f4e --- /dev/null +++ b/test/nilptr_aix.go @@ -0,0 +1,185 @@ +// run + +// Copyright 2018 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. + +// Test that the implementation catches nil ptr indirection +// in a large address space. + +// +build aix + +package main + +import "unsafe" + +// Having a big address space means that indexing +// at a 1G + 256 MB offset from a nil pointer might not +// cause a memory access fault. This test checks +// that Go is doing the correct explicit checks to catch +// these nil pointer accesses, not just relying on the hardware. +// The reason of the 1G offset is because AIX addresses start after 1G. +var dummy [256 << 20]byte // give us a big address space + +func main() { + // the test only tests what we intend to test + // if dummy starts in the first 256 MB of memory. + // otherwise there might not be anything mapped + // at the address that might be accidentally + // dereferenced below. + if uintptr(unsafe.Pointer(&dummy)) < 1<<32 { + panic("dummy not far enough") + } + + shouldPanic(p1) + shouldPanic(p2) + shouldPanic(p3) + shouldPanic(p4) + shouldPanic(p5) + shouldPanic(p6) + shouldPanic(p7) + shouldPanic(p8) + shouldPanic(p9) + shouldPanic(p10) + shouldPanic(p11) + shouldPanic(p12) + shouldPanic(p13) + shouldPanic(p14) + shouldPanic(p15) + shouldPanic(p16) +} + +func shouldPanic(f func()) { + defer func() { + if recover() == nil { + panic("memory reference did not panic") + } + }() + f() +} + +func p1() { + // Array index. + var p *[1 << 33]byte = nil + println(p[1<<32+256<<20]) // very likely to be inside dummy, but should panic +} + +var xb byte + +func p2() { + var p *[1 << 33]byte = nil + xb = 123 + + // Array index. + println(p[uintptr(unsafe.Pointer(&xb))]) // should panic +} + +func p3() { + // Array to slice. + var p *[1 << 33]byte = nil + var x []byte = p[0:] // should panic + _ = x +} + +var q *[1 << 33]byte + +func p4() { + // Array to slice. + var x []byte + var y = &x + *y = q[0:] // should crash (uses arraytoslice runtime routine) +} + +func fb([]byte) { + panic("unreachable") +} + +func p5() { + // Array to slice. + var p *[1 << 33]byte = nil + fb(p[0:]) // should crash +} + +func p6() { + // Array to slice. + var p *[1 << 33]byte = nil + var _ []byte = p[10 : len(p)-10] // should crash +} + +type T struct { + x [1<<32 + 256<<20]byte + i int +} + +func f() *T { + return nil +} + +var y *T +var x = &y + +func p7() { + // Struct field access with large offset. + println(f().i) // should crash +} + +func p8() { + // Struct field access with large offset. + println((*x).i) // should crash +} + +func p9() { + // Struct field access with large offset. + var t *T + println(&t.i) // should crash +} + +func p10() { + // Struct field access with large offset. + var t *T + println(t.i) // should crash +} + +type T1 struct { + T +} + +type T2 struct { + *T1 +} + +func p11() { + t := &T2{} + p := &t.i + println(*p) +} + +// ADDR(DOT(IND(p))) needs a check also +func p12() { + var p *T = nil + println(*(&((*p).i))) +} + +// Tests suggested in golang.org/issue/6080. + +func p13() { + var x *[10]int + y := x[:] + _ = y +} + +func p14() { + println((*[1]int)(nil)[:]) +} + +func p15() { + for i := range (*[1]int)(nil)[:] { + _ = i + } +} + +func p16() { + for i, v := range (*[1]int)(nil)[:] { + _ = i + v + } +} -- 2.50.0