Invalid code generated using atomic

[stuartcarnie]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8beta2 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/stuartcarnie/projects/go/gam"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xc/rq4s_b_91r9ds09t6bm93t080000gn/T/go-build814602722=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

https://play.golang.org/p/nulQCpZ2nn

What did you expect to see?

I expected 1 from Pop

What did you see instead?

In the case of the playground, 0. From go 1.8 a segfault due to an invalid pointer. By switching the atomic.StoreUintptr with prev.next = n in Push, the issue goes away.

Generated assembly reveals it may be an issue with compiler not recognizing that n escapes the Push function

[bradfitz]

@stuartcarnie, is this a regression from Go 1.7?

/cc @randall77

[randall77]

@stuartcarnie the code in that playground link never calls Pop. Is that link wrong/old somehow?

[stuartcarnie]

@randall77 my mistake (copy / paste error); I've updated the link

[stuartcarnie]

Incidentally, this also works, by ensuring *node escapes

[stuartcarnie]

@bradfitz it is not a regression, as it fails in the playground too

[randall77]

Here is a smaller repro:

func f() {
	x = uintptr(unsafe.Pointer(new(int)))
}
func g() {
	atomic.StoreUintptr(&x, uintptr(unsafe.Pointer(new(int))))
}
var x uintptr

f allocates the int on the heap. g allocates it on the stack.

I don't think this is actually a bug. Converting an unsafe.Pointer to a uintptr is asking for all kinds of trouble. We guarantee that this conversion works only if you immediately (before any safepoint) convert back, like unsafe.Pointer(uintptr(p) + ...). More complicated things like what you're doing are asking all of our analyses to lose the pointer. I believe that's what is happening here for escape analysis. You're also missing the required write barriers by secretly writing pointers as uintptrs.

I believe the correct answer for your problem is to use atomic.SwapPointer instead of atomic.SwapUintptr. Then you're never storing a pointer in a uintptr and you won't be confusing anything.

func h() {
	atomic.StorePointer(&y, unsafe.Pointer(new(int)))
}
var y unsafe.Pointer

That works.

[stuartcarnie]

@randall77 I notice that atomic.SwapPointer and related atomic.*Pointer functions are not intrinsics per this.

I realize this is an implementation detail and it could be considered being pedantic, but any chance those will become intrinsic also?

[bradfitz]

I realize this is an implementation detail and it could be considered being pedantic, but any chance those will become intrinsic also?

They would need a write barrier (already a function call) anyway.

[stuartcarnie]

Ahh, I thought the compiler emitted a check for writeBarrier.enabled and only called runtime.writebarrierptr during GC mark and GC mark termination phases (or certain other debugging situations where enabled would always be on)

[josharian]

Perhaps they could be intrinsified a bit as:

if runtime.writebarrierEnabled {
  write barrier call
} else {
  intrinsic swap
}

That'd make the wb-disabled case a bit faster at the cost of increased code size. Not sure how much it'd help or whether the trade-off is worth it.

[josharian]

Also, if the result of the swap is stored somewhere, then the swap itself should be GC-neutral, since both pointers are stored somewhere before and after. That might be a bit too fiddly, though.

[bradfitz]

@josharian, @stuartcarnie, feel free to file a tracking bug at least. Closed bugs aren't the best place to discuss.

[randall77]

We could intrinsify them so that in the common case that the write barrier is off, they would be a test/branch/store. Could be done, but I don't think it's high on anyone's priority list.
Possibly inlining improvements will do that for us. We would need mid-stack inlining implemented first (#10152).

[stuartcarnie]

apologies @bradfitz – do you mean a separate tracking bug for intrinsic support of atomic.*Pointer?

[bradfitz]

Yes.