From: Keith Randall Date: Sun, 15 Jun 2025 03:10:50 +0000 (-0700) Subject: cmd/compile: allow multi-field structs to be stored directly in interfaces X-Git-Tag: go1.26rc1~277 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=03ed43988f;p=gostls13.git cmd/compile: allow multi-field structs to be stored directly in interfaces If the struct is a bunch of 0-sized fields and one pointer field. Merged revert-of-revert for 4 CLs. original revert 681937 695016 693415 694996 693615 695015 694195 694995 Fixes #74092 Update #74888 Update #74908 Update #74935 (updated issues are bugs in the last attempt at this) Change-Id: I32246d49b8bac3bb080972dc06ab432a5480d560 Reviewed-on: https://go-review.googlesource.com/c/go/+/714421 Auto-Submit: Keith Randall Reviewed-by: David Chase LUCI-TryBot-Result: Go LUCI Reviewed-by: Keith Randall --- diff --git a/src/cmd/compile/internal/ssa/_gen/dec.rules b/src/cmd/compile/internal/ssa/_gen/dec.rules index 9f6dc36975..fce0026211 100644 --- a/src/cmd/compile/internal/ssa/_gen/dec.rules +++ b/src/cmd/compile/internal/ssa/_gen/dec.rules @@ -97,8 +97,10 @@ // Helpers for expand calls // Some of these are copied from generic.rules -(IMake _typ (StructMake val)) => (IMake _typ val) -(StructSelect [0] (IData x)) => (IData x) +(IMake _typ (StructMake ___)) => imakeOfStructMake(v) +(StructSelect (IData x)) && v.Type.Size() > 0 => (IData x) +(StructSelect (IData x)) && v.Type.Size() == 0 && v.Type.IsStruct() => (StructMake) +(StructSelect (IData x)) && v.Type.Size() == 0 && v.Type.IsArray() => (ArrayMake0) (StructSelect [i] x:(StructMake ___)) => x.Args[i] @@ -109,7 +111,7 @@ // More annoying case: (ArraySelect[0] (StructSelect[0] isAPtr)) // There, result of the StructSelect is an Array (not a pointer) and // the pre-rewrite input to the ArraySelect is a struct, not a pointer. -(StructSelect [0] x) && x.Type.IsPtrShaped() => x +(StructSelect x) && x.Type.IsPtrShaped() => x (ArraySelect [0] x) && x.Type.IsPtrShaped() => x // These, too. Bits is bits. @@ -119,6 +121,7 @@ (Store _ (StructMake ___) _) => rewriteStructStore(v) +(IMake _typ (ArrayMake1 val)) => (IMake _typ val) (ArraySelect (ArrayMake1 x)) => x (ArraySelect [0] (IData x)) => (IData x) diff --git a/src/cmd/compile/internal/ssa/_gen/generic.rules b/src/cmd/compile/internal/ssa/_gen/generic.rules index e09cd31c31..372e89863d 100644 --- a/src/cmd/compile/internal/ssa/_gen/generic.rules +++ b/src/cmd/compile/internal/ssa/_gen/generic.rules @@ -944,8 +944,10 @@ @x.Block (Load (OffPtr [t.FieldOff(int(i))] ptr) mem) // Putting struct{*byte} and similar into direct interfaces. -(IMake _typ (StructMake val)) => (IMake _typ val) -(StructSelect [0] (IData x)) => (IData x) +(IMake _typ (StructMake ___)) => imakeOfStructMake(v) +(StructSelect (IData x)) && v.Type.Size() > 0 => (IData x) +(StructSelect (IData x)) && v.Type.Size() == 0 && v.Type.IsStruct() => (StructMake) +(StructSelect (IData x)) && v.Type.Size() == 0 && v.Type.IsArray() => (ArrayMake0) // un-SSAable values use mem->mem copies (Store {t} dst (Load src mem) mem) && !CanSSA(t) => diff --git a/src/cmd/compile/internal/ssa/expand_calls.go b/src/cmd/compile/internal/ssa/expand_calls.go index 8a5b364c2f..6afce759b2 100644 --- a/src/cmd/compile/internal/ssa/expand_calls.go +++ b/src/cmd/compile/internal/ssa/expand_calls.go @@ -423,7 +423,14 @@ func (x *expandState) decomposeAsNecessary(pos src.XPos, b *Block, a, m0 *Value, if a.Op == OpIMake { data := a.Args[1] for data.Op == OpStructMake || data.Op == OpArrayMake1 { - data = data.Args[0] + // A struct make might have a few zero-sized fields. + // Use the pointer-y one we know is there. + for _, a := range data.Args { + if a.Type.Size() > 0 { + data = a + break + } + } } return x.decomposeAsNecessary(pos, b, data, mem, rc.next(data.Type)) } diff --git a/src/cmd/compile/internal/ssa/rewrite.go b/src/cmd/compile/internal/ssa/rewrite.go index 07308973b1..af2568ae89 100644 --- a/src/cmd/compile/internal/ssa/rewrite.go +++ b/src/cmd/compile/internal/ssa/rewrite.go @@ -2772,3 +2772,17 @@ func panicBoundsCCToAux(p PanicBoundsCC) Aux { func isDictArgSym(sym Sym) bool { return sym.(*ir.Name).Sym().Name == typecheck.LocalDictName } + +// When v is (IMake typ (StructMake ...)), convert to +// (IMake typ arg) where arg is the pointer-y argument to +// the StructMake (there must be exactly one). +func imakeOfStructMake(v *Value) *Value { + var arg *Value + for _, a := range v.Args[1].Args { + if a.Type.Size() > 0 { + arg = a + break + } + } + return v.Block.NewValue2(v.Pos, OpIMake, v.Type, v.Args[0], arg) +} diff --git a/src/cmd/compile/internal/ssa/rewritedec.go b/src/cmd/compile/internal/ssa/rewritedec.go index 16d0269210..c45034ead0 100644 --- a/src/cmd/compile/internal/ssa/rewritedec.go +++ b/src/cmd/compile/internal/ssa/rewritedec.go @@ -279,11 +279,20 @@ func rewriteValuedec_OpIData(v *Value) bool { func rewriteValuedec_OpIMake(v *Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] - // match: (IMake _typ (StructMake val)) + // match: (IMake _typ (StructMake ___)) + // result: imakeOfStructMake(v) + for { + if v_1.Op != OpStructMake { + break + } + v.copyOf(imakeOfStructMake(v)) + return true + } + // match: (IMake _typ (ArrayMake1 val)) // result: (IMake _typ val) for { _typ := v_0 - if v_1.Op != OpStructMake || len(v_1.Args) != 1 { + if v_1.Op != OpArrayMake1 { break } val := v_1.Args[0] @@ -839,17 +848,47 @@ func rewriteValuedec_OpStructMake(v *Value) bool { func rewriteValuedec_OpStructSelect(v *Value) bool { v_0 := v.Args[0] b := v.Block - // match: (StructSelect [0] (IData x)) + // match: (StructSelect (IData x)) + // cond: v.Type.Size() > 0 // result: (IData x) for { - if auxIntToInt64(v.AuxInt) != 0 || v_0.Op != OpIData { + if v_0.Op != OpIData { break } x := v_0.Args[0] + if !(v.Type.Size() > 0) { + break + } v.reset(OpIData) v.AddArg(x) return true } + // match: (StructSelect (IData x)) + // cond: v.Type.Size() == 0 && v.Type.IsStruct() + // result: (StructMake) + for { + if v_0.Op != OpIData { + break + } + if !(v.Type.Size() == 0 && v.Type.IsStruct()) { + break + } + v.reset(OpStructMake) + return true + } + // match: (StructSelect (IData x)) + // cond: v.Type.Size() == 0 && v.Type.IsArray() + // result: (ArrayMake0) + for { + if v_0.Op != OpIData { + break + } + if !(v.Type.Size() == 0 && v.Type.IsArray()) { + break + } + v.reset(OpArrayMake0) + return true + } // match: (StructSelect [i] x:(StructMake ___)) // result: x.Args[i] for { @@ -861,13 +900,10 @@ func rewriteValuedec_OpStructSelect(v *Value) bool { v.copyOf(x.Args[i]) return true } - // match: (StructSelect [0] x) + // match: (StructSelect x) // cond: x.Type.IsPtrShaped() // result: x for { - if auxIntToInt64(v.AuxInt) != 0 { - break - } x := v_0 if !(x.Type.IsPtrShaped()) { break diff --git a/src/cmd/compile/internal/ssa/rewritegeneric.go b/src/cmd/compile/internal/ssa/rewritegeneric.go index 1621153b43..f0560671ac 100644 --- a/src/cmd/compile/internal/ssa/rewritegeneric.go +++ b/src/cmd/compile/internal/ssa/rewritegeneric.go @@ -8985,16 +8985,13 @@ func rewriteValuegeneric_OpFloor(v *Value) bool { func rewriteValuegeneric_OpIMake(v *Value) bool { v_1 := v.Args[1] v_0 := v.Args[0] - // match: (IMake _typ (StructMake val)) - // result: (IMake _typ val) + // match: (IMake _typ (StructMake ___)) + // result: imakeOfStructMake(v) for { - _typ := v_0 - if v_1.Op != OpStructMake || len(v_1.Args) != 1 { + if v_1.Op != OpStructMake { break } - val := v_1.Args[0] - v.reset(OpIMake) - v.AddArg2(_typ, val) + v.copyOf(imakeOfStructMake(v)) return true } // match: (IMake _typ (ArrayMake1 val)) @@ -32109,17 +32106,47 @@ func rewriteValuegeneric_OpStructSelect(v *Value) bool { v0.AddArg2(v1, mem) return true } - // match: (StructSelect [0] (IData x)) + // match: (StructSelect (IData x)) + // cond: v.Type.Size() > 0 // result: (IData x) for { - if auxIntToInt64(v.AuxInt) != 0 || v_0.Op != OpIData { + if v_0.Op != OpIData { break } x := v_0.Args[0] + if !(v.Type.Size() > 0) { + break + } v.reset(OpIData) v.AddArg(x) return true } + // match: (StructSelect (IData x)) + // cond: v.Type.Size() == 0 && v.Type.IsStruct() + // result: (StructMake) + for { + if v_0.Op != OpIData { + break + } + if !(v.Type.Size() == 0 && v.Type.IsStruct()) { + break + } + v.reset(OpStructMake) + return true + } + // match: (StructSelect (IData x)) + // cond: v.Type.Size() == 0 && v.Type.IsArray() + // result: (ArrayMake0) + for { + if v_0.Op != OpIData { + break + } + if !(v.Type.Size() == 0 && v.Type.IsArray()) { + break + } + v.reset(OpArrayMake0) + return true + } return false } func rewriteValuegeneric_OpSub16(v *Value) bool { diff --git a/src/cmd/compile/internal/types/type.go b/src/cmd/compile/internal/types/type.go index 8de589bae3..2f5de288c2 100644 --- a/src/cmd/compile/internal/types/type.go +++ b/src/cmd/compile/internal/types/type.go @@ -1822,26 +1822,7 @@ func IsReflexive(t *Type) bool { // Can this type be stored directly in an interface word? // Yes, if the representation is a single pointer. func IsDirectIface(t *Type) bool { - switch t.Kind() { - case TPTR: - // Pointers to notinheap types must be stored indirectly. See issue 42076. - return !t.Elem().NotInHeap() - case TCHAN, - TMAP, - TFUNC, - TUNSAFEPTR: - return true - - case TARRAY: - // Array of 1 direct iface type can be direct. - return t.NumElem() == 1 && IsDirectIface(t.Elem()) - - case TSTRUCT: - // Struct with 1 field of direct iface type can be direct. - return t.NumFields() == 1 && IsDirectIface(t.Field(0).Type) - } - - return false + return t.Size() == int64(PtrSize) && PtrDataSize(t) == int64(PtrSize) } // IsInterfaceMethod reports whether (field) m is diff --git a/src/internal/abi/type.go b/src/internal/abi/type.go index 7f44a9de56..243b787cfc 100644 --- a/src/internal/abi/type.go +++ b/src/internal/abi/type.go @@ -121,8 +121,8 @@ const ( TFlagGCMaskOnDemand TFlag = 1 << 4 // TFlagDirectIface means that a value of this type is stored directly - // in the data field of an interface, instead of indirectly. Normally - // this means the type is pointer-ish. + // in the data field of an interface, instead of indirectly. + // This flag is just a cached computation of Size_ == PtrBytes == goarch.PtrSize. TFlagDirectIface TFlag = 1 << 5 // Leaving this breadcrumb behind for dlv. It should not be used, and no diff --git a/src/reflect/type.go b/src/reflect/type.go index 9b8726824e..914b5443f3 100644 --- a/src/reflect/type.go +++ b/src/reflect/type.go @@ -2528,8 +2528,7 @@ func StructOf(fields []StructField) Type { } switch { - case len(fs) == 1 && fs[0].Typ.IsDirectIface(): - // structs of 1 direct iface type can be direct + case typ.Size_ == goarch.PtrSize && typ.PtrBytes == goarch.PtrSize: typ.TFlag |= abi.TFlagDirectIface default: typ.TFlag &^= abi.TFlagDirectIface @@ -2698,8 +2697,7 @@ func ArrayOf(length int, elem Type) Type { } switch { - case length == 1 && typ.IsDirectIface(): - // array of 1 direct iface type can be direct + case array.Size_ == goarch.PtrSize && array.PtrBytes == goarch.PtrSize: array.TFlag |= abi.TFlagDirectIface default: array.TFlag &^= abi.TFlagDirectIface diff --git a/src/reflect/value.go b/src/reflect/value.go index b5d5aa8bf2..a82d976c47 100644 --- a/src/reflect/value.go +++ b/src/reflect/value.go @@ -1279,6 +1279,17 @@ func (v Value) Field(i int) Value { fl |= flagStickyRO } } + if fl&flagIndir == 0 && typ.Size() == 0 { + // Special case for picking a field out of a direct struct. + // A direct struct must have a pointer field and possibly a + // bunch of zero-sized fields. We must return the zero-sized + // fields indirectly, as only ptr-shaped things can be direct. + // See issue 74935. + // We use nil instead of v.ptr as it doesn't matter and + // we can avoid pinning a possibly now-unused object. + return Value{typ, nil, fl | flagIndir} + } + // Either flagIndir is set and v.ptr points at struct, // or flagIndir is not set and v.ptr is the actual struct data. // In the former case, we want v.ptr + offset. diff --git a/test/fixedbugs/issue74888.go b/test/fixedbugs/issue74888.go new file mode 100644 index 0000000000..a0083adb9c --- /dev/null +++ b/test/fixedbugs/issue74888.go @@ -0,0 +1,20 @@ +// compile + +// Copyright 2025 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 + +type P struct { + q struct{} + p *int +} + +func f(x any) { + h(x.(P)) +} + +//go:noinline +func h(P) { +} diff --git a/test/fixedbugs/issue74908.go b/test/fixedbugs/issue74908.go new file mode 100644 index 0000000000..cb2e951768 --- /dev/null +++ b/test/fixedbugs/issue74908.go @@ -0,0 +1,24 @@ +// compile + +// Copyright 2025 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 + +type Type struct { + any +} + +type typeObject struct { + e struct{} + b *byte +} + +func f(b *byte) Type { + return Type{ + typeObject{ + b: b, + }, + } +} diff --git a/test/fixedbugs/issue74935.go b/test/fixedbugs/issue74935.go new file mode 100644 index 0000000000..1f6f718b02 --- /dev/null +++ b/test/fixedbugs/issue74935.go @@ -0,0 +1,19 @@ +// run + +// Copyright 2025 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 "reflect" + +type W struct { + E struct{} + X *byte +} + +func main() { + w := reflect.ValueOf(W{}) + _ = w.Field(0).Interface() +}