Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: register ABI bringup testcase failures (tracking issue) #44816

Open
thanm opened this issue Mar 5, 2021 · 36 comments
Open

cmd/compile: register ABI bringup testcase failures (tracking issue) #44816

thanm opened this issue Mar 5, 2021 · 36 comments
Assignees

Comments

@thanm
Copy link
Contributor

@thanm thanm commented Mar 5, 2021

Note: this is not a regular bug, but instead tracking issue in which to collect testcases that cause failures with the new Go register ABI work.

What version of Go are you using (go version)?

$ go version
go version devel +a2f7067233 Fri Mar 5 15:34:32 2021 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

First instance:

$ git clone https://github.com/thanm/cabi-testgen
...
$ cd cabi-testgen/cabi-testgen
$ git checkout 0779299
$ go build .
$ ./cabi-testgen -q 1 -n 1 -s 1614962541 -o /tmp/cabiTest -p cabiTest -pragma registerparams
$ cd  /tmp/cabiTest
$ go build -p 1 .

What did you expect to see?

clean build

What did you see instead?

testcase.go:37:6: internal compiler error: 'Test0': not lowered: v10, StringPtr PTR STRING

goroutine 19 [running]:
runtime/debug.Stack(0xf9ecc0, 0xc0000be008, 0x0)
	/w/go/src/runtime/debug/stack.go:24 +0x9f
cmd/compile/internal/base.FatalfAt(0x2506000000002, 0xc0004b1cc8, 0x8, 0xc0003ad880, 0x2, 0x2)
	/w/go/src/cmd/compile/internal/base/print.go:227 +0x1bc
cmd/compile/internal/base.Fatalf(...)
	/w/go/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/ssagen.(*ssafn).Fatalf(0xc0003dd2f0, 0x2506000000002, 0xe4e7d2, 0x2, 0xc00009fcf0, 0x1, 0x1)
	/w/go/src/cmd/compile/internal/ssagen/ssa.go:7353 +0x1a7
cmd/compile/internal/ssa.(*Func).Fatalf(...)
	/w/go/src/cmd/compile/internal/ssa/func.go:656
cmd/compile/internal/ssa.checkLower(0xc00017dba0)
	/w/go/src/cmd/compile/internal/ssa/lower.go:41 +0x439
cmd/compile/internal/ssa.Compile(0xc00017dba0)
	/w/go/src/cmd/compile/internal/ssa/compile.go:96 +0x99e
cmd/compile/internal/ssagen.buildssa(0xc000100580, 0x0, 0x0)
	/w/go/src/cmd/compile/internal/ssagen/ssa.go:635 +0x18d2
cmd/compile/internal/ssagen.Compile(0xc000100580, 0x0)
	/w/go/src/cmd/compile/internal/ssagen/pgen.go:158 +0x5f
cmd/compile/internal/gc.compileFunctions.func2.1(0xc000124310, 0xc000100580, 0xc0000be700, 0xc0000bb5c0)
	/w/go/src/cmd/compile/internal/gc/compile.go:130 +0x65
created by cmd/compile/internal/gc.compileFunctions.func2
	/w/go/src/cmd/compile/internal/gc/compile.go:128 +0x8e


Reduced testcase attached, repro with "go tool compile -p x testcase.go".
testcase.go.txt

@thanm thanm self-assigned this Mar 5, 2021
@thanm
Copy link
Contributor Author

@thanm thanm commented Mar 5, 2021

@thanm
Copy link
Contributor Author

@thanm thanm commented Mar 5, 2021

OK, here is a second problem (different failure mode). This is on the caller side (requires two packages).

Recipe:

$ git clone https://github.com/thanm/cabi-testgen
...
$ cd cabi-testgen/cabi-testgen
$ git checkout 0779299
$ go build .
$ ./cabi-testgen -q 1 -n 1 -s 2345 -o /tmp/cabiTest -p cabiTest -pragma registerparams
$ cd  /tmp/cabiTest
$ go build -p 1 .

Failure mode:

# cabiTest/genCaller0
genCaller0/genCaller0.go:43:9: internal compiler error: 'Caller0': panic during schedule while compiling Caller0:

runtime error: index out of range [10] with length 9

goroutine 8 [running]:
cmd/compile/internal/ssa.Compile.func1(0xc000558cf8, 0xc00007e340)
	/w/go/src/cmd/compile/internal/ssa/compile.go:48 +0xa5
panic(0xe105a0, 0xc0000e86c0)
	/w/go/src/runtime/panic.go:972 +0x1d4
cmd/compile/internal/ssa.schedule(0xc00007e340)
	/w/go/src/cmd/compile/internal/ssa/schedule.go:277 +0x235c
cmd/compile/internal/ssa.Compile(0xc00007e340)
	/w/go/src/cmd/compile/internal/ssa/compile.go:96 +0x99e
cmd/compile/internal/ssagen.buildssa(0xc0001022c0, 0x0, 0x0)
	/w/go/src/cmd/compile/internal/ssagen/ssa.go:635 +0x18d2
cmd/compile/internal/ssagen.Compile(0xc0001022c0, 0x0)
	/w/go/src/cmd/compile/internal/ssagen/pgen.go:158 +0x5f
cmd/compile/internal/gc.compileFunctions.func2.1(0xc0000b8000, 0xc0001022c0, 0xc0000be088, 0xc0000ba1a0)
	/w/go/src/cmd/compile/internal/gc/compile.go:130 +0x65
created by cmd/compile/internal/gc.compileFunctions.func2
	/w/go/src/cmd/compile/internal/gc/compile.go:128 +0x8e

To reproduce, download the cabiTest.zip file, then

$ unzip cabiTest.zip
$ cd cabiTest
$ go build -p 1 .

cabiTest.zip

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Mar 5, 2021

The following program fails to compile at tip (with GOEXPERIMENT=regabi set during ./make.bash of course):

package main

import "fmt"

//go:registerparams
//go:noinline
func passStruct6(a Struct6) Struct6 {
        return a
}

type Struct6 struct {
        Struct1
}

type Struct1 struct {
        A, B, C uint
}

func main() {
        fmt.Println(passStruct6(Struct6{Struct1{1, 2, 3}}))
}

Output:

$ ~/toolchain/bin/go run error.go
# command-line-arguments
./error.go:8:2: internal compiler error: 'passStruct6': not lowered: v4, StructSelect UINT STRUCT

goroutine 14 [running]:
runtime/debug.Stack(0xf7e1e0, 0xc00000e018, 0x0)
        /usr/local/google/home/mknyszek/toolchain/src/runtime/debug/stack.go:24 +0x94
