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: regression in b65122f causes link time error error: R_PPC64_ADDR16_LO_DS not a multiple of 4 #24799
Comments
The commit in question is CL 102558. I think the optimizations enabled by this CL are safe on any architecture (even ones that don't allow unaligned accesses). It relies on the SSA rules to actually generate unaligned loads. This makes me think that we need more constraints on load merging when accessing global symbols on ppc64le. @laboger: does the code below cause a bad relocation? package main
import "encoding/binary"
var x [5]byte
func main() {
if binary.LittleEndian.Uint32(x[1:5]) == 1 {
panic("wrong")
}
} |
The list of DS instructions with this restriction is small. Your above example does not cause an error since it is a 4 bytes unsigned. I tried to change it to 8 bytes but that doesn't fail because the rules check to combine LE loads checks that the compile time offset is a multiple of 4. I believe I fixed all the rules where this was a problem due to the compile time offset being incorrect, because the problem did come up when I first added them. Also, asm9.go checks the constant offset field when assembling an instruction and if the offset field is not a multiple of 4 for a DS instruction an error is issued. I believe the fact that the error is coming from the linker means that the compile time offset is OK but the relocated offset ends up with a value or 1 or 2. And this error would probably only come from the external linker because I doubt the golang linker knows this is a problem. I will look into the failing .o file and see if I can find exactly the symbol that is causing the failure so a smaller reproducer can be created. |
Here is the before and after:
The code in asm9.go for symbolAccess is not checking if the offset being added to the relocation is a multiple of 4 when using DS relocation and maybe it should... I think the assumption was that 8 byte constants or static variables would always be aligned correctly but in this case it is not. |
Ah interesting, yes, static variables only have to be aligned to 1 byte. Increasing the linkers minimum data alignment to 4 might solve your problem: go/src/cmd/link/internal/ppc64/l.go Line 66 in 38cfeb9
We have it set to 2 on s390x, though thinking about it it might make sense to push it up to 8 since that would make the rewrite rules easier. |
It does work to change the alignment as you suggest above. I also think you were right originally, this is a problem in the combine rules, because it shouldn't be combining bytes to do an 8 byte load if it isn't aligned properly. I've been trying to add to the rule to check for alignment and I can't get it to build. Here is what I added:
But that causes this error:
Any advice on what is wrong with my rule? |
This reproduces the problem.
Fails with both the internal and external linker, with different messages:
I think there are multiple ways to fix it, just trying to decide which one is best.
I think the last choice is best since that doesn't force alignment on go.strings, and doesn't prevent some matching. |
I'm working on a fix to load an 8 byte go.string in a way that doesn't use relocation. |
Change https://golang.org/cl/107855 mentions this issue: |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version
)?tip
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?ppc64le
What did you do?
Trying to build openshift.
What did you expect to see?
Successful build.
What did you see instead?
Link errors at build time. Here is the full output:
This error does not occur with the commit before it.
Looks like the change was intended to merge loads even if they were unaligned, but if a DS load is chosen then the offset must be a multiple of 4 (otherwise you get the above error message).
@mundaym
The text was updated successfully, but these errors were encountered: