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

runtime: can't atomic access of first word of tiny-allocated struct on 32-bit architecture #37262

Open
NewbMiao opened this issue Feb 17, 2020 · 10 comments
Milestone

Comments

@NewbMiao
Copy link

@NewbMiao NewbMiao commented Feb 17, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.7 darwin/amd64
$ uname -a
Darwin newbmiaodeMacBook-Pro.local 17.7.0 Darwin Kernel Version 17.7.0: Sun Jun  2 20:31:42 PDT 2019; root:xnu-4570.71.46~1/RELEASE_X86_64 x86_64

Does this issue reproduce with the latest release?

Not sure

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/newbmiao/Library/Caches/go-build"
GOENV="/Users/newbmiao/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/newbmiao/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.7/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/m_/hlhv5wl17vg368420bbxszhc0000gn/T/go-build908799930=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

This issue about test the spec below, but got panic:

https://golang.org/pkg/sync/atomic/#pkg-note-BUG
The first word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned.

I use code below to test, and will panic occasionally
(Test it on my 64-bit mac: GOARCH=386 go build -ldflags=-compressdwarf=false -o test test.go)

But if i remove the fmt.Println() inside of the test function, it will pass.

I also debug it use gdb (append info below), x escape to heap, and use tinyallocs to alloc it

And i notice thatc.tinyoffset is 2.

But when no fmt.Println() it will be 0 or 16, Or just nextFreeFast to get a new object.

So i am confused, is that spec work in this situation?
Or maybe i misunderstand with this spec?
Or a issue cross compile under macOS?

May related with this issue: #599

Code:

package main
import (
	"fmt"
	"sync/atomic"
)

// On ARM, x86-32, and 32-bit MIPS, it is the caller's responsibility to arrange for 64-bit alignment of 64-bit words accessed atomically.
// The first word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned.
func test(){
	x := struct {
		v uint64
		y uint32
	}{}
	
	fmt.Println()  //if comment this line, it will pass on 32-bit arch
	atomic.AddUint64(&x.v, 1)
}
func main() {
	test()
}

gdb info

$ GOARCH=386 go build -ldflags=-compressdwarf=false -o test test.go
$ gdb ./hello
(gdb) b main.main
Thread 2 hit Breakpoint 1, main.main ()
    at /Dig101-Go/test.go:19
