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.

Sign up for GitHub

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

Jump to bottom

cmd/vet/all: false positive when taking address of return value on s390x and arm #25822

Closed
mundaym opened this issue Jun 11, 2018 · 4 comments
Closed

cmd/vet/all: false positive when taking address of return value on s390x and arm #25822

mundaym opened this issue Jun 11, 2018 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@mundaym
Copy link
Member

mundaym commented Jun 11, 2018

CL 114397 will add several lines to cmd/vet/all/whitelist/s390x.txt to suppress the following errors:

internal/cpu/cpu_s390x.s:9: [s390x] stfle: invalid MOVD of ret+0(FP); cpu.facilityList is 32-byte value
internal/cpu/cpu_s390x.s:18: [s390x] kmQuery: invalid MOVD of ret+0(FP); cpu.queryResult is 16-byte value
internal/cpu/cpu_s390x.s:25: [s390x] kmcQuery: invalid MOVD of ret+0(FP); cpu.queryResult is 16-byte value
internal/cpu/cpu_s390x.s:32: [s390x] kmctrQuery: invalid MOVD of ret+0(FP); cpu.queryResult is 16-byte value
internal/cpu/cpu_s390x.s:39: [s390x] kmaQuery: invalid MOVD of ret+0(FP); cpu.queryResult is 16-byte value
internal/cpu/cpu_s390x.s:46: [s390x] kimdQuery: invalid MOVD of ret+0(FP); cpu.queryResult is 16-byte value
internal/cpu/cpu_s390x.s:53: [s390x] klmdQuery: invalid MOVD of ret+0(FP); cpu.queryResult is 16-byte value

The assembly in question takes the address of the return value, which is an 8-byte pointer, so these errors are wrong. The assembly triggering these errors looks like: MOVD $ret+0(FP), R1 (the $ means address of).
@mundaym mundaym added this to the Unplanned milestone Jun 11, 2018
@mundaym mundaym added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 11, 2018
@gopherbot
Copy link

Change https://golang.org/cl/114397 mentions this issue: crypto, internal/cpu: fix s390x AES feature detection and update SHA implementations

gopherbot pushed a commit that referenced this issue Jun 11, 2018
@mundaym
crypto, internal/cpu: fix s390x AES feature detection and update SHA …
7ba0c62 
…implementations

Hardware AES support in Go on s390x currently requires ECB, CBC
and CTR modes be available. It also requires that either the
GHASH or GCM facilities are available. The existing checks missed
some of these constraints.

While we're here simplify the cpu package on s390x, moving masking
code out of assembly and into Go code. Also, update SHA-{1,256,512}
implementations to use the cpu package since that is now trivial.

Finally I also added a test for internal/cpu on s390x which loads
/proc/cpuinfo and checks it against the flags set by internal/cpu.

Updates #25822 for changes to vet whitelist.

Change-Id: Iac4183f571643209e027f730989c60a811c928eb
Reviewed-on: https://go-review.googlesource.com/114397
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@mundaym
crypto, internal/cpu: fix s390x AES feature detection and update SHA …
567b737 
…implementations

Hardware AES support in Go on s390x currently requires ECB, CBC
and CTR modes be available. It also requires that either the
GHASH or GCM facilities are available. The existing checks missed
some of these constraints.

While we're here simplify the cpu package on s390x, moving masking
code out of assembly and into Go code. Also, update SHA-{1,256,512}
implementations to use the cpu package since that is now trivial.

Finally I also added a test for internal/cpu on s390x which loads
/proc/cpuinfo and checks it against the flags set by internal/cpu.

Updates golang#25822 for changes to vet whitelist.

Change-Id: Iac4183f571643209e027f730989c60a811c928eb
Reviewed-on: https://go-review.googlesource.com/114397
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@mundaym
crypto, internal/cpu: fix s390x AES feature detection and update SHA …
db6c5da 
…implementations

Hardware AES support in Go on s390x currently requires ECB, CBC
and CTR modes be available. It also requires that either the
GHASH or GCM facilities are available. The existing checks missed
some of these constraints.

While we're here simplify the cpu package on s390x, moving masking
code out of assembly and into Go code. Also, update SHA-{1,256,512}
implementations to use the cpu package since that is now trivial.

Finally I also added a test for internal/cpu on s390x which loads
/proc/cpuinfo and checks it against the flags set by internal/cpu.

Updates golang#25822 for changes to vet whitelist.

Change-Id: Iac4183f571643209e027f730989c60a811c928eb
Reviewed-on: https://go-review.googlesource.com/114397
Run-TryBot: Michael Munday <mike.munday@ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/164623 mentions this issue: all: move internal/x to vendor/golang.org/x and revendor using 'go mod vendor'

gopherbot pushed a commit that referenced this issue Mar 11, 2019
all: move internal/x to vendor/golang.org/x and revendor using 'go mo…
c5cf662 
…d vendor'

This also updates the vendored-in versions of several packages: 'go
mod vendor' selects a consistent version of each module, but we had
previously vendored an ad-hoc selection of packages.

Notably, x/crypto/hkdf was previously vendored in at a much newer
commit than the rest of x/crypto. Bringing the rest of x/crypto up to
that commit introduced an import of golang.org/x/sys/cpu, which broke
the js/wasm build, requiring an upgrade of x/sys to pick up CL 165749.

Updates #30228
Updates #30241
Updates #25822

Change-Id: I5b3dbc232b7e6a048a158cbd8d36137af1efb711
Reviewed-on: https://go-review.googlesource.com/c/go/+/164623
Reviewed-by: Filippo Valsorda <filippo@golang.org>
@tklauser tklauser changed the title cmd/vet/all: false positive when taking address of return value on s390x cmd/vet/all: false positive when taking address of return value on s390x and arm Mar 12, 2019
@tklauser
Copy link
Member

tklauser commented Mar 12, 2019 •
edited

Similar whitelist entries had to be added on ARM with https://golang.org/cl/166677 for

internal/bytealg/equal_arm.s:17: [arm] Equal: invalid MOVW of ret+24(FP); bool is 1-byte value

@gopherbot
Copy link

Change https://golang.org/cl/176097 mentions this issue: go/analysis/passes: fix bugs discovered in std

@golang golang locked and limited conversation to collaborators May 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@tklauser @gopherbot @mundaym