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

test_atomic fail with 'Saw release store out of order' on ppc64 #58

Open
ivmai opened this issue Oct 20, 2023 · 11 comments
Open

test_atomic fail with 'Saw release store out of order' on ppc64 #58

ivmai opened this issue Oct 20, 2023 · 11 comments

Comments

@ivmai
Copy link
Owner

ivmai commented Oct 20, 2023

Source: master (400a88b)
Build: https://app.travis-ci.com/github/ivmai/libatomic_ops/jobs/611939900
Host: Ubuntu 22.04.2 LTS / ppc64le
Compiler: gcc (Ubuntu 11.3.0-1ubuntu1~22.04.1) 11.3.0
Config: CMAKE_OPTIONS="-DCMAKE_BUILD_TYPE=MinSizeRel -Denable_assertions=ON -Denable_atomic_intrinsics=OFF"
Occurrence: ~3/4
Output (test_atomic):
Saw release store out of order (bad CSE?): 2026939 < 2027014

@ivmai
Copy link
Owner Author

ivmai commented Oct 20, 2023

Not observed on commit 9f6bc3b or earlier (build: https://app.travis-ci.com/github/ivmai/libatomic_ops/jobs/609611344).
Commit 418dfb5 seems to be the 1st one the issue could be reproduced at (with some occurrence rate).

@ivmai
Copy link
Owner Author

ivmai commented Nov 2, 2023

@ivmai
Copy link
Owner Author

ivmai commented Nov 4, 2023

@ivmai
Copy link
Owner Author

ivmai commented Nov 5, 2023

Source: master (42684a2)
Reproduced on cfarm203.
Compiler: gcc (Debian 13.2.0-6) 13.2.0

cd out && cmake -DCMAKE_BUILD_TYPE=MinSizeRel -Denable_assertions=ON -Denable_atomic_intrinsics=OFF -Dbuild_tests=ON -Denable_werror=ON -Werror=dev .. && cmake --build . && ctest

Or:
gcc -Isrc -D AO_DISABLE_GCC_ATOMICS -Os tests/test_atomic.c -lpthread && ./a.out

Or:
gcc -Isrc -D AO_DISABLE_GCC_ATOMICS -Os -D AO_PREFER_GENERALIZED -D AO_GENERALIZE_ASM_BOOL_CAS tests/test_atomic.c -lpthread && ./a.out

@ivmai
Copy link
Owner Author

ivmai commented Nov 5, 2023

Not reproduced with clang.
Not reproduced with -O0, -O1, -O2, -O3.

@ivmai
Copy link
Owner Author

ivmai commented Nov 7, 2023

Looks like an issues in the compiler (gcc), The potential workaround is to avoid "static" for counter1 variable (in tests/test_atomic.c). Diff of asm (generated by gcc -I src -D AO_DISABLE_GCC_ATOMICS -Os -S tests/test_atomic.c):

--- test_atomic.s
+++ test_atomic-after-workaround.s
@@ -470,7 +470,8 @@
 	cmpwi 3,3,1
 	.cfi_register 65, 0
 	.cfi_offset 70, 8
-	bl _savegpr0_28
+	bl _savegpr0_27
+	.cfi_offset 27, -40
 	.cfi_offset 28, -32
 	.cfi_offset 29, -24
 	.cfi_offset 30, -16
@@ -517,28 +518,29 @@
 	li 3,0
 	lwz 12,8(1)
 	mtcrf 24,12
-	b _restgpr0_28
+	b _restgpr0_27
 	.cfi_restore 31
 	.cfi_restore 30
 	.cfi_restore 29
 	.cfi_restore 28
+	.cfi_restore 27
 	.cfi_restore 65
 	.cfi_restore 70
 .L32:
 	.cfi_restore_state
 	mr 3,28
 	bl AO_load_acquire
-	ld 5,8(31)
+	ld 27,8(31)
 	mr 30,3
 	mr 3,28
-	std 5,112(1)
 	bl AO_load_acquire
-	ld 5,112(1)
+	cmpld 0,30,27
+	ld 5,8(31)
 	mr 6,3
-	cmpld 0,30,5
 	ble 0,.L35
 	addis 4,2,.LC7@toc@ha
 	mr 6,30
+	mr 5,27
 	addi 4,4,.LC7@toc@l
 .L40:
 	addis 9,2,.LC5@toc@ha
@@ -554,7 +556,7 @@
 	addi 4,4,.LC8@toc@l
 	b .L40
 	.long 0
-	.byte 0,0,0,3,128,4,0,0
+	.byte 0,0,0,3,128,5,0,0
 	.cfi_endproc
 .LFE57:
 	.size	acqrel_thr,.-.L.acqrel_thr
@@ -4960,6 +4962,7 @@
 .LFE62:
 	.size	main,.-.L.main
 	.globl junk
+	.globl counter1
 	.section	".data"
 	.align 3
 	.set	.LANCHOR1,. + 0

ivmai added a commit that referenced this issue Nov 7, 2023
(fix of commit 418dfb5)

Issue #58 (libatomic_ops).

* tests/test_atomic.c [(!AO_NO_PTHREADS || !AO_USE_PTHREAD_DEFS)
&& AO_HAVE_store_release_write && AO_HAVE_load_acquire_read
&& __powerpc64__ && !__clang__] (counter1): Do not define as static.
@ivmai
Copy link
Owner Author

ivmai commented Nov 7, 2023

Hello @sharkcz, could you please look at this issue? Seems to be a bug in gcc on ppc64.
I have prepared a W/A but i have not analyzed the asm code diff.

@sharkcz
Copy link

sharkcz commented Nov 7, 2023

Hi @ivmai , does it need some special options/defines set to expose the issue? Our CI using Fedora 38 with gcc-13.2.1-4.fc38.ppc64le (snapshot from the gcc13 branch) hasn't seen it.

@ivmai
Copy link
Owner Author

ivmai commented Nov 7, 2023

Exactly, you could reproduce it in 2 ways:
Type
git checkout 20390d7~1 && gcc -Isrc -D AO_DISABLE_GCC_ATOMICS -Os tests/test_atomic.c -lpthread && ./a.out

Or
git checkout 20390d7~1 && ./configure && make check CC=gcc CFLAGS_EXTRA="-D AO_DISABLE_GCC_ATOMICS -Os"

@sharkcz
Copy link

sharkcz commented Nov 7, 2023

Thanks, reproduced.

@ivmai
Copy link
Owner Author

ivmai commented May 21, 2024

Hello @sharkcz
Is there any feedback?

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

No branches or pull requests

2 participants