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

runtime: 1.22 breaks code that worked with 1.21 #65706

Closed
markus-wa opened this issue Feb 14, 2024 · 10 comments
Closed

runtime: 1.22 breaks code that worked with 1.21 #65706

markus-wa opened this issue Feb 14, 2024 · 10 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@markus-wa
Copy link
Contributor

markus-wa commented Feb 14, 2024

Go version

go1.22.0

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/mark/.cache/go-build'
GOENV='/home/mark/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/mark/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/mark/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/mark/dev/markus-wa/demoinfocs-golang/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3279098905=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Upgrade from go1.21.7 to go1.22.0 and then

./scripts/download-test-data.sh s2.7z
go test -run TestS2 ./pkg/demoinfocs` in https://github.com/markus-wa/demoinfocs-golang

See also failing CI job with go1.22.0: https://github.com/markus-wa/demoinfocs-golang/actions/runs/7900058618/job/21560795144?pr=508

What did you see happen?

Test fails with interface conversion: interface {} is *sendtables2.fieldState, not uint64 - I think this is just a side-effect of another issue.

Full stack trace:

   demoinfocs_test.go:540: 
        	Error Trace:	/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:540
        	            				/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:541
        	            				/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:563
        	Error:      	Received unexpected error:
        	            	interface conversion: interface {} is *sendtables2.fieldState, not uint64
        	            	stacktrace:
        	            	goroutine 37 [running]:
        	            	runtime/debug.Stack()
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/runtime/debug/stack.go:24 +0x5e
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs.NewParserWithConfig.func1({0x991f40, 0xc00d458c10})
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/parser.go:403 +0x34
        	            	github.com/markus-wa/godispatch.(*Dispatcher).dispatchWithRecover.func1()
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:197 +0x3b
        	            	panic({0x991f40?, 0xc00d458c10?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/runtime/panic.go:770 +0x132
        	            	github.com/markus-wa/godispatch.callConsumerCode.func1()
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:122 +0x54
        	            	panic({0x95e640?, 0xc00ad526f0?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/runtime/panic.go:770 +0x132
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs/sendtables.PropertyValue.S2UInt64(...)
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/sendtables/propdecoder.go:152
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs.(*parser).bindPlayerWeaponsS2.func3({{0x0, 0x0, 0x0}, 0x0, 0x0, {0x0, 0x0, 0x0}, {0xb14e50, 0x0}, ...})
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/datatables.go:745 +0xb7
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs/sendtables2.(*Parser).OnPacketEntities(0xc00bc69b80, 0xc009ddbd40)
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/sendtables2/entity.go:574 +0xb9a
        	            	reflect.Value.call({0x924000?, 0xc00357dea8?, 0x2?}, {0xa67dc7, 0x4}, {0xc00326bef0, 0x1, 0xc0003d8620?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/reflect/value.go:596 +0xce5
        	            	reflect.Value.Call({0x924000?, 0xc00357dea8?, 0x415262?}, {0xc00326bef0?, 0x0?, 0xd3ffffffffffffff?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/reflect/value.go:380 +0xb9
        	            	github.com/markus-wa/godispatch.callConsumerCode({0x924000?, 0xc00357dea8?, 0xc00326bf08?}, {0xc00326bef0?, 0xc00326be98?, 0xc00326bec0?})
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:125 +0x3f
        	            	github.com/markus-wa/godispatch.(*Dispatcher).Dispatch(0xc004f7c900, {0xa4ae20, 0xc009ddbd40})
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:111 +0x20c
        	            	github.com/markus-wa/godispatch.(*Dispatcher).dispatchWithRecover(0xc004f7c918?, {0xa4ae20?, 0xc009ddbd40?})
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:202 +0x4f
        	            	github.com/markus-wa/godispatch.(*Dispatcher).dispatchQueue(0xc004f7c900, 0xc004f7ca20)
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:177 +0xd0
        	            	created by github.com/markus-wa/godispatch.(*Dispatcher).AddQueues in goroutine 126
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:162 +0x18a
        	Test:       	TestDemoSetS2
        	Messages:   	parsing of '../../test/cs-demos/set/s2.dem' failed

What did you expect to see?

Test passes, as it did with go1.21.7

@markus-wa
Copy link
Contributor Author

markus-wa commented Feb 14, 2024

Further info:

I ran go1.21.7 with GOEXPERIMENT=loopvar and the issue did NOT reproduce

I did a git bisect 1.22.0 1.21.7 on the go repo and found 25e4876 to be the offending revision

  • I don't fully understand why this would affect it, but the previous revision (2744155) works fine
  • I tested this multiple times to rule out flakiness and it reliably works/breaks with the good/bad revisions above
  • git revert 25e4876 on the go1.22.0 tag fixes the issue for me

I think it would not be impossible for this to be a case where we accidentally relied on some weird undocumented behaviour or something, but I can't fathom what it could be.

@markus-wa markus-wa changed the title cmd/go: 1.22 breaks code that worked with 1.21 runtime: 1.22 breaks code that worked with 1.21 Feb 14, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 14, 2024
@mauri870 mauri870 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 14, 2024
@odeke-em
Copy link
Member

@markus-wa thank you for this report! Could you please try to isolate a standalone reproduction that can massively help in crafting the fix and tests and trivially runnable by anyone?

@markus-wa
Copy link
Contributor Author

I appreciate the request @odeke-em, but right now I don't really know where to start. I reckon the failing test is just a symptom of something much deeper, and the issue likely lies far away from any of the panic's stack-trace.

If I can find some time, I will investigate this further, but I can't say how quickly I'll be able to produce a practical minimal reproduction example as it's tough to isolate some of the code in the linked repo.

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 14, 2024
@odeke-em
Copy link
Member

Even just the appropriate line numbers or the full failing stack trace can help out. Your report doesn't show the line numbers.

When I try your reproduction instructions I don't get the same problem, I get back the following

$ go version && go test -run TestS2 ./pkg/demoinfocs
go version go1.22.0 darwin/amd64
--- FAIL: TestS2 (0.00s)
    demoinfocs_test.go:210: 
        	Error Trace:	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:210
        	Error:      	Received unexpected error:
        	            	open ../../test/cs-demos/s2/s2.dem: no such file or directory
        	Test:       	TestS2
        	Messages:   	error opening demo "../../test/cs-demos/s2/s2.dem"
    demoinfocs_test.go:679: 
        	Error Trace:	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:679
        	            				/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:674
        	            				/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/runtime/panic.go:770
        	            				/Users/emmanuelodeke/go/pkg/mod/github.com/markus-wa/gobitread@v0.2.3/bitread.go:86
        	            				/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/internal/bitread/bitread.go:142
        	            				/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/internal/bitread/bitread.go:162
        	            				/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/pkg/demoinfocs/parser.go:380
        	            				/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:217
        	Error:      	Received unexpected error:
        	            	invalid argument
        	Test:       	TestS2
        	Messages:   	failed to close file, reader or writer
panic: invalid argument [recovered]
	panic: invalid argument

goroutine 21 [running]:
testing.tRunner.func1.2({0x6497640, 0x6ad6a60})
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/testing/testing.go:1634 +0x377
panic({0x6497640?, 0x6ad6a60?})
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/runtime/panic.go:770 +0x132
github.com/markus-wa/gobitread.(*BitReader).OpenWithBuffer(0xc000323ce0, {0x65bd638?, 0x0?}, {0xc000380000?, 0x20000?, 0x645f2c0?})
	/Users/emmanuelodeke/go/pkg/mod/github.com/markus-wa/gobitread@v0.2.3/bitread.go:86 +0xc5
github.com/markus-wa/demoinfocs-golang/v4/internal/bitread.newBitReader({0x65bd638, 0x0}, 0xc000126510)
	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/internal/bitread/bitread.go:142 +0x86
github.com/markus-wa/demoinfocs-golang/v4/internal/bitread.NewLargeBitReader(...)
	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/internal/bitread/bitread.go:162
github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs.NewParserWithConfig({0x65bd638, 0x0}, {0x0, 0x0, 0x0, {0x0, 0x0, 0x0}, 0x0, {0x0, ...}})
	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/pkg/demoinfocs/parser.go:380 +0xae
github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs_test.TestS2(0xc00013d1e0)
	/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/65706/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:217 +0x23f
testing.tRunner(0xc00013d1e0, 0x65b9238)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 1
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/testing/testing.go:1742 +0x390
FAIL	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs	1.173s
FAIL

@markus-wa
Copy link
Contributor Author

markus-wa commented Feb 14, 2024

Right, sorry about that - I forgot that the required testdata isn't downloaded by default as it's quite big.

Just edited the bug description.

You'll need to first run ./scripts/download-test-data.sh s2.7z (requires 7z CLI)

This failing CI job has the full error as well: https://github.com/markus-wa/demoinfocs-golang/actions/runs/7900058618/job/21560795144?pr=508

Here is the full stack trace:


   demoinfocs_test.go:540: 
        	Error Trace:	/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:540
        	            				/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:541
        	            				/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/demoinfocs_test.go:563
        	Error:      	Received unexpected error:
        	            	interface conversion: interface {} is *sendtables2.fieldState, not uint64
        	            	stacktrace:
        	            	goroutine 37 [running]:
        	            	runtime/debug.Stack()
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/runtime/debug/stack.go:24 +0x5e
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs.NewParserWithConfig.func1({0x991f40, 0xc00d458c10})
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/parser.go:403 +0x34
        	            	github.com/markus-wa/godispatch.(*Dispatcher).dispatchWithRecover.func1()
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:197 +0x3b
        	            	panic({0x991f40?, 0xc00d458c10?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/runtime/panic.go:770 +0x132
        	            	github.com/markus-wa/godispatch.callConsumerCode.func1()
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:122 +0x54
        	            	panic({0x95e640?, 0xc00ad526f0?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/runtime/panic.go:770 +0x132
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs/sendtables.PropertyValue.S2UInt64(...)
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/sendtables/propdecoder.go:152
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs.(*parser).bindPlayerWeaponsS2.func3({{0x0, 0x0, 0x0}, 0x0, 0x0, {0x0, 0x0, 0x0}, {0xb14e50, 0x0}, ...})
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/datatables.go:745 +0xb7
        	            	github.com/markus-wa/demoinfocs-golang/v4/pkg/demoinfocs/sendtables2.(*Parser).OnPacketEntities(0xc00bc69b80, 0xc009ddbd40)
        	            		/home/runner/work/demoinfocs-golang/demoinfocs-golang/pkg/demoinfocs/sendtables2/entity.go:574 +0xb9a
        	            	reflect.Value.call({0x924000?, 0xc00357dea8?, 0x2?}, {0xa67dc7, 0x4}, {0xc00326bef0, 0x1, 0xc0003d8620?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/reflect/value.go:596 +0xce5
        	            	reflect.Value.Call({0x924000?, 0xc00357dea8?, 0x415262?}, {0xc00326bef0?, 0x0?, 0xd3ffffffffffffff?})
        	            		/opt/hostedtoolcache/go/1.22.0/x64/src/reflect/value.go:380 +0xb9
        	            	github.com/markus-wa/godispatch.callConsumerCode({0x924000?, 0xc00357dea8?, 0xc00326bf08?}, {0xc00326bef0?, 0xc00326be98?, 0xc00326bec0?})
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:125 +0x3f
        	            	github.com/markus-wa/godispatch.(*Dispatcher).Dispatch(0xc004f7c900, {0xa4ae20, 0xc009ddbd40})
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:111 +0x20c
        	            	github.com/markus-wa/godispatch.(*Dispatcher).dispatchWithRecover(0xc004f7c918?, {0xa4ae20?, 0xc009ddbd40?})
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:202 +0x4f
        	            	github.com/markus-wa/godispatch.(*Dispatcher).dispatchQueue(0xc004f7c900, 0xc004f7ca20)
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:177 +0xd0
        	            	created by github.com/markus-wa/godispatch.(*Dispatcher).AddQueues in goroutine 126
        	            		/home/runner/go/pkg/mod/github.com/markus-wa/godispatch@v1.4.1/dispatch.go:162 +0x18a
        	Test:       	TestDemoSetS2
        	Messages:   	parsing of '../../test/cs-demos/set/s2.dem' failed

@seankhliao seankhliao added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Feb 14, 2024
@randall77
Copy link
Contributor

@markus-wa Did your program work with 1.20? It has the same growth factors as 1.22.

Does your program pass the race detector? I could see an undiagnosed race being triggered by a different map growth schedule.

@markus-wa
Copy link
Contributor Author

markus-wa commented Feb 14, 2024

Very interesting! Downgrading to 1.20 does indeed break it as well!

The test does pass the race detector, however - and in general it's not just the test that's affected, the whole library always works on 1.21 using vast datasets that would probably lead to different map growth scheduling characteristics (if that makes sense - that's just me guessing really)

So I wonder in what kind of other situation such a change could break/"fix" code?
I thought this change was just a matter of performance?

What would be the best steps to figuring out why these two load factors behave differently?

PS: I suppose this could be hinting at the idea that actually go1.21 was bugged, which we somehow relied upon in a weird way. if that would be the case I guess that would probably warrant a backport of the fix to 1.21 once we figure out why the different behaviour occurrs

@randall77
Copy link
Contributor

So I wonder in what kind of other situation such a change could break/"fix" code?
I thought this change was just a matter of performance?

Your thought is correct. It is not possible for this change to break spec-compliant programs.

I would audit all the uses of unsafe in your program and imported libraries.
You might also check for any dependence on map iteration order. It is possible that different growth thresholds lead to different possible iteration orders.

Just plain debugging your program would be a good step as well. Where is that *sendtables2.fieldState coming from?
Should that ever be in a PropertyValue?

@markus-wa
Copy link
Contributor Author

The map ordering thought is interesting - I'll take a look. I'm not sure about unsafe though - there's definitely no usage of it in my lib and I would be surprised if dependencies used it 🤔

Anyway, I'll report back after some more investigation!

@markus-wa
Copy link
Contributor Author

I'm not exactly sure where the behaviour came from but changing some seemingly non-map-related logic fixed the issue.

I would love to figure out what exactly happened here as I still don't think my program wasn't spec compliant but for lack of time and understanding of the matter I think I'll close this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

6 participants