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

Jump to bottom

cmd/compile: further failures relating to CL 450136 (inlined static initializer optimization) #56894

Closed
mdempsky opened this issue Nov 22, 2022 · 4 comments
Closed

cmd/compile: further failures relating to CL 450136 (inlined static initializer optimization) #56894

mdempsky opened this issue Nov 22, 2022 · 4 comments
Assignees
@mdempsky
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@mdempsky
Copy link
Member

Google-internal toolchain testing is hitting further failures relating to CL 450136 (internal tracking reference: b/260007024).

I think we should at least move the static-init optimization behind a compiler flag and disable it for now while we minimize the failure cases.
@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 22, 2022
@mdempsky mdempsky added this to the Go1.20 milestone Nov 22, 2022
@mdempsky mdempsky self-assigned this Nov 22, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 22, 2022
@cuonglm
Copy link
Member

cuonglm commented Nov 22, 2022

Could you share the stack trace with confidential information stripped?

@mdempsky
Copy link
Member Author

Could you share the stack trace with confidential information stripped?

At least the first case I looked at was a runtime test failure, not an ICE, so there's nothing useful to report yet without minimizing the failure.

@gopherbot
Copy link

Change https://go.dev/cl/452676 mentions this issue: cmd/compile: add -d=inlstaticinit debug flag

gopherbot pushed a commit that referenced this issue Nov 22, 2022
@mdempsky
cmd/compile: add -d=inlstaticinit debug flag
1521199 
This CL adds -d=inlstaticinit to control whether static initialization
of inlined function calls (added in CL 450136) is allowed.

We've needed to fix it once already (CL 451555) and Google-internal
testing is hitting additional failure cases, so putting this
optimization behind a feature flag seems appropriate regardless.

Also, while we diagnose and fix the remaining cases, this CL also
disables the optimization to avoid miscompilations.

Updates #56894.

Change-Id: If52a358ad1e9d6aad1c74fac5a81ff9cfa5a3793
Reviewed-on: https://go-review.googlesource.com/c/go/+/452676
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
@mdempsky
Copy link
Member Author

After further testing, the failures in question appear unrelated to this. At least some of the failures were probably actually fixed by CL 452616, and misattributed due to build failures interfering with bisection. See Russ's commit message in go.dev/cl/452817 for further explanation.

@golang golang locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mdempsky mdempsky

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
3 participants
@mdempsky @cuonglm @gopherbot