-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: unexpected nil dereference on s390x #38195
Comments
Change https://golang.org/cl/226759 mentions this issue: |
Store multiple instructions can clobber flags on s390x when the offset passed into the assembler is outside the range representable with a signed 20 bit integer. This is because the assembler uses the agfi instruction to implement the large offset. The assembler could use a different sequence of instructions, but for now just mark the instruction as 'clobberFlags' since this is risk free. Noticed while investigating #38195. No test yet since I'm not sure how to get this bug to trigger and I haven't seen it affect real code. Change-Id: I4a6ab96455a3ef8ffacb76ef0166b97eb40ff925 Reviewed-on: https://go-review.googlesource.com/c/go/+/226759 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
This looks like a bug in the register allocation for the function. v1909 is allocated to R5, then R5 is taken by v1408 but v1909 (still in R5) is then used as a source to v1368.
Any thoughts on what might be the issue here @randall77 @dr2chase @cherrymui? |
v1909 is created because v18 has been allocated to R0, which cannot be used as an address for loads or stores (it is always evaluated as 0 in address calculations). This means that the value of v1909 (R5) and v18 (R0) both contain the same value. So I wonder if this is a subtle bug in the way we handle multiple registers containing the same value. |
To generate the ssa.html for the bad code you can use master at 815509a: git clone https://gopkg.in/yaml.v3
cd yaml.v3
git checkout 9f266ea
GOSSAFUNC=yaml_parser_parse_node GOOS=linux GOARCH=s390x go test -c The value numbers will be different but the pattern is the same:
|
Change https://golang.org/cl/228060 mentions this issue: |
I think the problem is this code - specifically the call to go/src/cmd/compile/internal/ssa/regalloc.go Lines 986 to 990 in 34e38ac
That call frees all the copies of the value that is the input to the phi - some of which might actually be live and used by other values (as in this case). If the free'd value is subsequently used to copy a different phi input then the go/src/cmd/compile/internal/ssa/regalloc.go Line 1012 in 34e38ac
CL 228060 adds an ICE if this invalid state is seen. I think the solution to this specific bug is just to deallocate the register holding the input to the phi and leave any copies untouched. In general though this code is really hairy and I'm still not really sure that even with this change it is correct. It might be worth spending some time simplifying how phi allocations work so there are fewer data structures to mentally track. |
Change https://golang.org/cl/228061 mentions this issue: |
Keith put a good summary of this issue in the CL for anyone following along:
|
When setting the edge state in register allocation we should only be setting each register once. It is not possible for a register to hold multiple values at once. This CL converts the runtime error seen in #38195 into an internal compiler error (ICE). It is better for the compiler to fail than generate an incorrect program. The bug reported in #38195 is now exposed as: ./parserc.go:459:11: internal compiler error: 'yaml_parser_parse_node': R5 is already set (v1074/v1241) [stack trace] Updates #38195. Change-Id: Id95842fd850b95494cbd472b6fd5a55513ecacec Reviewed-on: https://go-review.googlesource.com/c/go/+/228060 Run-TryBot: Michael Munday <mike.munday@ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot please consider this for a backport This is a bug in the register allocator that results in the compiled program behaving incorrectly. There is currently no workaround and it results in very hard to debug issues. It has only been seen to cause problems on s390x but theoretically it affects all platforms. |
Backport issue(s) opened: #38442 (for 1.13), #38443 (for 1.14). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
This is a follow up issue to mikefarah/yq#394 which only appears to affect s390x. That issue indicates a bug in the compiler that affects Go 1.13.x and Go 1.14.x. Git bisect points to CL 173323 and if that CL is applied to Go 1.12.x then that release is also affected. I am fairly certain that CL 173323 isn't the root cause however. It appears to have revealed a latent bug.
To reproduce:
You will see unexpected nil dereference panics in most of the tests. For example:
Things that 'fix' the issue:
go/src/cmd/compile/internal/ssa/gen/S390X.rules
Lines 1948 to 1968 in 8e6a8d9
-N
(no opt) or-l=4
(max inlining) flagsNone of these are the cause I think. I suspect an op is missing a
clobberFlags
mark or something like that. The store multiple ops should have theclobberFlags
mark and currently don't but that doesn't appear to affect this issue (store multiple will only clobber flags if the displacement does not fit inside aint20
and this can only happen with autos - which would need a 512K stack frame).The text was updated successfully, but these errors were encountered: