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

crypto/sha256: escape analysis of digest.Sum disagrees on different architectures #48055

Closed
dsnet opened this issue Aug 30, 2021 · 4 comments
Closed

crypto/sha256: escape analysis of digest.Sum disagrees on different architectures #48055

dsnet opened this issue Aug 30, 2021 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Unplanned

Comments

@dsnet
Copy link
Member

dsnet commented Aug 30, 2021 •
edited

It seems to me that the result of escape analysis should be independent of the exact target architecture. This may be an assumption that is totally unfounded as my understanding of the escape analyzer is limited.

To reproduce, try building crypto/sha256 on various architectures

for X in 386 amd64 amd64p32 arm armbe arm64 arm64be ppc64 ppc64le mips mipsle mips64 mips64le mips64p32 mips64p32le ppc riscv riscv64 s390 s390x sparc sparc64 wasm; do
    echo $X
    GOARCH=$X ../../../bin/go build -gcflags="-m" 2>&1 | egrep "d0|tmp"
   echo
done

The following architectures believe that d0 and tmp escape to the heap:

arm
ppc64
mips
mipsle
mips64
mips64le
riscv64

while the following do not:

386
amd64
amd64p32
armbe
arm64
arm64be
ppc64le
mips64p32
mips64p32le
ppc
riscv
s390
s390x
sparc
sparc64
wasm

Using GOARCH=arm as an example, -gcflags="-m=100 prints the relevant information:

./sha256.go:209:5:[1] (*digest).Sum stmt: d0 := *d
./sha256.go:209:2:[1] (*digest).Sum stmt: var d0 digest
./sha256.go:210:7:[1] (*digest).Sum stmt: hash := (*digest).checkSum(d0)
./sha256.go:210:2:[1] (*digest).Sum stmt: var hash [32]byte
./sha256.go:209:2: d0 escapes to heap:
./sha256.go:209:2:   flow: {heap} = &d0:
./sha256.go:209:2:     from d0 (address-of) at ./sha256.go:210:12
./sha256.go:209:2:     from (*digest).checkSum(d0) (call parameter) at ./sha256.go:210:21
./sha256.go:211:2:[1] (*digest).Sum stmt: if d0.is224 { return append(in, hash[:Size224]...) }
./sha256.go:212:3:[1] (*digest).Sum stmt: return append(in, hash[:Size224]...)
./sha256.go:214:2:[1] (*digest).Sum stmt: return append(in, hash[:]...)
./sha256.go:207:22: parameter in leaks to ~r0 with derefs=0:
./sha256.go:207:22:   flow: ~r0 = in:
./sha256.go:207:22:     from append(in, hash[:Size224]...) (call parameter) at ./sha256.go:212:16
./sha256.go:207:22:     from return append(in, hash[:Size224]...) (return) at ./sha256.go:212:3
./sha256.go:207:7: d does not escape
./sha256.go:207:22: leaking param: in to result ~r0 level=0
./sha256.go:209:2: moved to heap: d0

./sha256.go:220:6: tmp escapes to heap:
./sha256.go:220:6:   flow: {heap} = &tmp:
./sha256.go:220:6:     from tmp (address-of) at ./sha256.go:223:22
./sha256.go:220:6:     from tmp[0:56 - len % 64] (slice) at ./sha256.go:223:14
./sha256.go:220:6:     from (*digest).Write(d, tmp[0:56 - len % 64]) (call parameter) at ./sha256.go:223:10
./sha256.go:225:10:[1] (*digest).checkSum stmt: (*digest).Write(d, tmp[0:64 + 56 - len % 64])
./sha256.go:220:6: tmp escapes to heap:
./sha256.go:220:6:   flow: {heap} = &tmp:
./sha256.go:220:6:     from tmp (address-of) at ./sha256.go:225:25
./sha256.go:220:6:     from tmp[0:64 + 56 - len % 64] (slice) at ./sha256.go:225:14
./sha256.go:220:6:     from (*digest).Write(d, tmp[0:64 + 56 - len % 64]) (call parameter) at ./sha256.go:225:10

