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: unaligned accesses on mips64 #46283

Closed
4a6f656c opened this issue May 20, 2021 · 10 comments
Closed

cmd/compile: unaligned accesses on mips64 #46283

4a6f656c opened this issue May 20, 2021 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@4a6f656c
Copy link
Contributor

Both test/abi/leaf.go and test/abi/spills4.go currently SIGBUS on openbsd/mips64:

##### ../test                                    
# go run run.go -- abi/leaf.go
exit status 2                                                                                                     
unexpected fault address 0xc0000bcf34
fatal error: fault
[signal SIGBUS: bus error code=0x1 addr=0xc0000bcf34 pc=0x12e5c]

...

# go run run.go -- abi/spills4.go
exit status 2
unexpected fault address 0xc0000bcf34
fatal error: fault
[signal SIGBUS: bus error code=0x1 addr=0xc0000bcf34 pc=0x12e5c]

...

This is due to an unaligned access, resulting from an ld instruction with an address that is not a multiple of 8:

0000000000012e58 <runtime.memequal128>:
   12e58:       dfa10008        ld      at,8(sp)
   12e5c:       dc220000        ld      v0,0(at)

I've bisected this to:

[84ca4949a71554265b3c8a99359a5fad6ca0cab1] cmd/compile: remove 8-byte alignment requirement of stack slot on mips64

Prior to that commit test/abi/leaf.go (although introduced later on) builds and runs, after that commit it fails. Note that this test probably passes on linux/mips64, although there will be a performance penalty due to the kernel handling the unaligned access fault. Unfortunately we do not run test on the openbsd/mips64 builder for performance reasons.

@4a6f656c
Copy link
Contributor Author

/cc @randall77 @cherrymui @erifan

@cherrymui
Copy link
Member

Thanks. Maybe we want to check alignment in https://tip.golang.org/src/cmd/compile/internal/reflectdata/alg.go#L47 for architectures that cannot do unaligned memory access. Or maybe we could use readUnaligned32/64 in memequalNN functions on those architectures.

@erifan
Copy link

erifan commented May 21, 2021

Thanks. Maybe we want to check alignment in https://tip.golang.org/src/cmd/compile/internal/reflectdata/alg.go#L47 for architectures that cannot do unaligned memory access.

I don't understand this piece of code, my initial idea was to revert CL 297771, but I don’t know if it is feasible to do so now. I don't have a test environment, and the Upstream machine seems to be very busy, I'm also not familiar with mips64. If you are free, could you help fix it? Thank you.

Or maybe we could use readUnaligned32/64 in memequalNN functions on those architectures.

I feel that the first method is better, because there may be similar codes elsewhere.

@mengzhuo
Copy link
Contributor

FYI: I've run this test on linux/mips64x 1000 times and all passes.

@randall77
Copy link
Contributor

This program should fail on a machine that forbids unaligned accesses, regardless of CL 297771. @4a6f656c could you try it on your machine?

package main

type T struct {
	a, b          int16
	c, d, e       int32
	r, s, t, u, v float32
}

//go:noinline
func f(p, q *T) bool {
	return *p == *q
}

type U struct {
	x int32
	t T
}

func main() {
	var t T
	var u U
	sink = &t
	sink = &u.t
	println(f(&t, &u.t))
}

var sink interface{}

@randall77
Copy link
Contributor

The other option to fix this is to forbid merging field comparisons in the generated eq functions, on architectures which can't do unaligned loads. Or do the merge but call memequal128_unaligned or some such on those architectures.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 21, 2021
@dmitshur dmitshur added this to the Go1.17 milestone May 21, 2021
@4a6f656c
Copy link
Contributor Author

FYI: I've run this test on linux/mips64x 1000 times and all passes.

Thanks - I expected that to be the case. My understanding is that the Linux kernel will silently patch up unaligned accesses, which means it will have a performance impact (e.g. one ld instruction will result in an unaligned access fault, a trap into the kernel, a series of instructions to copy the data, then returning to the user process - https://elixir.bootlin.com/linux/latest/source/arch/mips/kernel/unaligned.c). I'm not sure if the Linux mips64 kernel has /proc/cpu/alignment or another way of confirming that this is happening.

@4a6f656c
Copy link
Contributor Author

This program should fail on a machine that forbids unaligned accesses, regardless of CL 297771. @4a6f656c could you try it on your machine?

@randall77 - confirmed, this results in a SIGBUS with both an older and newer version of Go:

$ /usr/local/go.old/bin/go version 
go version devel +25e066a54d Tue Jul 21 18:08:14 2020 +1000 openbsd/mips64
$ /usr/local/go.old/bin/go build -o c c.go 
$ ./c                                      
unexpected fault address 0xc00000e0c4
fatal error: fault
[signal SIGBUS: bus error code=0x1 addr=0xc00000e0c4 pc=0x11e44]
...
$ /usr/local/go/bin/go version             
go version devel go1.17-15a374d5c1 Wed May 19 01:09:20 2021 +0000 openbsd/mips64
$ /usr/local/go/bin/go build -o c c.go     
$ ./c
unexpected fault address 0xc0000220c4
fatal error: fault
[signal SIGBUS: bus error code=0x1 addr=0xc0000220c4 pc=0x11f44]
...

So I guess we've just exposed a pre-existing bug :)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/322150 mentions this issue: cmd/compile: always include underlying type for map types

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/322151 mentions this issue: cmd/compile: ensure equal functions don't do unaligned loads

gopherbot pushed a commit that referenced this issue May 24, 2021
This is a different fix for #37716.

Should help make the fix for #46283 easier, since we will no longer
need to keep compiler-generated hash functions and the runtime
hash function in sync.

Change-Id: I84cb93144e425dcd03afc552b5fbd0f2d2cc6d39
Reviewed-on: https://go-review.googlesource.com/c/go/+/322150
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants