We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 2620 - solaris port missing atomics if not using gcc
Summary: solaris port missing atomics if not using gcc
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: atomic (show other bugs)
Version: 2.0.0
Hardware: All Solaris
: P2 major
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-02 01:21 UTC by binarycrusader
Modified: 2014-07-07 17:40 UTC (History)
0 users

See Also:

Attachments
solaris atomics support when not using gcc (7.51 KB, patch)
2014-07-03 02:11 UTC, binarycrusader 		Details | Diff
solaris atomics support when not using gcc (7.54 KB, patch)
2014-07-05 23:14 UTC, binarycrusader 		Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description binarycrusader 2014-07-02 01:21:15 UTC 
Since changeset 358696c354a8, SDL 2.0 has essentially been broken on Solaris when compiling with the Solaris Studio compiler (which uses the pthread implementation of SDL_AtomicLock) because the pthread-based spinlock support was never fully implemented (see bug 2618) and atomic support is not implemented for the Solaris platform.

Use of Solaris Studio compilers is desirable when it comes to SPARC as gcc performance is relatively poor compared to the Solaris Studio compilers, and is often better than gcc on x86.

This bug is to track the addition of atomics support for Solaris which will likely be done using the compiler-agnostic atomic.h standard header file in Solaris since version 10:

Standard C Library Functions                       atomic_add(3C)

NAME
     atomic_add,  atomic_add_8,  atomic_add_char,  atomic_add_16,
     atomic_add_short,       atomic_add_32,       atomic_add_int,
     atomic_add_long,       atomic_add_64,        atomic_add_ptr,
     atomic_add_8_nv,    atomic_add_char_nv,    atomic_add_16_nv,
     atomic_add_short_nv,  atomic_add_32_nv,   atomic_add_int_nv,
     atomic_add_long_nv,  atomic_add_64_nv,  atomic_add_ptr_nv  -
     atomic add operations
Comment 1 binarycrusader 2014-07-02 01:21:54 UTC 
I'll provide a patch soon, this is not a request for the developers to implement themselves!
Comment 2 binarycrusader 2014-07-02 05:42:40 UTC 
Solaris Studio compiler references for compiler barriers:

http://www.oracle.com/technetwork/server-storage/solarisstudio/documentation/oss-compiler-barriers-176055.pdf

...and memory fences and memory barriers:

http://www.oracle.com/technetwork/server-storage/solarisstudio/documentation/oss-memory-barriers-fences-176056.pdf

References for Solaris native memory barrier interfaces:

http://www.unix.com/man-page/OpenSolaris/9f/membar_consumer/
Comment 3 binarycrusader 2014-07-03 02:09:42 UTC 
Alright, I have a working atomic implementation for Solaris when not using gcc:

$ ./testatomic 
INFO: 
spin lock---------------------------------------

INFO: AtomicLock                   lock=1
INFO: AtomicUnlock                 lock=0
INFO: 
atomic -----------------------------------------

INFO: AtomicSet(10)        tfret=TRUE val=10
INFO: AtomicAdd(10)        tfret=FALSE val=20
INFO: AtomicIncRef()       tfret=TRUE val=1
INFO: AtomicIncRef()       tfret=TRUE val=2
INFO: AtomicDecRef()       tfret=FALSE val=1
INFO: AtomicDecRef()       tfret=FALSE val=0
INFO: AtomicCAS()          tfret=TRUE val=10
INFO: AtomicCAS()          tfret=TRUE val=20
INFO: 
epic test---------------------------------------

INFO: Size asserted to be >= 32-bit
INFO: Check static initializer
INFO: Test negative values
INFO: Verify maximum value
INFO: Test compare and exchange
INFO: Test Add
INFO: Test Add (Negative values)
INFO: Reset before count down test
INFO: Counting down from 2147483647, Expect 47 remaining
INFO: Thread subtracting 100 10737418 times
INFO: Thread subtracting 100 10737418 times
INFO: Finished in 0.600000 sec
INFO: Atomic 47 Non-Atomic 662054347
INFO: 
FIFO test---------------------------------------

INFO: Mode: Mutex
INFO: Starting 4 readers
INFO: Starting 4 writers
INFO: Finished in 1.989000 sec
INFO: 
INFO: Writer 0 wrote 1000000 events, had 126953 waits
INFO: Writer 1 wrote 1000000 events, had 63130 waits
INFO: Writer 2 wrote 1000000 events, had 106669 waits
INFO: Writer 3 wrote 1000000 events, had 68188 waits
INFO: Writers wrote 4000000 total events
INFO: 
INFO: Reader 0 read 813760 events, had 23350 waits
INFO:   { 150961, 183206, 299678, 179915 }
INFO: Reader 1 read 1195736 events, had 45272 waits
INFO:   { 340375, 309743, 206178, 339440 }
INFO: Reader 2 read 815857 events, had 20744 waits
INFO:   { 167507, 189029, 285866, 173455 }
INFO: Reader 3 read 1174647 events, had 42712 waits
INFO:   { 341157, 318022, 208278, 307190 }
INFO: Readers read 4000000 total events
INFO: 
FIFO test---------------------------------------

