From 9e619739fdc4cbaeb00a10ef95ce3e5d6996e8a7 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Sat, 3 Nov 2018 09:09:28 -0700 Subject: [PATCH] cmd/compile: copy all fields during SubstAny MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consider these functions: func f(a any) int func g(a any) int Prior to this change, since f and g have identical signatures, they would share a single generated func type. types.SubstAny makes a shallow type copy, even after instantiation, f and g share a single generated Result type. So if you instantiate f with any=T, call dowidth, instantiate g with any=U, and call dowidth, and if sizeof(T) != sizeof(U), then the Offset of the result for f is now wrong. I don't believe this happens at all right now, but it bit me hard when experimenting with some other compiler changes. And it's hard to debug. It results in rare stack corruption, causing problems far from the actual source of the problem. To fix this, change SubstAny to make deep copies of TSTRUCTs. name old alloc/op new alloc/op delta Template 35.3MB ± 0% 35.4MB ± 0% +0.23% (p=0.008 n=5+5) Unicode 29.1MB ± 0% 29.1MB ± 0% +0.16% (p=0.008 n=5+5) GoTypes 122MB ± 0% 122MB ± 0% +0.16% (p=0.008 n=5+5) Compiler 513MB ± 0% 514MB ± 0% +0.19% (p=0.008 n=5+5) SSA 1.94GB ± 0% 1.94GB ± 0% +0.01% (p=0.008 n=5+5) Flate 24.2MB ± 0% 24.2MB ± 0% +0.08% (p=0.008 n=5+5) GoParser 28.5MB ± 0% 28.5MB ± 0% +0.24% (p=0.008 n=5+5) Reflect 86.2MB ± 0% 86.3MB ± 0% +0.09% (p=0.008 n=5+5) Tar 34.9MB ± 0% 34.9MB ± 0% +0.13% (p=0.008 n=5+5) XML 47.0MB ± 0% 47.1MB ± 0% +0.18% (p=0.008 n=5+5) [Geo mean] 80.9MB 81.0MB +0.15% name old allocs/op new allocs/op delta Template 348k ± 0% 349k ± 0% +0.38% (p=0.008 n=5+5) Unicode 340k ± 0% 340k ± 0% +0.21% (p=0.008 n=5+5) GoTypes 1.27M ± 0% 1.28M ± 0% +0.27% (p=0.008 n=5+5) Compiler 4.90M ± 0% 4.92M ± 0% +0.36% (p=0.008 n=5+5) SSA 15.3M ± 0% 15.3M ± 0% +0.03% (p=0.008 n=5+5) Flate 232k ± 0% 233k ± 0% +0.14% (p=0.008 n=5+5) GoParser 291k ± 0% 292k ± 0% +0.42% (p=0.008 n=5+5) Reflect 1.05M ± 0% 1.05M ± 0% +0.14% (p=0.008 n=5+5) Tar 343k ± 0% 344k ± 0% +0.22% (p=0.008 n=5+5) XML 428k ± 0% 430k ± 0% +0.36% (p=0.008 n=5+5) [Geo mean] 807k 809k +0.25% Change-Id: I62134db642206cded01920dc1d8a7da61f7ca0ac Reviewed-on: https://go-review.googlesource.com/c/147038 Run-TryBot: Josh Bleecher Snyder TryBot-Result: Gobot Gobot Reviewed-by: Matthew Dempsky --- src/cmd/compile/internal/types/type.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/cmd/compile/internal/types/type.go b/src/cmd/compile/internal/types/type.go index b20039239b..45355e5798 100644 --- a/src/cmd/compile/internal/types/type.go +++ b/src/cmd/compile/internal/types/type.go @@ -665,23 +665,18 @@ func SubstAny(t *Type, types *[]*Type) *Type { } case TSTRUCT: + // Make a copy of all fields, including ones whose type does not change. + // This prevents aliasing across functions, which can lead to later + // fields getting their Offset incorrectly overwritten. fields := t.FieldSlice() - var nfs []*Field + nfs := make([]*Field, len(fields)) for i, f := range fields { nft := SubstAny(f.Type, types) - if nft == f.Type { - continue - } - if nfs == nil { - nfs = append([]*Field(nil), fields...) - } nfs[i] = f.Copy() nfs[i].Type = nft } - if nfs != nil { - t = t.copy() - t.SetFields(nfs) - } + t = t.copy() + t.SetFields(nfs) } return t