The tangible effect of this discrepancy is that sha256.digest.Sum always allocates on certain architectures.
dsnet added a commit to tailscale/tailscale that referenced this issue Aug 30, 2021
@dsnet
util/deephash: fix TestArrayAllocs
d8b9a0c 
Unfortunately this test fails on certain architectures.
The problem comes down to inconsistencies in the Go escape analysis
where specific variables are marked as escaping on certain architectures.
The variables escaping to the heap are unfortunately in crypto/sha256,
which makes it impossible to fixthis locally in deephash.

For now, fix the test by compensating for the allocations that
occur from calling sha256.digest.Sum.

See golang/go#48055

Fixes #2727

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
@dsnet dsnet mentioned this issue Aug 30, 2021
Merged
dsnet added a commit to tailscale/tailscale that referenced this issue Aug 30, 2021
@dsnet
util/deephash: fix TestArrayAllocs
26c6db1 
Unfortunately this test fails on certain architectures.
The problem comes down to inconsistencies in the Go escape analysis
where specific variables are marked as escaping on certain architectures.
The variables escaping to the heap are unfortunately in crypto/sha256,
which makes it impossible to fixthis locally in deephash.

For now, fix the test by compensating for the allocations that
occur from calling sha256.digest.Sum.

See golang/go#48055

Fixes #2727

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
@cuonglm
Copy link
Member

cuonglm commented Aug 30, 2021

This is because on these systems:

arm
ppc64
mips
mipsle
mips64
mips64le
riscv64

They use the generic version, which make the *digest escape to heap.

Other architectures has their own implementation, which does not, e.g, for arm64

go/src/crypto/sha256/sha256block_arm64.go

Line 12 in 56c3856

func sha256block(h []uint32, p []byte, k []uint32)

@randall77
Copy link
Contributor

This is kind of unfortunate, because the digest clearly does not escape in blockGeneric. The problem is in sha256block_generic.go, where we do

var block = blockGeneric

That makes callers of block not know the target function, and thus has to assume escaping arguments.

It would be better to use a wrapper

func block(dig *digest, p []byte) { blockGeneric(dig, p) }

The wrapper will get inlined, and escape analysis will see the flow properly.

@dsnet
Copy link
Member Author

dsnet commented Aug 30, 2021 •
edited

Thanks @cuonglm and @randall77. It seems the issue is more specific to crypto/sha256 than it is to cmd/compile. Renaming the issue.

@dsnet dsnet changed the title cmd/compile: escape analysis disagrees on different architectures crypto/sha256: escape analysis of digest.Sum disagrees on different architectures Aug 30, 2021
@cuonglm cuonglm self-assigned this Aug 30, 2021
@gopherbot
Copy link

Change https://golang.org/cl/346209 mentions this issue: crypto/sha256: avoid escaping on generic architectures

@cuonglm cuonglm removed their assignment Aug 30, 2021
dsnet added a commit to tailscale/tailscale that referenced this issue Aug 30, 2021
@dsnet
util/deephash: fix TestArrayAllocs
3f1317e 
Unfortunately this test fails on certain architectures.
The problem comes down to inconsistencies in the Go escape analysis
where specific variables are marked as escaping on certain architectures.
The variables escaping to the heap are unfortunately in crypto/sha256,
which makes it impossible to fixthis locally in deephash.

For now, fix the test by compensating for the allocations that
occur from calling sha256.digest.Sum.

See golang/go#48055

Fixes #2727

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
dsnet added a commit to tailscale/tailscale that referenced this issue Aug 30, 2021
@dsnet
util/deephash: fix TestArrayAllocs
15835f0 
Unfortunately this test fails on certain architectures.
The problem comes down to inconsistencies in the Go escape analysis
where specific variables are marked as escaping on certain architectures.
The variables escaping to the heap are unfortunately in crypto/sha256,
which makes it impossible to fixthis locally in deephash.

For now, fix the test by compensating for the allocations that
occur from calling sha256.digest.Sum.

See golang/go#48055

Fixes #2727

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
(cherry picked from commit 3f1317e)
@cherrymui cherrymui added this to the Unplanned milestone Aug 31, 2021
@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 31, 2021
@golang golang locked and limited conversation to collaborators Sep 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@cuonglm @dsnet @randall77 @gopherbot @cherrymui