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

cmd/compile: PPC64.rules for Zero are not checking alignment #21947

Closed
rgdoliveira opened this issue Sep 20, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@rgdoliveira
Copy link

commented Sep 20, 2017

System information

Go version: 1.9
OS: Alpine Linux 3.6 (but it is not an Alpine specific issue)
Architecture: ppc64le

Seems that PPC64.rules for Zero are not checking alignment in the cases when they should be.
I reproduced this issue when tried to compile geth project and it failed with error:

compile: invalid offset for DS form load/store 00056 (/home/rdutra/repos/aports/community/geth/src/go-ethereum-1.7.0/build/_workspace/src/github.com/ethereum/go-ethereum/eth/downloader/statesync.go:452) MOVD R0, "".~r2+33(FP)

Steps to reproduce the behaviour

Clone go-ethereum repository in a ppc64le distribution
run make (with go 1.9 installed)

@ianlancetaylor ianlancetaylor changed the title PPC64.rules for Zero are not checking alignment cmd/compile: PPC64.rules for Zero are not checking alignment Sep 20, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Sep 20, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Sep 20, 2017

Change https://golang.org/cl/64970 mentions this issue: cmd/compile: fix regression in PPC64.rules move zero

@laboger

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2017

When a MOVD instruction is used the offset field must be a multiple of 4. We had seen this problem a few months ago but MOVDstorezero did not get fixed at that time.

It can be reproduced when there is a function with something like this:

type Hash [32]byte

func testbool() (bool, Hash) {
        var h Hash
        return true, h 
}

When there is a MOVDstorezero to data that is >= 8 bytes but not nicely aligned, you can hit this error.

@laboger

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

This should be backported to go 1.9.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.9.1 Sep 21, 2017

@gopherbot gopherbot closed this in 3fda376 Sep 26, 2017

@laboger

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2017

I saw there was an announcement about go 1.9.1. Will this make it in there?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2017

I think 1.9.1 is going to be a security releases, so all the 1.9.1 issues will be bumped to 1.9.2.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2017

But I'll reopen this one now so that it is considered for 1.9.2.

@ianlancetaylor ianlancetaylor reopened this Oct 3, 2017

@rsc rsc modified the milestones: Go1.9.1, Go1.9.2 Oct 4, 2017

@funny-falcon

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2017

Is it bound with #22047 in any way?

@laboger

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2017

No, not related to #22047, the failure in this issue would show up as a compile time error, because in the rare case where the field being zeroed is not aligned to 4 bytes as required by the instruction, the assembler would issue an error message and fail the compile.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

CL 64970 OK for Go 1.9.2.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 15, 2017

Change https://golang.org/cl/70978 mentions this issue: [release-branch.go1.9] cmd/compile: fix regression in PPC64.rules move zero

kolyshkin added a commit to kolyshkin/golang that referenced this issue Oct 25, 2017

Go 1.9.1: backport ppc64le fix
Please see golang/go#21947 and/or
patch commit message for details.

NOTE: this needs to be reverted once we update to Go 1.9.2.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

gopherbot pushed a commit that referenced this issue Oct 25, 2017

[release-branch.go1.9] cmd/compile: fix regression in PPC64.rules mov…
…e zero

When a MOVDstorezero (8 bytes) is used the offset field
in the instruction must be a multiple of 4. This situation
had been corrected in the rules for other types of stores
but not for the zero case.

This also removes some of the special MOVDstorezero cases since
they can be handled by the general LowerZero case.

Updates made to the ssa test for lowering zero moves to
include cases where the target is not aligned to at least 4.

Fixes #21947

Change-Id: I7cceceb1be4898c77cd3b5e78b58dce0a7e28edd
Reviewed-on: https://go-review.googlesource.com/64970
Run-TryBot: Lynn Boger <laboger@linux.vnet.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/70978
Run-TryBot: Russ Cox <rsc@golang.org>
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

go1.9.2 has been packaged and includes:

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:09:16 UTC

@rsc rsc closed this Oct 26, 2017

@golang golang locked and limited conversation to collaborators Oct 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.