19      func main() {
(gdb) s
20              test()
(gdb) 
main.test () at /Dig101-Go/test.go:10
10      func test(){
(gdb) 
11              x := struct {
(gdb) 
runtime.newobject (typ=0x9d280 <type.*+93280>, ~r1=<optimized out>)
    at /usr/local/Cellar/go/1.13.7/libexec/src/runtime/malloc.go:1150
1150    func newobject(typ *_type) unsafe.Pointer {
(gdb) 
1151            return mallocgc(typ.size, typ, true)
(gdb) 
runtime.mallocgc (size=<optimized out>, typ=0x9d280 <type.*+93280>, needzero=true, 
    ~r3=<optimized out>)
    at /usr/local/Cellar/go/1.13.7/libexec/src/runtime/malloc.go:877
877     func mallocgc(size uintptr, typ *_type, needzero bool) unsafe.Pointer {
(gdb) 
878             if gcphase == _GCmarktermination {
(gdb) until 945
runtime.mallocgc (size=<optimized out>, typ=0x9d280 <type.*+93280>, needzero=true, ~r3=<optimized out>)
    at /usr/local/Cellar/go/1.13.7/libexec/src/runtime/malloc.go:945
945             if size <= maxSmallSize {
(gdb) n
946                     if noscan && size < maxTinySize {
(gdb) 
976                             off := c.tinyoffset
(gdb) 
978                             if size&7 == 0 {
(gdb) 
980                             } else if size&3 == 0 {
(gdb) 
981                                     off = round(off, 4)
(gdb) 
985                             if off+size <= maxTinySize && c.tiny != 0 {
(gdb) p off
$1 = 4
(gdb) p c.tinyoffset  <========= notice this line
$2 = 2
(gdb) n
987                                     x = unsafe.Pointer(c.tiny + off)
(gdb) 
988                                     c.tinyoffset = off + size
(gdb) 
989                                     c.local_tinyallocs++
(gdb) 
990                                     mp.mallocing = 0
(gdb) 
991                                     releasem(mp)
(gdb) 
992                                     return x
(gdb) 
main.test () at /Dig101-Go/test.go:16
16              fmt.Println() 
(gdb) 

17              atomic.AddUint64(&x.v, 1)
(gdb) 

Thread 2 received signal SIGSEGV, Segmentation fault.
runtime/internal/atomic.Xadd64 () at /usr/local/Cellar/go/1.13.7/libexec/src/runtime/internal/atomic/asm_386.s:105
105             MOVL    0, AX // crash when unaligned
(gdb) 
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2f7c]

goroutine 1 [running]:
runtime/internal/atomic.Xadd64(0x1141c0b4, 0x1, 0x0, 0x0, 0x0)
        /usr/local/Cellar/go/1.13.7/libexec/src/runtime/internal/atomic/asm_386.s:105 +0xc
main.test()
        /Dig101-Go/test.go:17 +0x72
main.main()
        /Dig101-Go/test.go:20 +0x11
[Inferior 1 (process 68214) exited with code 02]

What did you expect to see?

go run successfully

What did you see instead?

Panic sometimes:

$ ./test  # success

$ ./test # failed

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2f7c]

goroutine 1 [running]:
runtime/internal/atomic.Xadd64(0x1147e004, 0x1, 0x0, 0x0, 0x0)
        /usr/local/Cellar/go/1.13.7/libexec/src/runtime/internal/atomic/asm_386.s:105 +0xc
main.test()
        /Dig101-Go/test.go:17 +0x72
main.main()
        /Dig101-Go/test.go:20 +0x11
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 17, 2020

@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Feb 17, 2020
@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Feb 17, 2020

Maybe I'm missing something, but this looks like a malloc issue, not a compiler issue.

cc @mknyszek @aclements

@mknyszek

This comment has been minimized.

Copy link
Contributor

@mknyszek mknyszek commented Feb 18, 2020

@aclements Looks like the tiny allocator doesn't uphold the guarantee in the spec. The lines are:

			// Align tiny pointer for required (conservative) alignment.
			if size&7 == 0 {
				off = round(off, 8)
			} else if size&3 == 0 {
				off = round(off, 4)
			} else if size&1 == 0 {
				off = round(off, 2)
			}

In this case size is 12, and so we align to 4 (as the trace in the original post at the top shows), but it should be 8, because it contains an 8-byte field (right?). I don't think this is even just a 386 problem; we could end up misaligning in any case.

Maybe the fix is to just align to typ.align?

@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Feb 18, 2020

@mknyszek would that make the tiny allocator much less efficient, by increasing the number and size of gaps? And if so, are there mitigations?

@mknyszek

This comment has been minimized.

Copy link
Contributor

@mknyszek mknyszek commented Feb 18, 2020

@mknyszek would that make the tiny allocator much less efficient, by increasing the number and size of gaps? And if so, are there mitigations?

Maybe, but given that we don't have atomic types and instead have atomic operations, it's impossible to know whether we really need the full alignment, and so we must conservatively always assume we need it. In that sense, I can't think of any mitigations besides waiting for #36606 which may allow users to set a custom alignment (thereby signaling the alignment they actually need).

It might also be slower because we have to touch the type whereas we didn't have to before.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Feb 18, 2020

typ.align doesn't help in the current way. On 32-bit architectures it is 4 for int64. This is what #36606 is for.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Feb 18, 2020

I think this can only happen for a size 9-15 object on a 32-bit architecture. Maybe we round up for only this case. I don't think it is too bad.

@cherrymui cherrymui changed the title cmd/compile: cant atomic access of first word of allocated struct occasionally? runtime: can't atomic access of first word of tiny-allocated struct Feb 18, 2020
@cherrymui cherrymui changed the title runtime: can't atomic access of first word of tiny-allocated struct runtime: can't atomic access of first word of tiny-allocated struct on 32-bit architecture Feb 18, 2020
@aclements

This comment has been minimized.

Copy link
Member

@aclements aclements commented Feb 18, 2020

Since a type's size is rounded up to its alignment, I think this specific case with a 12 byte struct on 32-bit is really the only way that this can happen, and it's a direct consequence of the underalignment of uint64.

/cc @danscales for an interesting consequence of the current alignment rules

@mknyszek

This comment has been minimized.

Copy link
Contributor

@mknyszek mknyszek commented Feb 20, 2020

@aclements @cherrymui Got it. I missed the fact that the size is rounded up. That makes sense, and I was surprised that this wasn't more broken...

@josharian It looks like what I was proposing is already there, so there shouldn't be any packing problems.

It looks like this problem is wholly #36606 as @cherrymui mentioned, so we might want to just close this as a duplicate?

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Feb 20, 2020

It looks like this problem is wholly #36606 as @cherrymui mentioned, so we might want to just close this as a duplicate?

Not exactly. Fixing #36606 will make this go away, but we can also fix this separately. This issue is specific to the first word of an allocation, which we try to make it still 8-byte aligned, as mentioned in https://golang.org/pkg/sync/atomic/#pkg-note-BUG . Currently it is mostly aligned, except for tinyalloc (and sbrk mode).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.