-
Notifications
You must be signed in to change notification settings - Fork 125
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
Flaky "unsafe pointer conversion"-panics with -race and go1.14rc1 #29
Comments
I suspect the problem is that there is no guarantee that the alignment is correct after the pointer conversion here: Line 68 in 539464a
|
This test reproduces it reliably:
(Also one of the existing string tests) |
This removes the incorrect unsafe usage, instead using the marshalling operations from encoding/binary. Obviously, it's a bit slower: benchmark old ns/op new ns/op delta Benchmark32/1-8 4.68 5.02 +7.26% Benchmark32/2-8 5.15 5.16 +0.19% Benchmark32/4-8 5.05 5.56 +10.10% Benchmark32/8-8 6.10 6.95 +13.93% Benchmark32/16-8 8.18 9.94 +21.52% Benchmark32/32-8 12.5 15.4 +23.20% Benchmark32/64-8 20.7 26.7 +28.99% Benchmark32/128-8 38.3 55.9 +45.95% Benchmark32/256-8 75.8 94.1 +24.14% Benchmark32/512-8 150 191 +27.33% Benchmark32/1024-8 299 375 +25.42% Benchmark32/2048-8 586 730 +24.57% Benchmark32/4096-8 1170 1450 +23.93% Benchmark32/8192-8 2347 2888 +23.05% BenchmarkPartial32/8-8 157 155 -1.27% BenchmarkPartial32/16-8 185 184 -0.54% BenchmarkPartial32/32-8 243 240 -1.23% BenchmarkPartial32/64-8 201 206 +2.49% BenchmarkPartial32/128-8 223 240 +7.62% Benchmark64/1-8 12.3 12.2 -0.81% Benchmark64/2-8 12.8 12.9 +0.78% Benchmark64/4-8 14.2 13.9 -2.11% Benchmark64/8-8 16.1 15.5 -3.73% Benchmark64/16-8 13.7 15.3 +11.68% Benchmark64/32-8 16.2 19.2 +18.52% Benchmark64/64-8 20.3 26.4 +30.05% Benchmark64/128-8 28.8 40.8 +41.67% Benchmark64/256-8 47.3 68.8 +45.45% Benchmark64/512-8 81.8 126 +54.03% Benchmark64/1024-8 151 242 +60.26% Benchmark64/2048-8 291 477 +63.92% Benchmark64/4096-8 566 948 +67.49% Benchmark64/8192-8 1115 1873 +67.98% Benchmark128/1-8 12.9 12.9 +0.00% Benchmark128/2-8 13.2 13.5 +2.27% Benchmark128/4-8 14.3 14.2 -0.70% Benchmark128/8-8 15.9 16.1 +1.26% Benchmark128/16-8 13.6 16.0 +17.65% Benchmark128/32-8 15.9 20.0 +25.79% Benchmark128/64-8 20.5 26.9 +31.22% Benchmark128/128-8 29.1 41.2 +41.58% Benchmark128/256-8 46.5 69.6 +49.68% Benchmark128/512-8 81.0 126 +55.56% Benchmark128/1024-8 150 244 +62.67% Benchmark128/2048-8 292 471 +61.30% Benchmark128/4096-8 566 921 +62.72% Benchmark128/8192-8 1115 1868 +67.53% Nonetheless, it's correct and doesn't crash.
This removes the incorrect unsafe usage, instead using the marshalling operations from encoding/binary. Obviously, it's a bit slower: benchmark old ns/op new ns/op delta Benchmark32/1-8 4.68 5.02 +7.26% Benchmark32/2-8 5.15 5.16 +0.19% Benchmark32/4-8 5.05 5.56 +10.10% Benchmark32/8-8 6.10 6.95 +13.93% Benchmark32/16-8 8.18 9.94 +21.52% Benchmark32/32-8 12.5 15.4 +23.20% Benchmark32/64-8 20.7 26.7 +28.99% Benchmark32/128-8 38.3 55.9 +45.95% Benchmark32/256-8 75.8 94.1 +24.14% Benchmark32/512-8 150 191 +27.33% Benchmark32/1024-8 299 375 +25.42% Benchmark32/2048-8 586 730 +24.57% Benchmark32/4096-8 1170 1450 +23.93% Benchmark32/8192-8 2347 2888 +23.05% BenchmarkPartial32/8-8 157 155 -1.27% BenchmarkPartial32/16-8 185 184 -0.54% BenchmarkPartial32/32-8 243 240 -1.23% BenchmarkPartial32/64-8 201 206 +2.49% BenchmarkPartial32/128-8 223 240 +7.62% Benchmark64/1-8 12.3 12.2 -0.81% Benchmark64/2-8 12.8 12.9 +0.78% Benchmark64/4-8 14.2 13.9 -2.11% Benchmark64/8-8 16.1 15.5 -3.73% Benchmark64/16-8 13.7 15.3 +11.68% Benchmark64/32-8 16.2 19.2 +18.52% Benchmark64/64-8 20.3 26.4 +30.05% Benchmark64/128-8 28.8 40.8 +41.67% Benchmark64/256-8 47.3 68.8 +45.45% Benchmark64/512-8 81.8 126 +54.03% Benchmark64/1024-8 151 242 +60.26% Benchmark64/2048-8 291 477 +63.92% Benchmark64/4096-8 566 948 +67.49% Benchmark64/8192-8 1115 1873 +67.98% Benchmark128/1-8 12.9 12.9 +0.00% Benchmark128/2-8 13.2 13.5 +2.27% Benchmark128/4-8 14.3 14.2 -0.70% Benchmark128/8-8 15.9 16.1 +1.26% Benchmark128/16-8 13.6 16.0 +17.65% Benchmark128/32-8 15.9 20.0 +25.79% Benchmark128/64-8 20.5 26.9 +31.22% Benchmark128/128-8 29.1 41.2 +41.58% Benchmark128/256-8 46.5 69.6 +49.68% Benchmark128/512-8 81.0 126 +55.56% Benchmark128/1024-8 150 244 +62.67% Benchmark128/2048-8 292 471 +61.30% Benchmark128/4096-8 566 921 +62.72% Benchmark128/8192-8 1115 1868 +67.53% Nonetheless, it's correct and doesn't crash.
Sigh. go1.14 is officially out (yay), and
Sigh. Sorry it's getting late, that was completely wrong, let me try again. :D
I was going for something like this but checkptr still doesn't like it. nblocks := len(p) / 4
for i := 0; i < nblocks; i++ {
- k1 := *(*uint32)(unsafe.Pointer(&p[i*4]))
+ k1 := *(*uint32)(unsafe.Pointer(uintptr(unsafe.Pointer(&p[0])) + uintptr(i*4)*unsafe.Sizeof(&p[0])))
|
The check enforces this:
Any given byte is not going to be properly aligned for uint32 or uint64 so we can’t assume that we can do that conversion unfortunately. |
Fundamentally, the unsafe code needs changing. Feel free to take my patches: twmb/murmur3@b0c3ada...HEAD Note that using binary.LittleEndian (or my path) will resolve this repos tickets around issues across architectures, as well. |
That patch looks better than mine, which was just a quick fix. We might end up depending on your repo instead, @twmb. :) |
not merged. So, I've switched my code to github.com/DataDog/mmh3 |
The fork of @twmb seems quite nice: https://github.com/twmb/murmur3 |
FWIW DataDog/mmh3 theoretically same unaligned input problem, but I think it goes through unsafe & reflect.SliceHeader in a roundabout enough way that the new checks don't catch it: h := *(*reflect.SliceHeader)(unsafe.Pointer(&key))
h.Len = nblocks * 2
b := *(*[]uint64)(unsafe.Pointer(&h)) |
seems commit 229247d33b has already solved this problem, maybe we can close this issue? |
Likely, but for what it's worth only the warning was removed, the underlying issue the warning was about still exists. Given that nobody complained about the issue in years, it may not a large issue. "The main benefit of the check is to catch people making alignment assumptions on x86, where their program would then fail on another architecture. That just doesn't seem that much of a pain point to me. Even if it fails on another architecture, at least it does so loudly, without silently corrupting data" |
I am facing similar issue when migrating to go1.14.x fatal error: checkptr: unsafe pointer arithmetic goroutine 26 [running]: |
This is still broken in go1.15.5 darwin/amd64 for me. In particular, murmur32 is tripping checkptr even when the pointer is actually aligned. Seems like a bug in Go to me. |
I am facing the same issue
|
@sheregeda did you read the thread? Especially #29 (comment)
|
I cannot reproduce the panic below with a minimal example and it occurs very often, but neither all the time nor in the same test - so this report might be entirely unhelpful, but I am reporting it anyway in case it makes any sense to you:
When running the Syncthing database testsuite on go1.14rc1 with race detection enabled on linux/amd64 I get the following panic:
(rest of the backtrace does not include murmur3 nor bloom).
Do you have any ideas what this might be about? Thanks in advance for any help.
The text was updated successfully, but these errors were encountered: