[Bf-committers] Different behavior of our atomic operation depending on compiler

[Sergey Sharybin]

it's not so crucial to have atm, but nice to solve it still.

We can reformulate out usage of atomic_cas and make it return non-zero if
value changed, which would simply mean we need to update non-OSX versions?

Other thing is that update_maximum() from guardedalloc is not really atomic
as well, that would also need to be addressed.

On Wed, Jul 1, 2015 at 12:39 PM, Bastien Montagne <montagne29 at wanadoo.fr>
wrote:

> Ah yes indeed, thanks!
>
> Also, now in the 'cas' case on OSX, we have this code:
>
>      uint64_t init_val = *v;
>      OSAtomicCompareAndSwap64((int64_t)old, (int64_t)_new, (int64_t *)v);
>      return init_val;
>
> (because OSX has no primitive returning original value of 'v').
>
> As far as I can see, this operation is no more strictly atomic:
> * thread A sets its init_val
> * thread B calls OSAtomicCompareAndSwap64() and change value of v
> * thread A calls OSAtomicCompareAndSwap64() based on 'new' v value
> * thread A returns invalid orginal value...
>
> Probably not a crucial issue currently?
>
>
> Le 01/07/2015 11:36, Brecht Van Lommel a écrit :
> > "InterlockedExchangeAdd(p, x) + x" works exactly the same as"
> > __sync_add_and_fetch(p, x)", no need to use asm.
> >
> > On Wed, Jul 1, 2015 at 9:53 AM, Bastien Montagne <montagne29 at wanadoo.fr>
> wrote:
> >> So, trying to hunt an strange issue with Windows scons builds of
> >> filebrowser work, I found that our attomic operations (defined in
> >> intern/atomic/atomci_ops.h) do not return the same things on MSVC than
> >> with other OS/compilers.
> >>
> >> With GCC & co we use `__sync_add_and_fetch`, on OSX, `OSAtomicAdd64`,
> >> with MSVC `InterlockedExchangeAdd64`, and have a fallback implementation
> >> in plain assembler.
> >>
> >> According to documentations (and my limited asm understanding), MSVC
> >> version returns the value of `*p` *before* the operation, while all
> >> others return it *after* the operation (i.e. return the result of the
> >> operation).
> >>
> >>
> https://gcc.gnu.org/onlinedocs/gcc-5.1.0/gcc/_005f_005fsync-Builtins.html
> >>
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms683599(v=vs.85).aspx
> >>
> https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/OSAtomicAdd64.3.html
> >>
> >> I doubt this is desired behavior, and think it could easily generate
> >> nasty bug some day… MSVC does not seem to have a 'return value after
> >> operation' variant from quick look at the doc? Maybe we should just
> >> remove the MSVC case completely and use asm version instead?
> >>
> >> Bastien
> >> _______________________________________________
> >> Bf-committers mailing list
> >> Bf-committers at blender.org
> >> http://lists.blender.org/mailman/listinfo/bf-committers
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers at blender.org
> > http://lists.blender.org/mailman/listinfo/bf-committers
>
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>



-- 
With best regards, Sergey Sharybin