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
Closed

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

rgdoliveira opened this issue Sep 20, 2017 · 12 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone

Comments

@rgdoliveira
Copy link

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 20, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Sep 20, 2017
@ianlancetaylor
Copy link
Contributor

CC @laboger

@gopherbot
Copy link

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

@laboger
Copy link
Contributor

laboger 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
Copy link
Contributor

laboger 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
@laboger
Copy link
Contributor

laboger commented Oct 2, 2017

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

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Is it bound with #22047 in any way?

@laboger
Copy link
Contributor

laboger 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.

@ianlancetaylor ianlancetaylor added release-blocker and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 13, 2017
@rsc
Copy link
Contributor

rsc commented Oct 13, 2017

CL 64970 OK for Go 1.9.2.

@rsc rsc added the CherryPickApproved Used during the release process for point releases label Oct 14, 2017
@gopherbot
Copy link

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
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
…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
Copy link
Contributor

rsc 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 as completed 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.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants