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/internal/ssa: plan9-amd64 is broken #15001
Comments
cmd/asm seems to accept:
Not sure why cmd/internal/obj would be unhappy with cmd/compile generating it. |
I can generate that exact instruction (with different output register) with:
and it works fine. |
Ah, I see what is happening. Small repro:
We're trying to generate |
I note that trying to assemble:
fails with:
Definitely confusing that cmd/internal/obj isn't printing the (CX) index. Also notably, changing it to (CX*1) makes it assemble successfully. Is that an easy fix? Is the SSA backend not setting the index scale correctly? |
I think this is a separate issue. |
CL https://golang.org/cl/21245 mentions this issue. |
I've uploaded https://go-review.googlesource.com/#/c/21245 which fixes the compilation failure. The result now is:
which translates to roughly:
Notably the load is occurring before we've validated that the slice length is >122. It's possible this could result in triggering a SIGSEGV instead of a panicindex failure. Also, I can't quite make sense of the |
Notably, if you remove the So all that to say, I think CL 21245 fixes the immediate problems with CL 21005 (namely that plan9-amd64 fails to build), but there's still a subtle correctness issue with the load combining rules. |
Yuck, yes, we need to be a bit more careful with these rules, don't we. Maybe we need to put the resulting load in a block that is dominated by or equal to the blocks of the original loads. Now that I look at it, I think we need uses==1 for all of the byte loads as well. |
Thanks for fixing this issue. |
https://go-review.googlesource.com/#/c/21005 broke plan9-amd64. This can be reproduced with
Standalone mostly minimized test case at https://gist.github.com/mdempsky/8e17da862a0f3a2c1036. This test case repros even with GOOS=linux, so the failure isn't plan9 specific at all, except that package syscall's plan9-specific code happens to tickle the failure.
Also, it seems to be sensitive to inlining, because removing the definition of Pread causes it to stop failing.
/cc @randall77 @dr2chase
The text was updated successfully, but these errors were encountered: