Open GWRon opened 1 year ago
I already asked this on discord but maybe it fits here too:
in Brl.Treads AtomicSwap()
is defined:
Function AtomicSwap:Int( target:Int Var,value:Int )
Repeat
Local oldval:Int=target
If CompareAndSwap( Varptr target,oldval,value ) Return oldval
Forever
End Function
What happens if a second thread manipulates target
after it is stored in Local oldVal
but before executing CompareAndSwap
?
Asking as CompareAndSwap is called with "oldVal" rather than "target" (which would use the "original" value then).
This means if something changed "target" meanwhile, the Repeat ... Forever
might never exit.
This minimizes the chances I guess.
Function AtomicSwap:Int( target:Int Var,value:Int )
Repeat
Local oldval:Int=target
If CompareAndSwap( Varptr target,target,value ) Return oldval
Forever
End Function
BUT ... the issue here might remain: If someone really wants to use the returned "oldVal" it might NOT be the one which really was replaced. (this is why there is not just __sync_bool_compare_and_swap
(the one used on non-apple) but also __sync_value_compare_and_swap
Also I am not sure what GCC creates out of the C-Code, as the function indirection might lead to more temporary variables and thus ways for other threads to manipulate the "original" meanwhile.
The generated C code for now looks like this:
BBINT brl_threads_CompareAndSwap(BBINT* bbt_target,BBINT bbt_oldValue,BBINT bbt_newValue){
return bbAtomicCAS((int*)&(*bbt_target),(int)bbt_oldValue,(int)bbt_newValue);
}
BBINT brl_threads_AtomicSwap(BBINT* bbt_target,BBINT bbt_value){
do{
BBINT bbt_oldval=*bbt_target;
if(brl_threads_CompareAndSwap(&(*bbt_target),bbt_oldval,bbt_value)!=0){
return bbt_oldval;
}
}while(!(0));
}
directly calling bbAtomicCas:
Function AtomicSwap:Int( target:Int Var,value:Int )
Repeat
Local oldval:Int=target
If bbAtomicCAS( Varptr target,target,value ) Return oldval
Forever
End Function
BBINT brl_threads_AtomicSwap(BBINT* bbt_target,BBINT bbt_value){
do{
BBINT bbt_oldval=*bbt_target;
if(bbAtomicCAS((int*)&(*bbt_target),(int)*bbt_target,(int)bbt_value)!=0){
return bbt_oldval;
}
}while(!(0));
}
Dunno if the GCC optimizes it to the same final assembler code at the end.
We could always force a minimum of C11 and implement support for C11's "atomic" primitives...
Rather than "Int", we'd need to define the variable as an atomic int, eg "AtomicInt", which you'd need to use instead for atomic operations. But then it would be guaranteed to work correctly.
Most important is..if the MacOS stuff will rather sooner than later require a change.
So before it surprises you..maybe be prepared.
Regarding atomicint. I think an bbAtomicSet( variable:int var, value:int ) would already help. What it does in the background is maybe not that important
It might even be interesting if it needs something for reading too (depending on memory layout the integer could be a 2 asm command thing..right?) Might with overloading be interesting for :long too.
Before introducing c11 stuff (I understand that you are a bit eager to try it out :-)) it might be worth to check if the generated code (asm by gcc) uses multiple operations to in that CAS stuff.
If this stuff is not that atomic as we thought... we need to also ensure that TCondVar etc are not relying on such a nonatomic thing.
Well, there's nothing to directly replace it with on macOS, since the preferred replacement is to use atomic_fetch_add
, which expects something like an atomic_int
.
Seems gcc offers atomic functions since 4.8 https://gcc.gnu.org/onlinedocs/gcc-4.8.2/gcc/_005f_005fatomic-Builtins.html
So doesn't that mean we could use that (for non macos targets)? The atomic store variant sounds also interesting (set without compare)
Regarding "C11" ... You made some efforts to have it "C99" compatible (or so) to support old linux boxes with old GCC. It might be potentially useful to have something working for them too.
Hmm ... but it might mean to keep it "potentially incorrect" as it is now.
Couldn't we introduce "Atomic" as a keyword?
Field Atomic i:int
Local Atomic i:int
Asking as introducing int_atomic
might be simpler but I am not sure how often other stuff would be needed too (atomic types ).
Anyways. If you add some kind of int_atomic
(or atomic_int
to have it similar to the C naming scheme) do not forget about long
, float
and double
and other >=32bit variants.
While checking for ways to avoid mutexes when simply manipulating integers I saw there is "bbAtomicAdd".
Implementation is:
But OSAtomicAdd32 is deprecated by Apple:
I am not sure if we can - and should - already replace it with the suggested
atomic_fetch_add