INFO: Mode: LockFree
INFO: Starting 4 readers
INFO: Starting 4 writers
INFO: Finished in 0.787000 sec
INFO: 
INFO: Writer 0 wrote 1000000 events, had 516 waits
INFO: Writer 1 wrote 1000000 events, had 899 waits
INFO: Writer 2 wrote 1000000 events, had 875 waits
INFO: Writer 3 wrote 1000000 events, had 0 waits
INFO: Writers wrote 4000000 total events
INFO: 
INFO: Reader 0 read 1012443 events, had 71221 waits
INFO:   { 258346, 258274, 236799, 259024 }
INFO: Reader 1 read 1036510 events, had 68317 waits
INFO:   { 257074, 254655, 275780, 249001 }
INFO: Reader 2 read 1016088 events, had 71329 waits
INFO:   { 258316, 259286, 238192, 260294 }
INFO: Reader 3 read 934959 events, had 65052 waits
INFO:   { 226264, 227785, 249229, 231681 }
INFO: Readers read 4000000 total events
Comment 4 binarycrusader 2014-07-03 02:11:23 UTC 
Created attachment 1725 [details]
solaris atomics support when not using gcc

Also fixes build failure due to redundant -no-undefined in Makefile.in and incorrect platform check for Solaris that should check __SVR4 in addition to __sun.
Comment 5 binarycrusader 2014-07-03 04:22:07 UTC 
I've tested linux build after applying the attached patch; everything seems fine still.

Please note that the attached patch assumes that the patch for bug 2618 was applied first.

They're not technically dependent on each other since the first just deletes code, but this one might not apply cleanly without it.
Comment 6 binarycrusader 2014-07-05 22:07:20 UTC 
Comment on attachment 1725 [details]
solaris atomics support when not using gcc

Ooops; I missed the fact that some of the results in testatomic were actually wrong (I was expect it to exit with failure).

Specifically, I didn't realise that the AtomicAdd function was supposed to return the *previous* value instead of the new value.
Comment 7 binarycrusader 2014-07-05 23:14:48 UTC 
Created attachment 1735 [details]
solaris atomics support when not using gcc

Now actually passes testatomic (return value is previous value for SDL_AtomicAdd instead of previous value).  Again, this was due to the unexpected behaviour of testatomic not failing if previous value is not returned and I missed that:

INFO: 
spin lock---------------------------------------

INFO: AtomicLock                   lock=1
INFO: AtomicUnlock                 lock=0
INFO: 
atomic -----------------------------------------

INFO: AtomicSet(10)        tfret=TRUE val=10
INFO: AtomicAdd(10)        tfret=TRUE val=20
INFO: AtomicIncRef()       tfret=TRUE val=1
INFO: AtomicIncRef()       tfret=TRUE val=2
INFO: AtomicDecRef()       tfret=TRUE val=1
INFO: AtomicDecRef()       tfret=TRUE val=0
INFO: AtomicCAS()          tfret=TRUE val=10
INFO: AtomicCAS()          tfret=TRUE val=20
INFO: 
epic test---------------------------------------

INFO: Size asserted to be >= 32-bit
INFO: Check static initializer
INFO: Test negative values
INFO: Verify maximum value
INFO: Test compare and exchange
INFO: Test Add
INFO: Test Add (Negative values)
INFO: Reset before count down test
INFO: Counting down from 2147483647, Expect 47 remaining
INFO: Thread subtracting 100 10737418 times
INFO: Thread subtracting 100 10737418 times
INFO: Finished in 0.703000 sec
INFO: Atomic 47 Non-Atomic 863928447
INFO: 
FIFO test---------------------------------------

INFO: Mode: Mutex
INFO: Starting 4 readers
INFO: Starting 4 writers
INFO: Finished in 2.073000 sec
INFO: 
INFO: Writer 0 wrote 1000000 events, had 50449 waits
INFO: Writer 1 wrote 1000000 events, had 52441 waits
INFO: Writer 2 wrote 1000000 events, had 3274 waits
INFO: Writer 3 wrote 1000000 events, had 52781 waits
INFO: Writers wrote 4000000 total events
INFO: 
INFO: Reader 0 read 1308624 events, had 141510 waits
INFO:   { 396818, 371875, 217309, 322622 }
INFO: Reader 1 read 1230506 events, had 151150 waits
INFO:   { 357893, 315857, 256915, 299841 }
INFO: Reader 2 read 672899 events, had 130716 waits
INFO:   { 121218, 137596, 221141, 192944 }
INFO: Reader 3 read 787971 events, had 153719 waits
INFO:   { 124071, 174672, 304635, 184593 }
INFO: Readers read 4000000 total events
INFO: 
FIFO test---------------------------------------

INFO: Mode: LockFree
INFO: Starting 4 readers
INFO: Starting 4 writers
INFO: Finished in 0.792000 sec
INFO: 
INFO: Writer 0 wrote 1000000 events, had 23031 waits
INFO: Writer 1 wrote 1000000 events, had 27192 waits
INFO: Writer 2 wrote 1000000 events, had 25128 waits
INFO: Writer 3 wrote 1000000 events, had 19426 waits
INFO: Writers wrote 4000000 total events
INFO: 
INFO: Reader 0 read 1135708 events, had 20304 waits
INFO:   { 284329, 283726, 284818, 282835 }
INFO: Reader 1 read 1087647 events, had 21484 waits
INFO:   { 272969, 268527, 273379, 272772 }
INFO: Reader 2 read 645245 events, had 18298 waits
INFO:   { 158509, 164122, 158612, 164002 }
INFO: Reader 3 read 1131400 events, had 20425 waits
INFO:   { 284193, 283625, 283191, 280391 }
INFO: Readers read 4000000 total events
Comment 8 Sam Lantinga 2014-07-07 17:40:33 UTC 
Fixed, thanks!
https://hg.libsdl.org/SDL/rev/1e283b7a1580

I didn't accept the top level Makefile.in change, and the change to testatomic.c looked wrong to me, but everything else looks good. If you need either of the two changes I didn't accept, go ahead and submit a new bug for each of them.

Thanks!