cmd/compile/internal/base.FatalfAt(0x802100000002, 0xc0005d3cc8, 0x8, 0xc0004812e0, 0x2, 0x2)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/base/print.go:227 +0x1ac
cmd/compile/internal/base.Fatalf(...)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/ssagen.(*ssafn).Fatalf(0xc00010a3c0, 0x802100000002, 0xe2da72, 0x2, 0xc0000acf60, 0x1, 0x1)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/ssa.go:7353 +0x195
cmd/compile/internal/ssa.(*Func).Fatalf(...)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/func.go:656
cmd/compile/internal/ssa.checkLower(0xc0003f81a0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/lower.go:41 +0x3fa
cmd/compile/internal/ssa.Compile(0xc0003f81a0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/compile.go:96 +0x98d
cmd/compile/internal/ssagen.buildssa(0xc0003d4160, 0x0, 0x0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/ssa.go:635 +0x1892
cmd/compile/internal/ssagen.Compile(0xc0003d4160, 0x0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/pgen.go:158 +0x52
cmd/compile/internal/gc.compileFunctions.func2.1(0xc0003f0080, 0xc0003d4160, 0xc0000a24e8, 0xc0003dd010)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/gc/compile.go:130 +0x56
created by cmd/compile/internal/gc.compileFunctions.func2
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/gc/compile.go:128 +0x85

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Mar 5, 2021

Another ICE at tip:

package main

import "fmt"

//go:registerparams
//go:noinline
func passStruct8(a Struct8) Struct8 {
        return a
}

type Struct8 struct {
        Struct5
        Struct1
}

type Struct1 struct {
        A, B, C uint
}

type Struct5 struct {
        A             uint16
        B             int16
        C, D          uint32
        E             int32
        F, G, H, I, J float32
}

func main() {
        fmt.Println(passStruct8(Struct8{
                Struct5{1, 2, 3, 4, 5, 1.0, 2.0, 3.0, 4.0, 5.0},
                Struct1{1, 2, 3},
        }))
}

Output:

# command-line-arguments                                                                                                                                                                                    
./error2.go:33:1: internal compiler error: 'main': panic during regalloc while compiling main:                                                                                                              
                                                                                                                                                                                                            
nil v, s.orig[8], vi = {0 <nil> 0xc0006d0518 2 5 false false}, spill = v103 = StoreReg <mem>                                                                                                                
                                                                                                                                                                                                            
goroutine 9 [running]:                        
cmd/compile/internal/ssa.Compile.func1(0xc0004bccf8, 0xc0000a4000)                                 
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/compile.go:48 +0x9c
panic(0xd9d380, 0xc000068330)                                                                         
        /usr/local/google/home/mknyszek/toolchain/src/runtime/panic.go:972 +0x1c5
cmd/compile/internal/ssa.(*regAllocState).placeSpills(0xc000082480)                                
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/regalloc.go:1816 +0xed8  
cmd/compile/internal/ssa.(*regAllocState).regalloc(0xc000082480, 0xc0000a4000)                    
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/regalloc.go:1730 +0x73e6
cmd/compile/internal/ssa.regalloc(0xc0000a4000)                                                       
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/regalloc.go:147 +0x5d
cmd/compile/internal/ssa.Compile(0xc0000a4000)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/compile.go:96 +0x98d
cmd/compile/internal/ssagen.buildssa(0xc0001042c0, 0x1, 0x0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/ssa.go:635 +0x1892
cmd/compile/internal/ssagen.Compile(0xc0001042c0, 0x1)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/pgen.go:158 +0x52
cmd/compile/internal/gc.compileFunctions.func2.1(0xc000030100, 0xc0001042c0, 0xc0000ba700, 0xc0000b72b0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/gc/compile.go:130 +0x56
created by cmd/compile/internal/gc.compileFunctions.func2
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/gc/compile.go:128 +0x85



goroutine 9 [running]:
runtime/debug.Stack(0xf7e1e0, 0xc0000ba008, 0x0)
        /usr/local/google/home/mknyszek/toolchain/src/runtime/debug/stack.go:24 +0x94
cmd/compile/internal/base.FatalfAt(0x2101000000002, 0xc00043a3c0, 0x32, 0xc000490500, 0x5, 0x5)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/base/print.go:227 +0x1ac
cmd/compile/internal/base.Fatalf(...)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/ssagen.(*ssafn).Fatalf(0xc00061c600, 0x2101000000002, 0xe55822, 0x2c, 0xc000420740, 0x4, 0x4)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/ssa.go:7353 +0x195
cmd/compile/internal/ssa.(*Func).Fatalf(...)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/func.go:656
cmd/compile/internal/ssa.Compile.func1(0xc0004bccf8, 0xc0000a4000)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/compile.go:53 +0x225
panic(0xd9d380, 0xc000068330)
        /usr/local/google/home/mknyszek/toolchain/src/runtime/panic.go:972 +0x1c5
cmd/compile/internal/ssa.(*regAllocState).placeSpills(0xc000082480)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/regalloc.go:1816 +0xed8 
cmd/compile/internal/ssa.(*regAllocState).regalloc(0xc000082480, 0xc0000a4000)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/regalloc.go:1730 +0x73e6
cmd/compile/internal/ssa.regalloc(0xc0000a4000)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/regalloc.go:147 +0x5d
cmd/compile/internal/ssa.Compile(0xc0000a4000)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssa/compile.go:96 +0x98d
cmd/compile/internal/ssagen.buildssa(0xc0001042c0, 0x1, 0x0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/ssa.go:635 +0x1892
cmd/compile/internal/ssagen.Compile(0xc0001042c0, 0x1)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/ssagen/pgen.go:158 +0x52
cmd/compile/internal/gc.compileFunctions.func2.1(0xc000030100, 0xc0001042c0, 0xc0000ba700, 0xc0000b72b0)
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/gc/compile.go:130 +0x56
created by cmd/compile/internal/gc.compileFunctions.func2
        /usr/local/google/home/mknyszek/toolchain/src/cmd/compile/internal/gc/compile.go:128 +0x85

@ALTree ALTree changed the title register ABI bringup testcase failures (tracking issue) cmd/compile: register ABI bringup testcase failures (tracking issue) Mar 5, 2021
@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Mar 5, 2021

I don't know what I changed, but now when I run that last test case I get

tmp $ ~/toolchain/bin/go run error2.go
# command-line-arguments
<autogenerated>:1: invalid instruction: 00022 (/usr/local/google/home/mknyszek/tmp/error2.go:7) MOVUPS  R8, X5
<autogenerated>:1: invalid instruction: 00028 (/usr/local/google/home/mknyszek/tmp/error2.go:7) MOVUPS  R9, X5
<autogenerated>:1: invalid instruction: 00034 (/usr/local/google/home/mknyszek/tmp/error2.go:7) MOVUPS  R10, X5
<autogenerated>:1: invalid instruction: 00228 (/usr/local/google/home/mknyszek/tmp/error2.go:8) MOVUPS  X5, R8
<autogenerated>:1: invalid instruction: 00228 (/usr/local/google/home/mknyszek/tmp/error2.go:8) MOVUPS  X6, R9
<autogenerated>:1: invalid instruction: 00228 (/usr/local/google/home/mknyszek/tmp/error2.go:8) MOVUPS  X7, R10

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 6, 2021

Change https://golang.org/cl/299389 mentions this issue: cmd/compile: fix OpArg decomposer for registers in expandCalls

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Mar 6, 2021

@mknyszek
Welcome to the land of concurrent compilation, it seemed like a good idea at the time. Your test shows two bugs.
Try

GOMAXPROCS=1 GOSSADIR=`pwd` GOSSAFUNC=passStruct8 go build -p 1 -gcflags='-c=1' mk.go

to see one of them more repeatably.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 6, 2021

Change https://golang.org/cl/299409 mentions this issue: cmd/compile: fix broken type+offset calc for register args

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 7, 2021

Change https://golang.org/cl/299549 mentions this issue: cmd/compile: fix confusion in generating SelectN index

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Mar 7, 2021

Smaller (edited) reproducer of the "invalid instruction" failures:

package main

import "fmt"

type P struct {
        a int8
        x float64
}

type T struct {
        d,e P
}

//go:registerparams
//go:noinline
func G(t T) float64 {
        return float64(t.d.a + t.e.a)+t.d.x+t.e.x
}

func main() {
        fmt.Println(G(T{P{10,20},P{30,40}}))
}

yields

go build mk.go
# command-line-arguments
<autogenerated>:1: cursym="".main, invalid instruction: 00034 (/Users/drchase/work/tmp/abi/mk.go:22)	MOVSD	$f64.4034000000000000(SB), BX
<autogenerated>:1: cursym="".main, invalid instruction: 00034 (/Users/drchase/work/tmp/abi/mk.go:22)	MOVL	$30, X0
<autogenerated>:1: cursym="".G, invalid instruction: 00017 (/Users/drchase/work/tmp/abi/mk.go:17)	MOVUPS	BX, X2

Says "autogenerated", but appears in the compilation output (v8), must be an artifact of the assembler reporting. I modified the diagnostic printer to be clear about what's being compiled.

# /Users/drchase/work/tmp/abi/mk.go
	00000 (16) TEXT "".G(SB), ABIInternal
	00001 (16) FUNCDATA $0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	00002 (16) FUNCDATA $1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
v7	00003 (+17) MOVL X0, CX
v13	00004 (17) ADDL CX, AX
v14	00005 (17) MOVBQSX AX, AX
v15	00006 (17) XORPS X0, X0
v15	00007 (17) CVTSL2SD AX, X0
v8	00008 (17) MOVUPS BX, X2
v18	00009 (17) ADDSD X2, X0
v21	00010 (17) ADDSD X1, X0
b1	00011 (17) RET
	00012 (?) END

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Mar 7, 2021

Found it. I thought that abiutils would render the parameter registers I0, F0, I1, F1 -- in the order that the parameters use them -- and instead it does them I0, I1, F0, F1. Oops.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 7, 2021

Change https://golang.org/cl/299570 mentions this issue: cmd/compile: fix failure to communicate between ABIinfo producer&consumer

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 8, 2021

Change https://golang.org/cl/299410 mentions this issue: cmd/compile: test register ABI for method, interface, closure calls

gopherbot pushed a commit that referenced this issue Mar 9, 2021
Includes test taken from
#44816 (comment)
and improved debugging output.

Updates #44816

Change-Id: I94aeb9c5255f175fe80727be29d218bad54bf7ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/299389
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 9, 2021
Includes more enhancements to debugging output.

Updates #44816.

Change-Id: I5b21815cf37ed21e7dec6c06f538090f32260203
Reviewed-on: https://go-review.googlesource.com/c/go/+/299409
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 9, 2021
Old: return the ABI register index of the result (wrong!)
New: return the index w/in sequence of result registers (right!)

Fixed bug:
genCaller0/genCaller0.go:43:9: internal compiler error: 'Caller0':
   panic during schedule while compiling Caller0:
runtime error: index out of range [10] with length 9

Updates #44816.

Change-Id: I1111e283658a2d6422986ae3d61bd95d1b9bde5e
Reviewed-on: https://go-review.googlesource.com/c/go/+/299549
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 9, 2021
…umer

ABI info producer and consumer had different ideas for register
order for parameters.

Includes a test, includes improvements to debugging output.

Updates #44816.

Change-Id: I4812976f7a6c08d6fc02aac1ec0544b1f141cca6
Reviewed-on: https://go-review.googlesource.com/c/go/+/299570
Trust: David Chase <drchase@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2021

Change https://golang.org/cl/300749 mentions this issue: cmd/compile: attempt to get spills right for output params in registers

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 12, 2021

Change https://golang.org/cl/300150 mentions this issue: cmd/compile: set unsayable "names" for regabi testing triggers

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 12, 2021

Change https://golang.org/cl/301270 mentions this issue: cmd/compile: minor cleanup -- remove dead code conditional on test

gopherbot pushed a commit that referenced this issue Mar 12, 2021
This is enabled with a ridiculous magic name for method,
or for last input type passed, that needs to be changed
to something inutterable before actual release.

Ridiculous method name: MagicMethodNameForTestingRegisterABI
Ridiculous last (input) type name: MagicLastTypeNameForTestingRegisterABI

RLTN is tested with strings.Contains, so you can have
MagicLastTypeNameForTestingRegisterABI1
and
MagicLastTypeNameForTestingRegisterABI2
if that is helpful

Includes test test/abi/fibish2.go

Updates #44816.

Change-Id: I592a6edc71ca9bebdd1d00e24edee1ceebb3e43f
Reviewed-on: https://go-review.googlesource.com/c/go/+/299410
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 12, 2021
It would fail now if it were turned on.

Updsates #44816.

Change-Id: I19d94f0cb2dd84271f5304c796d7c81e1e64af25
Reviewed-on: https://go-review.googlesource.com/c/go/+/301270
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 15, 2021
ALSO:
found evidence that stack maps for bodyless methods are wrong.
gofmt in test/abi
removed never-executed code in types/size.go

Updates #44816.

Change-Id: I658c33f049337fb6f1e625f0c55430d25bfa877e
Reviewed-on: https://go-review.googlesource.com/c/go/+/300749
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2021

Change https://golang.org/cl/302071 mentions this issue: cmd/compile: (fixed) spill output parameters passed in registers as autos

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2021

Change https://golang.org/cl/302249 mentions this issue: cmd/compile: fix open defer of method call

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Mar 16, 2021

Cleanup tasks once the rubble stops bouncing:

  1. Prune fields out of AuxCall -- I put a lot of stuff in for transition purposes that is redundant w/ the ABI, the ABI is better.
  2. Rationalize use of LocalsOffset() -- that's the thing that is non-zero if there's a link register, and getting that right has been a source of bugs and builder-breakage.
  3. Restructure "parameters" to not be fields?

Somewhere in here I think we need to sync w/ the generics effort -- I am a little curious what happens to things like "Size" and "Offset" down the road. Conceivably, they will become expressions, that will be a minor party. If first effort is all templates, that won't happen immediately, but we might want to plan ahead.

gopherbot pushed a commit that referenced this issue Mar 16, 2021
…utos

Repair of CL 300749.

ALSO:
found evidence that stack maps for bodyless methods are wrong.
gofmt in test/abi
removed never-executed code in types/size.go

Updates #44816.
Updates #40724.

Change-Id: Ifeb5fee60f60e7c7b58ee0457f58a3265d6cf3f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/302071
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 16, 2021
Code generation for open defers failed to account for
presence of method receiver and thus was OFF BY ONE.

Fixes #45062.
Updates #44816.
Updates #40724.

Change-Id: Ia90ea8fd0f7d823e1f757c406f9127136c2ffdd2
Reviewed-on: https://go-review.googlesource.com/c/go/+/302249
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
auxten added a commit to gopher-lang/gopher that referenced this issue Mar 19, 2021
* cmd/compile: graceful handle error in noder LoadPackage

When syntax.Parse returns error, noder.file will be nil. Currently, we
continue accessing it regardlessly and depend on gc.hidePanic to hide
the panic from user.

Instead, we should gracefully handle the error in LoadPackage, then exit
earlier if any error occurred.

Updates #43311

Change-Id: I0a108ef360bd4f0cc9f481071b8967355e1513af
Reviewed-on: https://go-review.googlesource.com/c/go/+/294030
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: do not set type for OPACK

That's an invalid operation and depend on gc.hidePanic to report error.

Updates #43311

Change-Id: I78d615c40ab1e7887f612491e215c1c2bb758ef6
Reviewed-on: https://go-review.googlesource.com/c/go/+/294031
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: skip diag error in checkassign if one was emitted

While at it, also remove checkassignlist, which is not used.

For #43311

Change-Id: Ie7ed81f68080d8881fca6035da64a9755f2cb555
Reviewed-on: https://go-review.googlesource.com/c/go/+/294032
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: do not set type for OTYPE

That's an invalid operation and depend on gc.hidePanic to report error.

Updates #43311

Change-Id: Ib0761dcf4d9d2a23c41de7eff0376677a90b942e
Reviewed-on: https://go-review.googlesource.com/c/go/+/294033
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: do not add invalid key to constSet

After CL 272654, the compiler now use go/constant.Value to represent
constant nodes. That makes ir.ConstantValue requires node type to
correctly return value for untyped int node. But untyped int node can
have nil type after typechecked, e.g: using int value as key for
map[string]int, that makes the compiler crashes.

To fix it, just don't add the invalid key to constSet, since when
it's not important to report duplicated keys when they aren't valid.

For #43311
Fixes #44432

Change-Id: I44d8f2b95f5cb339e77e8a705a94bcb16e62beb9
Reviewed-on: https://go-review.googlesource.com/c/go/+/294034
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: make check2 gracefully exit if it reported errors

Otherwise, if -d=panic was set, check2 will treat already reported
error as internal compiler error.

For #43311
Fixes #44445

Change-Id: I5dbe06334666df21d9107396b9dcfdd905aa1e44
Reviewed-on: https://go-review.googlesource.com/c/go/+/294850
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: register abi, morestack work and mole whacking

Morestack works for non-pointer register parameters

Within a function body, pointer-typed parameters are correctly
tracked.

Results still not hooked up.

For #40724.

Change-Id: Icaee0b51d0da54af983662d945d939b756088746
Reviewed-on: https://go-review.googlesource.com/c/go/+/294410
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: fix pointer maps for morestack

Verified with test and with single step watching changes to register
values across morestack calls, after reload.

Also added stack-growth test with pointer parameters of varying lifetime.

For #40724.

Change-Id: Idb5fe27786ac5c6665a734d41e68d3d39de2f4da
Reviewed-on: https://go-review.googlesource.com/c/go/+/294429
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>

* runtime: remove a duplicated testcase of TestPallocDataFindScavengeCa…

Change-Id: Ib44729ffb5d4d7b84114dcf028b7e0418c9d5035
GitHub-Last-Rev: 13f59a650aa424b4852ee4a803eebc793dbbfc15
GitHub-Pull-Request: golang/go#42434
Reviewed-on: https://go-review.googlesource.com/c/go/+/268021
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Trust: Martin Möhrmann <moehrmann@google.com>

* cmd/go: reject 'go list -m MOD@patch' when no existing version of MOD is required

Noticed while debugging failing tests for #36460.

Fixes #44788

Change-Id: Ic2cf511d871b29284f7372920f6f7d452825dd63
Reviewed-on: https://go-review.googlesource.com/c/go/+/298651
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* runtime: remove GODEBUG=scavenge mode

Change-Id: Ic4c7b5086303c7faa49f4cbf6738e66d5de35c7e
Reviewed-on: https://go-review.googlesource.com/c/go/+/282012
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>

* cmd/go: ensure that the test subprocess always times out in TestScript/test_write_profiles_on_timeout

This test verifies the behavior of a test that fails due to timing
out. However, the test to be timed out was only sleeping for 1s before
returning successfully. That is empirically not always long enough for
the test process itself to detect the timeout and terminate.

We could replace the sleep with a select{}, but that would assume that
the deadlock detector does not terminate a test that reaches that
state (true today, but not necessarily so).

We could replace the arbitrarily sleep with an arbitrarily longer
sleep, but that's, well, arbitrary.

Instead, have the test sleep in an unbounded loop to ensure that it
always continues to run until the timeout is detected, and check the
test output to ensure that it actually reached the timeout path.

Fixes #32983

Change-Id: Ie7f210b36ef0cc0a4db473f780e15a3d6def8bda
Reviewed-on: https://go-review.googlesource.com/c/go/+/289889
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

* cmd/internal: Add 6 args to ppc64 optab

This is a preparatory patch to support 6 arg opcodes on POWER10,
and simplify 5 arg opcode processing (e.g RLWNM and similar).

This expands the optab structure, and renames a4 arguments to a6.
No actual change in functionality is made.

Change-Id: I785e4177778e4bf1326cf8e46e8aeaaa0e4d406b
Reviewed-on: https://go-review.googlesource.com/c/go/+/295031
Run-TryBot: Lynn Boger <laboger@linux.vnet.ibm.com>
Run-TryBot: Carlos Eduardo Seo <carlos.seo@linaro.org>
Reviewed-by: Lynn Boger <laboger@linux.vnet.ibm.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: Keith Randall <khr@golang.org>

* cmd/compile: implement simple register results

at least for ints and strings

includes simple test

For #40724.

Change-Id: Ib8484e5b957b08f961574a67cfd93d3d26551558
Reviewed-on: https://go-review.googlesource.com/c/go/+/295309
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* syscall: treat proc thread attribute lists as unsafe.Pointers

It turns out that the proc thread update function doesn't actually
allocate new memory for its arguments and instead just copies the
pointer values into the preallocated memory. Since we were allocating
that memory as []byte, the garbage collector didn't scan it for pointers
to Go allocations and freed them. We _could_ fix this by requiring that
all users of this use runtime.KeepAlive for everything they pass to the
update function, but that seems harder than necessary. Instead, we can
just do the allocation as []unsafe.Pointer, which means the GC can
operate as intended and not free these from beneath our feet. In order
to ensure this remains true, we also add a test for this.

Fixes #44662.

Change-Id: Ib392ba8ceacacec94b11379919c8179841cba29f
Reviewed-on: https://go-review.googlesource.com/c/go/+/297389
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Trust: Alex Brainman <alex.brainman@gmail.com>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

* cmd/compile: implement too-big-to-SSA struct passing in registers

Added a test that exercises named results

Change-Id: Ie228b68f4f846266595a95e0f65a6e4b8bf79635
Reviewed-on: https://go-review.googlesource.com/c/go/+/297029
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: remove I-saw-a-register-pragma chatter

It is not multithreaded-compilation-safe, and also seems
to cause problems on the noopt-builder.

Change-Id: I52dbcd507d256990f1ec7c8040ec7b76595aae4f
Reviewed-on: https://go-review.googlesource.com/c/go/+/298850
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile/internal/syntax: print type parameters and type lists

types2 uses the syntax printer to print expressions (for tracing
or error messages), so we need to (at least) print type lists in
interfaces.

While at it, also implement the printing of type parameter lists.

Fixes #44766.

Change-Id: I36a4a7152d9bef7251af264b5c7890aca88d8dc3
Reviewed-on: https://go-review.googlesource.com/c/go/+/298549
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

* cmd/go: clarify errors for commands run outside a module

The new error message tells the user what was wrong (no go.mod found)
and directs them to 'go help modules', which links to tutorials.

Fixes #44745

Change-Id: I98f31fec4a8757eb1792b45491519da4c552cb0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/298650
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

* cmd/go: don't report missing std import errors for tidy and vendor

'go mod tidy' and 'go mod vendor' normally report errors when a
package can't be imported, even if the import appears in a file that
wouldn't be compiled by the current version of Go. These errors are
common for packages introduced in higher versions of Go, like "embed"
in 1.16.

This change causes 'go mod tidy' and 'go mod vendor' to ignore
missing package errors if the import path appears to come from the
standard library because it lacks a dot in the first path element.

Fixes #44557
Updates #27063

Change-Id: I61d6443e77ab95fd8c0d1514f57ef4c8885a77cc
Reviewed-on: https://go-review.googlesource.com/c/go/+/298749
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* cmd/compile: add register abi tests

Change-Id: I4b2b62a8eb1c4bf47f552214127d4ed5710af196
Reviewed-on: https://go-review.googlesource.com/c/go/+/297030
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: tweak offset-generator to elide more +0 offsets

this caused a problem in write barrier code when a spurious
zero-offset prevented a write barrier elision.

removed cache after instrumenting it and discovering
zero safe hits (one value must dominate the other, else
unsafe).

Change-Id: I42dfdb4d38ebfe158b13e766a7fabfc514d773f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/297349
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: return (and receive) medium-large results

includes three tests

Change-Id: I33ac0cfe35085d4b6ad2775abcaa3d7d6527b49f
Reviewed-on: https://go-review.googlesource.com/c/go/+/297031
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: remove types2.(*Selection).TArgs(), now that instance bug seems fixed

Previously, we would sometimes see an internal (*instance) type for a
receiver of a types2 method, which was a bug. To deal with that, we put
in an extra (*Selection).TArgs() method. However, that (*instance) type
is no longer showing up for receivers, so we can remove the types2
method we added and do the work with existing types2 API methods.

Change-Id: I03e68f5bbaaf82fe706b6efecbb02e951bbd3cd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/298869
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Trust: Dan Scales <danscales@google.com>

* encoding/json: marshal maps using reflect.Value.MapRange

Map serialization using reflect.Value.MapIndex cannot retrieve
map keys that contain a NaN, resulting in a panic.
Switch the implementation to use the reflect.Value.MapRange method
instead, which iterates over all map entries regardless of whether
they are directly retrievable.

Note that according to RFC 8259, section 4, a JSON object should
have unique names, but does not forbid the occurrence of duplicate names.

Fixes #43207

Change-Id: If4bc55229b1f64b8ca4b0fed37549725efdace39
Reviewed-on: https://go-review.googlesource.com/c/go/+/278632
Trust: Meng Zhuo <mzh@golangcn.org>
Trust: Joe Tsai <thebrokentoaster@gmail.com>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Meng Zhuo <mzh@golangcn.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>

* cmd/go: update error expectations in TestScript/mod_install_pkg_version

This test was missed in CL 298650, and not caught by TryBots because
it is skipped it short mode (and we forgot to add longtest TryBots on
that CL).

Updates #44745

Change-Id: I696d01307dabf351b0e4735db0644f4e09c8e369
Reviewed-on: https://go-review.googlesource.com/c/go/+/298794
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* docs: fix case of GitHub

Change `Github` to `GitHub`

Change-Id: I514e8dc9a19182fcf9fcf5bc1b5fbff253c1a947
GitHub-Last-Rev: 7124c7058e0c7ff19dc8440fa79271eb6cfdaea9
GitHub-Pull-Request: golang/go#44260
Reviewed-on: https://go-review.googlesource.com/c/go/+/291950
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* cmd/go: update PWD variable for 'go generate'

Most subprocess invocations in the go command use base.AppendPWD to
append an accurate value of PWD to the command's environment, which can
speed up calls like os.Getwd and also help to provide less-confusing
output from scripts. Update `go generate` to do so.

Fixes #43862

Change-Id: I3b756f1532b2d922f7d74fd86414d5567a0122c0
GitHub-Last-Rev: 3ec8da265a2777d1dcbcea00f107b8f5905f3640
GitHub-Pull-Request: golang/go#43940
Reviewed-on: https://go-review.googlesource.com/c/go/+/287152
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Baokun Lee <bk@golangcn.org>

* math/big: remove bounds checks for shrVU_g inner loop

Make explicit a shrVU_g precondition.
Replace i with i+1 throughout the loop.
The resulting loop is functionally identical,
but the compiler can do better BCE without the i-1 slice offset.

Benchmarks results on amd64 with -tags=math_big_pure_go.

name                          old time/op  new time/op  delta
NonZeroShifts/1/shrVU-8       4.55ns ± 2%  4.45ns ± 3%   -2.27%  (p=0.000 n=28+30)
NonZeroShifts/1/shlVU-8       4.07ns ± 1%  4.13ns ± 4%   +1.55%  (p=0.000 n=26+29)
NonZeroShifts/2/shrVU-8       6.12ns ± 1%  5.55ns ± 1%   -9.30%  (p=0.000 n=28+28)
NonZeroShifts/2/shlVU-8       5.65ns ± 3%  5.70ns ± 2%   +0.92%  (p=0.008 n=30+29)
NonZeroShifts/3/shrVU-8       7.58ns ± 2%  6.79ns ± 2%  -10.46%  (p=0.000 n=28+28)
NonZeroShifts/3/shlVU-8       6.62ns ± 2%  6.69ns ± 1%   +1.07%  (p=0.000 n=29+28)
NonZeroShifts/4/shrVU-8       9.02ns ± 1%  7.79ns ± 2%  -13.59%  (p=0.000 n=27+30)
NonZeroShifts/4/shlVU-8       7.74ns ± 1%  7.82ns ± 1%   +0.92%  (p=0.000 n=26+28)
NonZeroShifts/5/shrVU-8       10.6ns ± 1%   8.9ns ± 3%  -16.31%  (p=0.000 n=25+29)
NonZeroShifts/5/shlVU-8       8.59ns ± 1%  8.68ns ± 1%   +1.13%  (p=0.000 n=27+29)
NonZeroShifts/10/shrVU-8      18.2ns ± 2%  14.4ns ± 1%  -20.96%  (p=0.000 n=27+28)
NonZeroShifts/10/shlVU-8      14.1ns ± 1%  14.1ns ± 1%   +0.46%  (p=0.001 n=26+28)
NonZeroShifts/100/shrVU-8      161ns ± 2%   118ns ± 1%  -26.83%  (p=0.000 n=29+30)
NonZeroShifts/100/shlVU-8      119ns ± 2%   120ns ± 2%   +0.92%  (p=0.000 n=29+29)
NonZeroShifts/1000/shrVU-8    1.54µs ± 1%  1.10µs ± 1%  -28.63%  (p=0.000 n=29+29)
NonZeroShifts/1000/shlVU-8    1.10µs ± 1%  1.10µs ± 2%     ~     (p=0.701 n=28+29)
NonZeroShifts/10000/shrVU-8   15.3µs ± 2%  10.9µs ± 1%  -28.68%  (p=0.000 n=28+28)
NonZeroShifts/10000/shlVU-8   10.9µs ± 2%  10.9µs ± 2%   -0.57%  (p=0.003 n=26+29)
NonZeroShifts/100000/shrVU-8   154µs ± 1%   111µs ± 2%  -28.04%  (p=0.000 n=27+28)
NonZeroShifts/100000/shlVU-8   113µs ± 2%   113µs ± 2%     ~     (p=0.790 n=30+30)

Change-Id: Ib6a621ee7c88b27f0f18121fb2cba3606c40c9b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/297049
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

* cmd/link: add relocs type for mips64x

The race detector of llvm adds four reloc types even with -fPIC

elf.R_MIPS_CALL16
elf.R_MIPS_GPREL32
elf.R_MIPS_64
elf.R_MIPS_GOT_DISP

Change-Id: If73119dcba14ef74395273eb680f52a0aa853217
Reviewed-on: https://go-review.googlesource.com/c/go/+/270297
Trust: Meng Zhuo <mzh@golangcn.org>
Run-TryBot: Meng Zhuo <mzh@golangcn.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* net/http/httptrace: fix doc typo

Change-Id: I919d9c3968c0fcd33774e714f22182504790bd01
Reviewed-on: https://go-review.googlesource.com/c/go/+/284143
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Trust: Daniel Martí <mvdan@mvdan.cc>

* testing: add TB.Setenv

Add a new method TB.Setenv that'll set environment variables
only for the isolated lifetime of the test, and will clean up
and unset these variables when the test ends.
This method disables the test or benchmark from running in
parallel.

Fixes #41260

Change-Id: I0a18f094ec1c6ec3157b4b12993ea3075e2e9867
GitHub-Last-Rev: 0ca12fa565318f350b927e2ef94f3b4f792c75c2
GitHub-Pull-Request: golang/go#41857
Reviewed-on: https://go-review.googlesource.com/c/go/+/260577
Trust: Daniel Martí <mvdan@mvdan.cc>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: roger peppe <rogpeppe@gmail.com>

* cmd/go/internal/modfetch: detect and recover from missing ziphash file

Previously, if an extracted module directory existed in the module
cache, but the corresponding ziphash file did not, if the sum was
missing from go.sum, we would not verify the sum. This caused 'go get'
not to write missing sums. 'go build' in readonly mode (now the
default) checks for missing sums and doesn't attempt to fetch modules
that can't be verified against go.sum.

With this change, when requesting the module directory with
modfetch.DownloadDir, if the ziphash file is missing, the go command
will re-hash the zip without downloading or re-extracting it again.

Note that the go command creates the ziphash file before the module
directory, but another program could remove it separately, and it
might not be present after a crash.

Fixes #44749

Change-Id: I64551e048a3ba17d069de1ec123d5b8b2757543c
Reviewed-on: https://go-review.googlesource.com/c/go/+/298352
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

* reflect: use global variables for register count

This change switches reflect to use global variables for ABI-related
register counts instead of internal/abi constants. The advantage of
doing so is that we can make the internal/abi constants non-zero and
enable the runtime register argument spiller/unspiller even if they're
not used. It's basically turning two things we need to flip when we
switch to the register ABI into one.

It also paves the way for testing the reflect register ABI path
independently, because now we can switch the global variables at will
and run the register-assignment algorithm in tests without having the
rest of the runtime be broken.

Change-Id: Ie23629a37a5c80aeb24909d4bd9eacbd3f0c06d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/293149
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* reflect: include the alignment of zero-sized types in stack offsets

This change modifies the reflect ABI assignment algorithm to catch
zero-sized types at the top level of each argument and faux-stack-assign
them. It doesn't actually generate an ABI step, which is unnecessary,
but it ensures that the offsets of further stack-assigned arguments are
aligned to the alignment of that zero-sized argument.

This change is necessary to have the register ABI assignment algorithm
gracefully degrade to ABI0 when no registers are present in the ABI.

Fixes #44377.

Change-Id: Ia95571688a61259302bb3c6d5fb33fbb6b5e8db8
Reviewed-on: https://go-review.googlesource.com/c/go/+/293789
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: Than McIntosh <thanm@google.com>

* cmd/go: require a module root in 'go list -m' with an unversioned path

Fixes #44803

Change-Id: Ie6ee2e3bca1809c91ecedec75d2c6620da914b29
Reviewed-on: https://go-review.googlesource.com/c/go/+/298752
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

* runtime: update paniclk ordering

Now that allglock is no longer taken in throw, paniclk can move to the
bottom of the lock order where it belongs.

There is no fundamental reason that we really need to skip checks on
paniclk in lockWithRank (despite the recursive throws that could be
caused by lock rank checking, startpanic_m would still allow the crash
to complete). However, the partial order of lockRankPanic should be
every single lock that may be held before a throw, nil dereference,
out-of-bounds access, which our partial order doesn't cover.

Updates #42669

Change-Id: Ic3efaea873dc2dd9fd5b0d6ccdd5319730b29a22
Reviewed-on: https://go-review.googlesource.com/c/go/+/270862
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

* text/template: wrap errors returned by template functions instead of stringifying them

Fixes #34201

Change-Id: Ic2e2967e4b01167345cf38bd006cabb206a64377
GitHub-Last-Rev: 5d0c4856550614484a8dbfb68c37aa1abcfcc529
GitHub-Pull-Request: golang/go#42398
Reviewed-on: https://go-review.googlesource.com/c/go/+/267838
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Trust: Daniel Martí <mvdan@mvdan.cc>
Trust: Pontus Leitzler <leitzler@gmail.com>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Go Bot <gobot@golang.org>

* cmd/compile: only check return for valid functions

CheckReturn uses fn.Type() unconditionally, so for invalid function,
fn.Type() will be nil, causes the compiler crashes.

Updates #43311

Change-Id: I4420dd296c72ea83986b38fbf2c7f51fa59757c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/298709
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: don't expand invalid embedded interface

The invalid interface type will be reported already, so don't expand
that invalid one, which causes the compiler crashes.

Updates #43311

Change-Id: Ic335cfa74f0b9fcfd0929dc5fd31d9156a8f5f5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/298710
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: do not set type for OTYPESW

Same as CL 294031, but for OTYPESW.

Updates #43311

Change-Id: I996f5938835baff1d830c17ed75652315106bdfd
Reviewed-on: https://go-review.googlesource.com/c/go/+/298712
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: do not report error for invalid constant

Invalid constant was already reported by noder, so don't re-check in
typecheck, which lead to compiler crashing.

Updates #43311

Change-Id: I48e2f540601cef725c1ff628c066ed15d848e771
Reviewed-on: https://go-review.googlesource.com/c/go/+/298713
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: fix wrong condition in tcShift

CL 279442 refactored typecheck arithmetic operators, but using wrong
condition for checking invalid rhs.

Updates #43311

Change-Id: I7a03a5535b82ac4ea4806725776b0a4f7af1b79a
Reviewed-on: https://go-review.googlesource.com/c/go/+/298714
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: do not set ONAME type when evaluated in type context

Updates #43311

Change-Id: I26e397d071b434256dab0cc7fff9d134b80bd6e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/298711
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* test: enable "-d=panic" by default for errorcheck*

Fixes #43311

Change-Id: I134d6c0524c198998a3c093dd3a8144052e8f7a9
Reviewed-on: https://go-review.googlesource.com/c/go/+/298715
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/go/internal/modfetch/codehost: report git errors more accurately

Previously, if you attempted to fetch a private repository, or your
Git/curl client failed for an unknown reason, codehost would return an
UnknownRevisionError, which reported that a given revision in go.mod
was "unknown". This is confusing to many users who can go look in
their browser for example and see that the commit-ish exists.

Instead check whether "git ls-remote" exited with an error, and if so,
return that instead of the UnknownRevision message.

Fixes #42751.

Change-Id: I0dbded878b2818280e61126a4493767d719ad577
Reviewed-on: https://go-review.googlesource.com/c/go/+/297950
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* runtime: encapsulate access to allgs

Correctly accessing allgs is a bit hairy. Some paths need to lock
allglock, some don't. Those that don't are safest using atomicAllG, but
usage is not consistent.

Rather than doing this ad-hoc, move all access* through forEachG /
forEachGRace, the locking and atomic versions, respectively. This will
make it easier to ensure safe access.

* markroot is the only exception, as it has a far-removed guarantee of
safe access via an atomic load of allglen far before actual use.

Change-Id: Ie1c7a8243e155ae2b4bc3143577380c695680e89
Reviewed-on: https://go-review.googlesource.com/c/go/+/279994
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

* cmd/compile: match Aux and AuxInt explicitly in store combining rule

CL 280456 introduced a new store combining rule. On the LHS some
of the Aux and AuxInt of the stores are not specified, therefore
ignored during the matching. The rule is only correct if they
match. This CL adds explict match.

TODO: maybe we want the rule matcher require Aux/AuxInt to be
always specified on the LHS (using _ to explicitly ignore)? Or
maybe we want it to match the zero value if not specified? The
current approach is error-prone.

Fixes #44823.

Change-Id: Ic12b4a0de63117f2f070039737f0c905f28561bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/299289
Trust: Cherry Zhang <cherryyz@google.com>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>

* cmd/compile: use getcallerpc for racefuncentry

Currently, when instrumenting for the race detector, the compiler
inserts racefuncentry/racefuncentryfp at the entry of instrumented
functions. racefuncentry takes the caller's PC. On AMD64, we synthesize
a node which points to -8(FP) which is where the return address is
stored. Later this node turns to a special Arg in SSA that is not
really an argument. This causes problems in the new ABI work so that
special node has to be special-cased.

This CL changes the special node to a call to getcallerpc, which lowers
to an intrinsic in SSA. This also unifies AMD64 code path and LR machine
code path, as getcallerpc works on all platforms.

Change-Id: I1377e140b91e0473cfcadfda221f26870c1b124d
Reviewed-on: https://go-review.googlesource.com/c/go/+/297929
Trust: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>

* cmd/compile: use getcallersp for gorecover "fp" arg

Currently, the compiler synthesize a special ".fp" node, which
points to the FP of the current frame, be to used to call
gorecover. Later that node turns to an Arg in SSA that is not
really an arg, causing problems for the new ABI work which changes
the handling of Args, so we have to special-case that node.

This CL changes the compiler to get the FP by using getcallersp,
which is an intrinsic in SSA and works on all platforms. As we
need the FP, not the caller SP, one drawback is that we have to
add FixedFrameSize for LR machines. But it does allow us to remove
that special node.

Change-Id: Ie721d51efca8116c9d23cc4f79738fffcf847df8
Reviewed-on: https://go-review.googlesource.com/c/go/+/297930
Trust: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>

* cmd/internal/goobj: regenerate builtin list

Change-Id: Ib8cb5f90e084838f00ecba78641bbb5d48ecac32
Reviewed-on: https://go-review.googlesource.com/c/go/+/297931
Trust: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>

* runtime: remove racefuncenterfp

No longer needed with previous CL.

Change-Id: I7c01f9e0e34ecb9553ef1b3d662f33419fd3a244
Reviewed-on: https://go-review.googlesource.com/c/go/+/297932
Trust: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>

* cmd/internal/goobj: store relocation type as uint16

Currently, relocation type is stored as uint8 in object files, as
Go relocations do not exceed 255. In the linker, however, it is
used as a 16-bit type, because external relocations can exceed
255. The linker has to store the extra byte in a side table. This
complicates many things.

Just store it as uint16 in object files. This simplifies things,
with a small cost of increasing the object file sizes.

               before      after
hello.o         1672        1678
runtime.a    7927784     8056194

Change-Id: I313cf44ad0b8b3b76e35055ae55d911ff35e3158
Reviewed-on: https://go-review.googlesource.com/c/go/+/268477
Trust: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>

* reflect: add VisibleFields function

When writing code that reflects over a struct type, it's a common requirement to know the full set of struct fields, including fields available due to embedding of anonymous members while excluding fields that are erased because they're at the same level as another field with the same name.

The logic to do this is not that complex, but it's a little subtle and easy to get wrong.

This CL adds a new `VisibleFields` function to the reflect package that returns the full set of effective fields that apply in a given struct type.

Performance isn't a prime consideration, as it's common to cache results by type.

Fixes #42782

Change-Id: I7f1af76cecff9b8a2490f17eec058826e396f660
Reviewed-on: https://go-review.googlesource.com/c/go/+/281233
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Daniel Martí <mvdan@mvdan.cc>

* cmd/internal/objabi: use a separate bit to mark weak relocation

Instead of using two relocation types R_XXX and R_WEAKXXX, use a
separate bit, R_WEAK, to mark weak relocations. This makes it
easier to add more weak relocation types.

Change-Id: Iec4195c2aefa65f59e464c83018246e17cd08173
Reviewed-on: https://go-review.googlesource.com/c/go/+/268478
Trust: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>

* math/big: add shrVU and shlVU benchmarks

Change-Id: Id67d6ac856bd9271de99c3381bde910aa0c166e0
Reviewed-on: https://go-review.googlesource.com/c/go/+/296011
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

* cmd/compile: rename internal-abi.md to abi-internal.md

Allows muscle-memoried tab completion of cmd/compile/internal/...
paths to work again.

Change-Id: Ib54a5f2cc9fabcb876c2e62635828ab28b565501
Reviewed-on: https://go-review.googlesource.com/c/go/+/299530
Reviewed-by: Austin Clements <austin@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>

* cmd/compile: improve IsNonNil rule on riscv64

IsNonNil is readily implemented using SNEZ on riscv64, removing over 8,000
instructions from the go binary. Other rules will improve on this sequence,
however in this case it makes sense to use a direct simplification.

Change-Id: Ib4068599532398afcd05f51d160673ef5fb5e5a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/299230
Trust: Joel Sing <joel@sing.id.au>
Reviewed-by: Michael Munday <mike.munday@lowrisc.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* encoding/binary: limit bytes read by Uvarint to <= 10

Limits the number of bytes that can be consumed by Uvarint
to MaxVarintLen64 (10) to avoid wasted computations.
With this change, if Uvarint reads more than MaxVarintLen64
bytes, it'll return the erroring byte count of n=-(MaxVarintLen64+1)
which is -11, as per the function signature.

Updated some tests to reflect the new change in expectations of n
when the number of bytes to be read exceeds the limits..

Fixes #41185

Change-Id: Ie346457b1ddb0214b60c72e81128e24d604d083d
Reviewed-on: https://go-review.googlesource.com/c/go/+/299531
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>

* cmd/compile: minor doc improvements

These are left over from comments I failed to leave on CL 249463;
apparently I never hit "Reply".

Change-Id: Ia3f8a900703c347f8f98581ec1ac172c0f72cd9e
Reviewed-on: https://go-review.googlesource.com/c/go/+/299589
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

* cmd/link/internal/ld: fix typo in a comment

Change-Id: I6d0fafd38f0ad9392f163a9d8cd94d103bf2a059
Reviewed-on: https://go-review.googlesource.com/c/go/+/299669
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: fix width not calculated for imported type

The compiler currently has problem that some imported type is missing
size calculation. The problem is not triggered until CL 283313 merged,
due to the compiler can compile the functions immediately when it sees
them, so during SSA generation, size calculation is still ok.

CL 283313 makes the compiler always push functions to compile queue,
then drain from it for compiling function. During this process, the
types calculation size is disabled, so calculating size during SSA now
make the compiler crashes.

To fix this, we can just always calculate type size during typechecking,
when importing type from other packages.

Fixes #44732

Change-Id: I8d00ea0b5aadd432154908280e55d85c75f3ce92
Reviewed-on: https://go-review.googlesource.com/c/go/+/299689
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

* cmd/link: use testing.T.TempDir in tests

Change-Id: I6fc8c9ee6d2246bfd874eb58b411e34ddbeaf723
Reviewed-on: https://go-review.googlesource.com/c/go/+/299670
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>

* os/signal: remove comments about SA_RESTART

It's no longer necessary for non-Go signal handlers to use SA_RESTART.

For #20400
Fixes #44761

Change-Id: Ie3c7fecc631a4a2822331754296ea09b308e1391
Reviewed-on: https://go-review.googlesource.com/c/go/+/298269
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* cmd/vet: bring in sigchanyzer to report unbuffered channels to signal.Notify

Brings in the static analyzer "sigchanyzer", that we created at
Orijtech, Inc, and already submitted in CL 274352, as

    golang.org/x/tools/go/analysis/passes/sigchanyzer

and add it to cmd/vet as one of the passes.

Fixes #9399

Change-Id: I83708b8ea5ca8ede5ee31efab55cbce7419434ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/299532
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Trust: Bryan C. Mills <bcmills@google.com>

* cmd/compile: remove 4-byte alignment requirement of stack slot on arm

This CL applies CL 267999 to arm.

Updates #42385

Change-Id: Iad82aafcb7b0a5a77a4bea32f648320f57a17cdd
Reviewed-on: https://go-review.googlesource.com/c/go/+/297773
Reviewed-by: eric fang <eric.fang@arm.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: eric fang <eric.fang@arm.com>
Run-TryBot: eric fang <eric.fang@arm.com>

* runtime: document netpollclose

Document that network poller implementations need to define
netpollclose.

Change-Id: Idc73dea7cfd503d4de7e1d95902f0f102cf5ed8f
Reviewed-on: https://go-review.googlesource.com/c/go/+/297809
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

* cmd/compile: make ValAndOff.{Val,Off} return an int32

The ValAndOff type is a 64bit integer holding a 32bit value and a
32bit offset in each half, but for historical reasons its Val and Off
methods returned an int64. This was convenient when AuxInt was always
an int64, but now that AuxInts are typed we can return int32 from Val
and Off and get rid of a several casts and now unnecessary range
checks.

This change:

- changes the Val and Off methods to return an int32 (from int64);
- adds Val64 and Off64 methods for convenience in the few remaining
  places (in the ssa.go files) where Val and Off are stored in int64
  fields;
- deletes makeValAndOff64, renames makeValAndOff32 to makeValAndOff
- deletes a few ValAndOff methods that are now unused;
- removes several validOff/validValAndOff check that will always
  return true.

Passes:

  GOARCH=amd64 gotip build -toolexec 'toolstash -cmp' -a std
  GOARCH=386 gotip build -toolexec 'toolstash -cmp' -a std
  GOARCH=s390x gotip build -toolexec 'toolstash -cmp' -a std

(the three GOARCHs with SSA rules files impacted by the change).

Change-Id: I2abbbf42188c798631b94d3a55ca44256f140be7
Reviewed-on: https://go-review.googlesource.com/c/go/+/299149
Trust: Alberto Donizetti <alb.donizetti@gmail.com>
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

* cmd/link/internal/ld: deflake TestWindowsIssue36495

Over a dozen of the ld tests were missing closes. That was less
obvious before CL 299670 started using T.TempDir instead, which fails
a test when the tempdir can't be cleaned up (as it can't on Windows
when things are still open), insteading of leaving tempdirs around on
disk after the test.

Most of the missing closes were fixed in CL 299670, but the builders
helpfully pointed out that I missed at least this one.

Change-Id: I35f695bb7cbfba31e16311c5af965c148f9d7943
Reviewed-on: https://go-review.googlesource.com/c/go/+/299929
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Trust: Dan Scales <danscales@google.com>
Trust: Alex Brainman <alex.brainman@gmail.com>

* net: use io.Discard in TestSendfileOnWriteTimeoutExceeded

Replace ioutil.Discard which is deprecated as of Go 1.16.

This was already done in CL 263142 but accidentially re-introduced in
CL 285914.

Change-Id: Ife0944d416294b1ba7c8f6b602aa68a3b9213c50
Reviewed-on: https://go-review.googlesource.com/c/go/+/299989
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>

* cmd/compile:  get instantiated generic types working with interfaces

Get instantiatiated generic types working with interfaces, including
typechecking assignments to interfaces and instantiating all the methods
properly. To get it all working, this change includes:

 - Add support for substituting in interfaces in subster.typ()

 - Fill in the info for the methods for all instantiated generic types,
   so those methods will be available for later typechecking (by the old
   typechecker) when assigning an instantiated generic type to an
   interface. We also want those methods available so we have the list
   when we want to instantiate all methods of an instantiated type. We
   have both for instantiated types encountered during the initial noder
   phase, and for instantiated types created during stenciling of a
   function/method.

 - When we first create a fully-instantiated generic type (whether
   during initial noder2 pass or while instantiating a method/function),
   add it to a list so that all of its methods will also be
   instantiated. This is needed so that an instantiated type can be
   assigned to an interface.

 - Properly substitute type names in the names of instantiated methods.

 - New accessor methods for types.Type.RParam.

 - To deal with generic types which are empty structs (or just don't use
   their type params anywhere), we want to set HasTParam if a named type
   has any type params that are not fully instantiated, even if the
   type param is not used in the type.

 - In subst.typ() and elsewhere, always set sym.Def for a new forwarding
   type we are creating, so we always create a single unique type for
   each generic type instantiation. This handles recursion within a
   type, and also recursive relationships across many types or methods.
   We remove the seen[] hashtable, which was serving the same purpose,
   but for subst.typ() only. We now handle all kinds of recursive types.

 - We don't seem to need to force types.CheckSize() on
   created/substituted generic types anymore, so commented out for now.

 - Add an RParams accessor to types2.Signature, and also a new
   exported types2.AsSignature() function.

Change-Id: If6c5dd98427b20bfe9de3379cc16f83df9c9b632
Reviewed-on: https://go-review.googlesource.com/c/go/+/298449
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Dan Scales <danscales@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>

* cmd/compile: add debugging mode for import/export

Just add a simple magic number with each op, to detect when
the reader gets desynchronized from the writer.

Change-Id: Iac7dab7f465b0021b1d7ae31c8f8a353ac3663a2
Reviewed-on: https://go-review.googlesource.com/c/go/+/299769
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>

* runtime: add pollDesc partial edges

gscan is taken during stack growth, which may occur while pollDesc is
held.

mallocgc may also be called while pollDesc is held. mallocgc may take
mheap or mheapSpecial. The former exists, but is out of order; the
latter is missing.

Fixes #44881

Change-Id: Ie25935d9d433e813c11a528ee47255b317a09f41
Reviewed-on: https://go-review.googlesource.com/c/go/+/300009
Trust: Michael Pratt <mpratt@google.com>
Trust: Dan Scales <danscales@google.com>
Reviewed-by: Dan Scales <danscales@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>

* cmd/compile: detect duplicate importing earlier

Change-Id: I05ba944e189a884b727e40a9526d212612c3e923
Reviewed-on: https://go-review.googlesource.com/c/go/+/299770
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>

* cmd/compile: remove skipping of implicit operations during export

We'll need to attach types to these operations, so we need to
represent them in the import/export data.

Some of the operations use a selector indicating a different package,
so we need to provide an option to encode the package of a selector.
The default selector() function can't encode that extra information,
as selector's exact encoding is used by go/types.

Change-Id: I4c110fe347b3d915f88a722834bc4058baea7854
Reviewed-on: https://go-review.googlesource.com/c/go/+/299771
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>

* cmd/compile: fix OpArg decomposer for registers in expandCalls

Includes test taken from
https://github.com/golang/go/issues/44816#issuecomment-791618179
and improved debugging output.

Updates #44816

Change-Id: I94aeb9c5255f175fe80727be29d218bad54bf7ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/299389
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: fix broken type+offset calc for register args

Includes more enhancements to debugging output.

Updates #44816.

Change-Id: I5b21815cf37ed21e7dec6c06f538090f32260203
Reviewed-on: https://go-review.googlesource.com/c/go/+/299409
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: fix confusion in generating SelectN index

Old: return the ABI register index of the result (wrong!)
New: return the index w/in sequence of result registers (right!)

Fixed bug:
genCaller0/genCaller0.go:43:9: internal compiler error: 'Caller0':
   panic during schedule while compiling Caller0:
runtime error: index out of range [10] with length 9

Updates #44816.

Change-Id: I1111e283658a2d6422986ae3d61bd95d1b9bde5e
Reviewed-on: https://go-review.googlesource.com/c/go/+/299549
Trust: David Chase <drchase@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

* cmd/compile: fix failure to communicate between ABIinfo producer&consumer

ABI info producer and consumer had different ideas for register
order for parameters.

Includes a test, includes improvements to debugging output.

Updates #44816.

Change-Id: I4812976f7a6c08d6fc02aac1ec0544b1f141cca6
Reviewed-on: https://go-review.googlesource.com/c/go/+/299570
Trust: David Chase <drchase@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>

* net: don't append a dot to TXT records on Plan 9

TXT records are not domain names, so no need to call absDomainName.
The output now matches the pure Go resolver.

Fixes #44619

Change-Id: I1ebf09152ff5c0446d2e2b4c26671358892d9dc9
Reviewed-on: https://go-review.googlesource.com/c/go/+/296589
Reviewed-by: David du Colombier <0intro@gmail.com>
Run-TryBot: David du Colombier <0intro@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: David du Colombier <0intro@gmail.com>
Trust: Ian Lance Taylor <iant@golang.org>

* runtime, cmd/link/internal/ld: disable memory profiling when data unreachable

If runtime.MemProfile is unreachable, default to not collecting any
memory profiling samples, to save memory on the hash table.

Fixes #42347

Change-Id: I9a4894a5fc77035fe59b1842e1ec77a1182e70c1
Reviewed-on: https://go-review.googlesource.com/c/go/+/299671
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: Keith Randall <khr@golang.org>

* cmd/asm,cmd/compile: support 5 operand RLWNM/RLWMI on ppc64

These instructions are actually 5 argument opcodes as specified
by the ISA.  Prior to this patch, the MB and ME arguments were
merged into a single bitmask operand to workaround the limitations
of the ppc64 assembler backend.

This limitation no longer exists. Thus, we can pass operands for
these opcodes without having to merge the MB and ME arguments in
the assembler frontend or compiler backend.

Likewise, support for 4 operand variants is unchanged.

Change-Id: Ib086774f3581edeaadfd2190d652aaaa8a90daeb
Reviewed-on: https://go-review.googlesource.com/c/go/+/298750
Reviewed-by: Lynn Boger <laboger@linux.vnet.ibm.com>
Reviewed-by: Carlos Eduardo Seo <carlos.seo@linaro.org>
Trust: Carlos Eduardo Seo <carlos.seo@linaro.org>

* go/types: improve the positioning of broken import errors

The heuristic gopls uses to guess error spans can get tripped-up on
certain valid characters in an import path (for example '-').

Update the error for broken imports to capture the full import path
span, so that gopls doesn't need to rely on heuristics.

Change-Id: Ieb8e0dce11933643f701b32271ff5f3477fecaaa
Reviewed-on: https://go-review.googlesource.com/c/go/+/300169
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

* cmd/compile/internal/types2: better error reporting framework (starting point)

Until now, errors which came with additional details (e.g., a declaration
cycle error followed by the list of objects involved in the cycle, one per
line) were reported as an ordinary error followed by "secondary" errors,
with the secondary errors marked as such by having a tab-indented error
message.

This approach often required clients to filter these secondary errors
(as they are not new errors, they are just clarifying a previously
reported error).

This CL introduces a new internal error_ type which permits accumulating
various error information that may then be reported as a single error.

Change-Id: I25b2f094facd37e12737e517f7ef8853d465ff77
Reviewed-on: https://go-review.googlesource.com/c/go/+/296689
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

* cmd/compile/internal/types2: remove concept of finals

This is a 1:1 port of the respective change in go/types
in https://golang.org/cl/299590.

Change-Id: I65ad723f2e21e3d95fc0b94665e0121e31871a48
Reviewed-on: https://go-review.googlesource.com/c/go/+/300250
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

* cmd/go: allow '+' in package import paths in module mode

This change upgrades x/mod to pull in the fix from CL 300149.

Fixes #44776.

Change-Id: I273f41df2abfff76d91315b7f19fce851c8770d8
Reviewed-on: https://go-review.googlesource.com/c/go/+/300176
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* internal/poll: implement a pipe pool for splice() call

In scenarios where splice() is called, splice() is usually called not just once, but many times,
which means that a lot of pipes will be created and destroyed frequently, costing an amount of system resources
and slowing down performance, thus I suggest that we add a pipe pool for reusing pipes.

Benchmark tests:

goos: linux
goarch: amd64
pkg: internal/poll
cpu: AMD EPYC 7K62 48-Core Processor

name                  old time/op    new time/op    delta
SplicePipe-8            1.36µs ± 1%    0.02µs ± 0%   -98.57%  (p=0.001 n=7+7)
SplicePipeParallel-8     747ns ± 4%       4ns ± 0%   -99.41%  (p=0.001 n=7+7)

name                  old alloc/op   new alloc/op   delta
SplicePipe-8             24.0B ± 0%      0.0B       -100.00%  (p=0.001 n=7+7)
SplicePipeParallel-8     24.0B ± 0%      0.0B       -100.00%  (p=0.001 n=7+7)

name                  old allocs/op  new allocs/op  delta
SplicePipe-8              1.00 ± 0%      0.00       -100.00%  (p=0.001 n=7+7)
SplicePipeParallel-8      1.00 ± 0%      0.00       -100.00%  (p=0.001 n=7+7)

Fixes #42740

Change-Id: Idff654b7264342084e089b5ba796c87c380c471b
Reviewed-on: https://go-review.googlesource.com/c/go/+/271537
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>

* testing: fix typo in a comment

Change-Id: I781808327be84113cd55c52bc214b821cd166114
Reviewed-on: https://go-review.googlesource.com/c/go/+/300269
Trust: fannie zhang <Fannie.Zhang@arm.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

* crypto/rand: supports for getrandom syscall in DragonFlyBSD

Since the 5.7 release, DragonFlyBSD supports as well
the getrandom function, the actual stable is 5.8.

Change-Id: I2b8fc468771b10ac12b38ea7e8e5314342de6375
GitHub-Last-Rev: c5c496f41898d58f2c6f3ccc81f754792f49edbe
GitHub-Pull-Request: golang/go#42617
Reviewed-on: https://go-review.googlesource.com/c/go/+/269999
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Trust: Ian Lance Taylor <iant@golang.org>
Trust: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>

* cmd/go: avoid password prompts in TestScript/mod_get_private_vcs

In some cases, this test would prompt for interactive SSH passwords in
order to authenticate to github.com over SSH. Setting GIT_SSH_COMMAND
to /bin/false prevents that, while still provoking the desired Git
failure mode.

Updates #44904.

Change-Id: Idc9fe9f47d2ccb6c8a4ea988b73d9c8c774e4079
Reviewed-on: https://go-review.googlesource.com/c/go/+/300156
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Kevin Burke <kev@inburke.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Kevin Burke <kev@inburke.com>

* runtime: update signature of reflectcall functions

reflectcall tail calls runtime.call{16,32,...} functions, so they
have the same signature as reflectcall. It is important for them
to have the correct arg map, because those functions, as well as
the function being reflectcall'd, could move the stack. When that
happens, its pointer arguments, in particular regArgs, need to be
adjusted. Otherwise it will still point to the old stack, causing
memory corruption.

This only caused failures on the regabi builder because it is the
only place where internal/abi.RegArgs is not a zero-sized type.

May fix #44821.

Change-Id: Iab400ea6b60c52360d0b43a793f6bfe50ca9989b
Reviewed-on: https://go-review.googlesource.com/c/go/+/300154
Trust: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

* cmd/compile: remove ".fp" fake arg

No longer needed with previous CLs.

Change-Id: I9a1c11092a2736c190fa8e8ddfbb913b708957eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/300155
Trust: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>

* runtime/race: update dead link

LLVM changed their main branch name, so this link didn't work anymore.

Change-Id: I4c3a67b26e2bda012071281e29ea3c932c185130
Reviewed-on: https://go-review.googlesource.com/c/go/+/300469
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

* go/types: add missing build tag to api_go1.18_test.go

This file has a go:build comment without a corresponding +build comment.

Change-Id: Id01604242a14d8ead16ffb9aa1b45eef7706956a
Reviewed-on: https://go-review.googlesource.com/c/go/+/300450
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

* cmd/gofmt: fix const association to avoid inaccurate comment

The const parseTypeParams was grouped with printer-related consts in
gofmt.go, implicitly suggesting that it must be kept in sync with
go/format/format.go.

Change-Id: Ia65dc15c27fef2c389f963071252adee32ec6bd6
Reviewed-on: https://go-review.googlesource.com/c/go/+/300451
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

* cmd/compile:  deal with helper generic types that add methods to T

Deal with cases like: 'type P[T any] T' (used to add methods to an
arbitrary type T), In this case, P[T] has kind types.TTYPEPARAM (as does
T itself), but requires more code to substitute than a simple TTYPEPARAM
T. See the comment near the beginning of subster.typ() in stencil.go.

Add new test absdiff.go. This test has a case for complex types (which
I've commented out) that will only work when we deal better with Go
builtins in generic functions (like real and imag).

Remove change in fmt.go for TTYPEPARAMS that is no longer needed (since
all TTYPEPARAMS have a sym) and was sometimes causing an extra prefix
when formatting method names.

Separate out the setting of a TTYPEPARAM bound, since it can reference
the TTYPEPARAM being defined, so must be done separately. Also, we don't
currently (and may not ever) need bounds after types2 typechecking.

Change-Id: Id173057e0c4563b309b95e665e9c1151ead4ba77
Reviewed-on: https://go-review.googlesource.com/c/go/+/300049
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Dan Scales <danscales@google.com>
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

* archive/zip: fix panic in Reader.Open

When operating on a Zip file that contains a file prefixed with "../",
Open(...) would cause a panic in toValidName when attempting to strip
the prefixed path components.

Fixes CVE-2021-27919
Fixes #44916

Change-Id: Ic755d8126cb0897e2cbbdacf572439c38dde7b35
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1004761
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Reviewed-by: Russ Cox <rsc@google.com>
Reviewed-by: Katie Hockman <katiehockman@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/300489
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

* encoding/xml: prevent infinite loop while decoding

This change properly handles a TokenReader which
returns an EOF in the middle of an open XML
element.

Thanks to Sam Whited for reporting this.

Fixes CVE-2021-27918
Fixes #44913

Change-Id: Id02a3f3def4a1b415fa2d9a8e3b373eb6cb0f433
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1004594
Reviewed-by: Russ Cox <rsc@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/300391
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

* flag: panic if flag name begins with - or contains =

Fixes #41792

Change-Id: I9b4aae8a899e3c3ac9532d27932d275cfb1fab48
GitHub-Last-Rev: f06b1e17674bf77bdc2d3e798df4c4379748c8d2
GitHub-Pull-Request: golang/go#42737
Reviewed-on: https://go-review.googlesource.com/c/go/+/271788
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Rob Pike <r@golang.org>

* runtime: check partial lock ranking order

To ease readability we typically keep the partial order lists sorted by
rank. This isn't required for correctness, it just makes it (slightly)
easier to read the lists.

Currently we must notice out-of-order entries during code review, which
is an error-prone process.

Add a test to enforce ordering, and fix the errors that have crept in.
Most of the existing errors were misordered lockRankHchan or
lockRankPollDesc.

While we're here, I've moved the correctness check that the partial
ordering satisfies…
@thanm
Copy link
Contributor Author

@thanm thanm commented Mar 23, 2021

Another test case from the fuzzer that breaks with regiabi (plus pragma).
testcase.zip

Stack trace:

panic: runtime error: index out of range [5] with length 5

goroutine 10 [running]:
cmd/compile/internal/ssagen.(*state).storeParameterRegsToStack(0xc0005d4100, 0xc0005d2120, 0xc0005e4000, 0xc0003c5450, 0xc0004801c0)
	/w/go/src/cmd/compile/internal/ssagen/ssa.go:668 +0x2fb
cmd/compile/internal/ssagen.buildssa(0xc0001422c0, 0x0, 0x0)
	/w/go/src/cmd/compile/internal/ssagen/ssa.go:589 +0x1d90
cmd/compile/internal/ssagen.Compile(0xc0001422c0, 0x0)
	/w/go/src/cmd/compile/internal/ssagen/pgen.go:151 +0x52
cmd/compile/internal/gc.compileFunctions.func2.1(0xc00010c400, 0xc0001422c0, 0xc00000e720, 0xc000025a50)
	/w/go/src/cmd/compile/internal/gc/compile.go:130 +0x56
created by cmd/compile/internal/gc.compileFunctions.func2
	/w/go/src/cmd/compile/internal/gc/compile.go:128 +0x85

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 24, 2021

Change https://golang.org/cl/304233 mentions this issue: cmd/compile: fix array case in types-for-register parameter

gopherbot pushed a commit that referenced this issue Mar 24, 2021
Corrected typo/thinko.

We should keep the test for this, but it doesn't run yet because of reflection
as far as I know (but I am not testing w/ GOEXPERIMENT).

See #44816 (comment)

Updates #40724
Updates #44816

Change-Id: Ia12d0d4db00a8ec7174e72de460173876bd17874
Reviewed-on: https://go-review.googlesource.com/c/go/+/304233
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@thanm
Copy link
Contributor Author

@thanm thanm commented Mar 25, 2021

Another test case from the fuzzer that causes a runtime error with regiabi (plus pragma).
testcase0325.zip

Symptom:

$ go run .
starting main
Error: fail |0|0| on =genChecker0.Test0= parm 1 elem 2
Error: fail |0|0| =genChecker0.Test0= reflect return 0
Error: fail |0|0| =genChecker0.Test0= reflect return 1
Error: fail |0|0| =genChecker0.Test0= reflect return 2
FAILURES: 4
exit status 2

@thanm
Copy link
Contributor Author

@thanm thanm commented Mar 25, 2021

Another test case from the fuzzer that causes a runtime error with regiabi (plus pragma). This is in the regular call path, not reflect.
testcase0325second.zip

Symptom:

$ go run .
starting main
Error: fail  |0|3| =genChecker0.Test3= parm 0 elem 0
FAILURES: 1
exit status 2

I eyeballed it in the debugger. Looks like RAX is being clobbered in the prolog before it can be used, or something along those lines.

@dr2chase
Copy link
Contributor

@dr2chase dr2chase commented Mar 25, 2021

Ugh, "works for me".

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 30, 2021

Change https://golang.org/cl/306109 mentions this issue: reflect: undo register count increments on register assignment failure

@thanm
Copy link
Contributor Author

@thanm thanm commented Mar 31, 2021

Another day, another new fuzzer failure. This one is pretty similar to the most recent one.
testcase0330.zip

From the SSA dump of the test function prolog:

b1:-
v1 (?) = InitMem <mem>
v2 (?) = SP <uintptr>
v3 (?) = SB <uintptr>
v23 (+54) = ArgIntReg <unsafe.Pointer> {p3+0} [3]
v26 (54) = ArgIntReg <uintptr> {p3+8} [4]
v29 (54) = ArgIntReg <unsafe.Pointer> {p3+16} [5]
v32 (54) = ArgIntReg <uintptr> {p3+24} [6]
v35 (54) = ArgIntReg <uint32> {p3+32} [7]
v40 (54) = Arg <int64> {p10} (p10[int64])
v52 (?) = MOVSDconst <float64> [0]
v61 (56) = LEAQ <*[256]uint64> {pad} v2
v88 (+67) = ArgIntReg <int> {p0+8} [1] (p0+8[int])
v2067 (67) = CMPQconst <flags> [16] v88
v2075 (54) = LEAQ <*StructF0S2> {r2} [24] v2
v2078 (54) = LEAQ <*StructF0S2> {r2} [40] v2
v8 (?) = LEAQ <*uint8> {go.string."genChecker0"} v3
v4 (?) = LEAQ <*uint8> {go.string."parm"} v3
v2179 (54) = MOVQstore <mem> {p3} v2 v23 v1
v2180 (54) = MOVQstore <mem> {p3} [8] v2 v26 v2179
v2181 (54) = MOVQstore <mem> {p3} [16] v2 v29 v2180
v2182 (54) = MOVQstore <mem> {p3} [24] v2 v32 v2181
v2183 (54) = MOVLstore <mem> {p3} [32] v2 v35 v2182
v2475 (222) = ArgFloatReg <float64> {p4+0} [0]
v2482 (222) = ArgIntReg <int8> {p6+0} [8]
v2506 (222) = ArgIntReg <unsafe.Pointer> {p0+0} [0]
v2508 (222) = ArgIntReg <uintptr> {p0+8} [1]
v2511 (222) = ArgIntReg <int64> {p2+0} [2]

So basically a bunch of Arg*Reg ops, then some other code, then more Arg*Reg ops. This is problematic; since regalloc can't see the later Arg*Reg ops, it will assign live param registers to the intervening code.

@thanm
Copy link
Contributor Author

@thanm thanm commented Mar 31, 2021

Another new failure. Zip file with contents:
testcase0330second.zip

Here it looks as though something is going wonky with respect to params that have to be spilled to the stack (since they are address taken).

Signature:

func Test1(p0 int8, p1 StructF1S0, p2 complex64, p3 complex64, _ *float64, p5 float32) (r0 StructF1S5, r1 ArrayF1S0E2, r2 int8, r3 int64, r4 float64) {

Here's what StructF1S0 looks like (expanded nested structs to improve readability):

type StructF1S0 struct {
F0 *string
F1 int64
F20 uint64
F21 int16
F3 float32
}

Checking code:

  ap1 := &p1
  ...
  p1f4c := StructF1S3{F0: StructF1S4{F0: float32(-1.4203252e+37)}}
  if deref_1(ap1).F3 != p1f4c {
    genUtils.NoteFailureElem(18, 9, 1, "genChecker9", "parm", 1, 4, false, pad[0])
    return
  }

In the prolog of the function when we spill p1 (since it is address taken), the code looks like

00008 (40) MOVQ BX, "".p1+16(SP)
00009 (40) MOVQ CX, "".p1+24(SP)
00010 (40) MOVQ DI, "".p1+32(SP)
00011 (40) MOVW SI, "".p1+40(SP)
00012 (40) MOVSS X0, "".p1+44(SP)

Seems ok. The checking code looks like

00079 (+78) LEAQ "".p1+16(SP), AX
00080 (78) MOVQ AX, (SP)
00081 (+78) PCDATA $1, $1
00082 (+78) CALL "".deref_1(SB)
00083 (78) MOVQ 8(SP), AX
00084 (78) MOVQ AX, ""..autotmp_29-88(SP)
00085 (78) MOVUPS 16(SP), X0
00086 (78) MOVUPS X0, ""..autotmp_29-80(SP)
00087 (78) MOVUPS 32(SP), X0
00088 (78) MOVUPS X0, ""..autotmp_29-64(SP)
00089 (78) MOVSS ""..autotmp_29-56(SP), X0
00090 (78) MOVSS $(-1.4203251708691171e+37), X1
00091 (78) UCOMISS X1, X0
00092 (78) JNE 207

The UCOMISS at 00091 is pick up the wrong value. So somewhere along the line (either spilling p1 or materializing its spill address) we got something wrong.

@thanm
Copy link
Contributor Author

@thanm thanm commented Apr 1, 2021

One last fuzzer failure for the day. Haven't analyzed it at all, but here it is.
testcase0330third.zip

gopherbot pushed a commit that referenced this issue Apr 1, 2021
Currently when register assignment fails we roll back all the abiParts
that were generated in the process. However, the total number of
registers also increases, but does not get rolled back. The result is
a very incorrect register assignment.

For #40724.
For #44816.

Change-Id: I1934ea5f95f7608ff2067166255099dbc9135e8c
Reviewed-on: https://go-review.googlesource.com/c/go/+/306109
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@thanm
Copy link
Contributor Author

@thanm thanm commented Apr 2, 2021

Switching back to reflect testing now that the regular call path seems clean. Here is a reduced testcase that shows a reflect.Call failure:
testcase0402second.zip

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 2, 2021

Change https://golang.org/cl/306929 mentions this issue: reflect: remove short-circuits for zero-sized types in ABI algorithm

gopherbot pushed a commit that referenced this issue Apr 2, 2021
This change removes two short-circuits for zero-sized types (zero-sized
structs and zero-sized struct fields) in the recursive cases of the ABI
algorithm, because this does not match the spec's algorithm, nor the
compiler's algorithm.

The failing case here is a struct with a field that is an array of
non-zero length but whose element type is zero-sized. This struct must
be stack-assigned because of the array, according to the algorithm.
The reflect package was register-assigning it.

Because there were two short-circuits, this can also appear if a struct
has a field that is a zero-sized struct but contains such an array,
also.

This change adds regression tests for both of these cases.

For #44816.
For #40724.

Change-Id: I956804170962448197a1c9853826e3436fc8b1ea
Reviewed-on: https://go-review.googlesource.com/c/go/+/306929
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@thanm
Copy link
Contributor Author

@thanm thanm commented Apr 7, 2021

Another reflect test cast that the fuzzer found a failure in:
testcase0407.zip

To reproduce, unpack the zip file and then:

$ cd cabiTest
GOEXPERIMENT=regabi,regabireflect,regabiargs go run .
$ starting main
Error: fail reflect |9|71|19| =genChecker71.Test19= parm 0 elem 1
FAILURES: 1
exit status 2
$ 

I should also add that in my repo I have this diff:

$ git diff
diff --git a/src/reflect/regabiargs_off.go b/src/reflect/regabiargs_off.go
index 655e955474..5dc4a063e4 100644
--- a/src/reflect/regabiargs_off.go
+++ b/src/reflect/regabiargs_off.go
@@ -7,4 +7,4 @@
 
 package reflect
 
-const experimentRegabiArgs = 0
+const experimentRegabiArgs = 1

This is a method call, not a regular call. In addition there seem to be a number of other moving parts (the param in question is address taken and escapes to the heap). It is a value receiver (not pointer), and the size of the type in question is zero. I shoudl also add that it looks as though there is a method wrapper involved (from the stack trace). Here is a dump of the abi regs at the point where the regular (non-reflect) call takes place, when we've just entered the Test19 function:

(gdb) showabiregs
"0: rax "$35 = 27
"1: rbx "$36 = 9887
"2: rcx "$37 = 0
"3: rdi "$38 = 11297
"4: rsi "$39 = 824633786928
"5: r8 "$40 = 2588397972534514886
"6: r9 "$41 = 8
"7: r10 "$42 = 140737353884592
"8: r11 "$43 = 824633786928
(gdb) 

and here is the dump for the reflect call

(gdb) showabiregs
"0: rax "$44 = 27
"1: rbx "$45 = 0
"2: rcx "$46 = 0
"3: rdi "$47 = 11297
"4: rsi "$48 = 824633786928
"5: r8 "$49 = 2588397972534514886
"6: r9 "$50 = 2588397972534514886
"7: r10 "$51 = 0
"8: r11 "$52 = 0
(gdb) 

So $rbx doesn't look right.

@thanm
Copy link
Contributor Author

@thanm thanm commented Apr 7, 2021

New fuzzer failure (compile time crash). Zip file with repro:
testcase0407second.zip

To reproduce, do:

$ unzip ...
$ cd cabiTest
$ GOEXPERIMENT=regabi,regabiargs go build  .
# cabiTest/genCaller42
genCaller42/genCaller42.go:166:35: internal compiler error: 'Caller2': panic during expand calls while compiling Caller2:

Unexpected selector of memory

goroutine 136 [running]:
cmd/compile/internal/ssa.Compile.func1()
	/ssd2/go/src/cmd/compile/internal/ssa/compile.go:48 +0xa5
panic(0xd85e60, 0xf7da28)
	/ssd2/go/src/runtime/panic.go:1031 +0x21f
cmd/compile/internal/ssa.(*expandState).rewriteSelect(0xc00017bce0, 0xc0011b0940, 0xc0011b00f0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:399 +0x1dc6
cmd/compile/internal/ssa.(*expandState).rewriteSelect(0xc00017bce0, 0xc0011b0940, 0xc0011a3f20, 0x0, 0xe36f00, 0x0, 0x0, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:469 +0x10de
cmd/compile/internal/ssa.(*expandState).rewriteSelect(0xc00017bce0, 0xc0011b0940, 0xc0011b0940, 0x0, 0x0, 0x0, 0x0, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:483 +0x11ee
cmd/compile/internal/ssa.expandCalls(0xc00012a000)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:1357 +0x145d
cmd/compile/internal/ssa.Compile(0xc00012a000)
	/ssd2/go/src/cmd/compile/internal/ssa/compile.go:96 +0xc38
cmd/compile/internal/ssagen.buildssa(0xc000262580, 0x2, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssagen/ssa.go:642 +0x23c5
cmd/compile/internal/ssagen.Compile(0xc000262580, 0x2)
	/ssd2/go/src/cmd/compile/internal/ssagen/pgen.go:151 +0x58
cmd/compile/internal/gc.compileFunctions.func2.1()
	/ssd2/go/src/cmd/compile/internal/gc/compile.go:130 +0x75
created by cmd/compile/internal/gc.compileFunctions.func2
	/ssd2/go/src/cmd/compile/internal/gc/compile.go:128 +0xe5



goroutine 136 [running]:
runtime/debug.Stack(0xf8b2c0, 0xc000202008, 0x0)
	/ssd2/go/src/runtime/debug/stack.go:24 +0x94
cmd/compile/internal/base.FatalfAt(0xa623000000002, 0xc000a46340, 0x32, 0xc002f8de00, 0x5, 0x5)
	/ssd2/go/src/cmd/compile/internal/base/print.go:227 +0x1d7
cmd/compile/internal/base.Fatalf(...)
	/ssd2/go/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/ssagen.(*ssafn).Fatalf(0xc000128000, 0xa623000000002, 0xe5d83a, 0x2c, 0xc002fa0340, 0x4, 0x4)
	/ssd2/go/src/cmd/compile/internal/ssagen/ssa.go:7383 +0x1ed
cmd/compile/internal/ssa.(*Func).Fatalf(0xc00012a000, 0xe5d83a, 0x2c, 0xc002fa0340, 0x4, 0x4)
	/ssd2/go/src/cmd/compile/internal/ssa/func.go:667 +0x327
cmd/compile/internal/ssa.Compile.func1()
	/ssd2/go/src/cmd/compile/internal/ssa/compile.go:53 +0x230
panic(0xd85e60, 0xf7da28)
	/ssd2/go/src/runtime/panic.go:1031 +0x21f
cmd/compile/internal/ssa.(*expandState).rewriteSelect(0xc00017bce0, 0xc0011b0940, 0xc0011b00f0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:399 +0x1dc6
cmd/compile/internal/ssa.(*expandState).rewriteSelect(0xc00017bce0, 0xc0011b0940, 0xc0011a3f20, 0x0, 0xe36f00, 0x0, 0x0, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:469 +0x10de
cmd/compile/internal/ssa.(*expandState).rewriteSelect(0xc00017bce0, 0xc0011b0940, 0xc0011b0940, 0x0, 0x0, 0x0, 0x0, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:483 +0x11ee
cmd/compile/internal/ssa.expandCalls(0xc00012a000)
	/ssd2/go/src/cmd/compile/internal/ssa/expand_calls.go:1357 +0x145d
cmd/compile/internal/ssa.Compile(0xc00012a000)
	/ssd2/go/src/cmd/compile/internal/ssa/compile.go:96 +0xc38
cmd/compile/internal/ssagen.buildssa(0xc000262580, 0x2, 0x0)
	/ssd2/go/src/cmd/compile/internal/ssagen/ssa.go:642 +0x23c5
cmd/compile/internal/ssagen.Compile(0xc000262580, 0x2)
	/ssd2/go/src/cmd/compile/internal/ssagen/pgen.go:151 +0x58
cmd/compile/internal/gc.compileFunctions.func2.1()
	/ssd2/go/src/cmd/compile/internal/gc/compile.go:130 +0x75
created by cmd/compile/internal/gc.compileFunctions.func2
	/ssd2/go/src/cmd/compile/internal/gc/compile.go:128 +0xe5

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2021

Change https://golang.org/cl/308309 mentions this issue: cmd/compile: sanitize before/after expansion OpSelectN references

gopherbot pushed a commit that referenced this issue Apr 8, 2021
In expand_calls, OpSelectN occurs both before and after the rewriting.
Attempting to rewrite a post-expansion OpSelectN is bad.
(The only ones rewritten in place are the ones returning mem;
others are synthesized to replace other selection chains with
register references.)

Updates #40724.
Updates #44816#issuecomment-815258897.

Change-Id: I7b6022cfb47f808d3ce6cc796c067245f36047f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/308309
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2021

Change https://golang.org/cl/308589 mentions this issue: cmd/compile: add recursive-invalidate Value method, use in expand_calls

@thanm
Copy link
Contributor Author

@thanm thanm commented Apr 8, 2021

Another reflect problem:
testcase0408.zip

This one seems to have to do with floating point values. Repro:

$ unzip testcase0408.zip
...
$ cd cabiTest
GOEXPERIMENT=regabi,regabireflect,regabiargs go run .
starting main
Error: fail reflect |10|73|5| =genChecker73.Test5= parm 5 elem 0
FAILURES: 1
exit status 2
$

Looking at the element in question ("p5"), on the calling side it is set up with

p5 = genChecker73.StructF5S2{F0: genChecker73.ArrayF5S0E0{}, F1: float32(-1.1470688e+37)}

If I stop at the entry point of genChecker73.Test5 and look at the floating point regs after the regular direct call, I see:

(gdb) p /x $xmm0.uint128
$32 = 0xbfeb8263537523d6
(gdb) p /x $xmm1.uint128
$33 = 0x3ffca0716ee47b6c
(gdb) p /x $xmm2.uint128
$34 = 0xfd0a12c9
(gdb) 

For the reflect call, here's what's coming through:

(gdb) p /x $xmm0.uint128
$35 = 0xbfeb8263537523d6
(gdb) p /x $xmm1.uint128
$36 = 0x3ffca0716ee47b6c
(gdb) p /x $xmm2.uint128
$37 = 0x490ee0
(gdb) 

So you can see that the value of $xmm2 doesn't look right.

gopherbot pushed a commit that referenced this issue Apr 8, 2021
This removes more unused values during transformation.
Leaving them in the tree can create type conflicts in OpArg* references.

Updates #40724.
Updates #44816.
Fixes #45417.

Change-Id: I07dcb7b4b2bf8d79e22e0543cb2fb52c2ececb96
Reviewed-on: https://go-review.googlesource.com/c/go/+/308589
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
@thanm
Copy link
Contributor Author

@thanm thanm commented Apr 8, 2021

... and one more fuzzer-generated failure, also in reflect. Zip file:
testcase0408second.zip

This one is interesting in that the parameter in question isn't actually assigned to a register (as far as I can tell), it's passed on the stack. Haven't done any additional analysis.

Repro:

$ unzip testcase0408second.zip
...
$ cd cabiTest
$ GOEXPERIMENT=regabi,regabireflect,regabiargs go run . )
starting main
Error: fail reflect |31|99|13| =genChecker99.Test13= parm 6 elem 0
FAILURES: 1
exit status 2
$

gopherbot pushed a commit that referenced this issue May 6, 2021
This disables the "testing names" for method names and
trailing input types passed to closure/interface/other calls.
The logic using the names remains, so that editing the change
to enable local testing is not too hard.

Also fixes broken build tag in reflect/abi_test.go

Updates #44816.

Change-Id: I3d222d2473c98d04ab6f1122ede9fea70c994af1
Reviewed-on: https://go-review.googlesource.com/c/go/+/300150
Trust: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants