Fixed Standard_Atomic.hxx

Dear Roman,

could you please update patch according to new rules (in git branch)?

>>Also Standard_Atomic_Decrement() used wrong int volatile*
>> which also resulted in wrong result in highly contended mode.
Could you please clarify this?
_InterlockedIncrement() definition exactly fits Microsoft headers (volatile keyword usage, see WinBase.h - checked in Visual Studio 2008/2010).
Moreover it looks like "volatile int* " and "int volatile* " means the same thing (but not "int* volatile ").

>>inline int Standard_Atomic_DecrementTest (volatile int* theValue)
Standard_Atomic_DecrementTest() function was removed because it is pointless. Is it really so useful alias to be widely used in application code?

>> #elif defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
>> // || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 1)
Modern gcc SHOULD define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 to indicate that current platform supports atomic operations on 32bit integers.
If we want to support these built-in functions in older versions of gcc we should check something like this instead:
>> #elif defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)
>> || (__GNUC__ == 4 && __GNUC_MINOR__ >= 1 && __GNUC_MINOR__ < 4)
However current build scripts do not add -march=i486 argument on 32-bit Linux so it will result in linker errors.
But probably this is more correct to update our building procedures too
(this is pointless to support ancient i386 today).

>>use of fetch-and-add semantic Standard_Atomic_Add()
>>int Standard_Atomic_Add (volatile int* theValue, int theVal)
First, function entirely defined in header and should have inline keyword to avoid compiler/linker errors.
Second - it is misconception to declare global function implemented only in one rare case. It should be either implemented for other scenarios (using __sync_sub_and_fetch() / WinAPI) or its definition should be hidden to user.

More hints:
- Functions duplicated several times for each implementation. This is more convenient (more compact, less duplication, slightly less error-prone) to use platform-dependent macros inside function body instead.
- Use MacOS X functions in case when gcc built-ins unavailable:
#if defined(__APPLE__)
  #include <libkern/OSAtomic.h>

  int Standard_Atomic_Increment (volatile int* theValue)
  {
    return OSAtomicIncrement32Barrier(theValue);
  }

  int Standard_Atomic_Decrement (volatile int* theValue)
  {
    return OSAtomicDecrement32Barrier (theValue);
  }
#endif

Pushed branch CR22980 into the git repository


Kirill, thanks a lot for detailed comments and sorry for delayed response on my side.
Here are responses.

1. Windows:

volatile long* vs long volatile*.
There is apparently inconsistency in MSFT itself.

WinBase.h (in Windows SDK 7.0) has:

InterlockedIncrement (
    __inout LONG volatile *lpAddend
    );
see also http://msdn.microsoft.com/en-us/library/windows/desktop/ms683614%28v=vs.85%29.aspx
(although this is not intrinsic function)

but the compiler VS2008 has both:
in intrin.h:
__MACHINEI(long __cdecl _InterlockedIncrement(long volatile *))

in <memory>:
extern "C" long __CLRCALL_PURE_OR_CDECL _InterlockedIncrement(volatile long *);
extern "C" long __CLRCALL_PURE_OR_CDECL _InterlockedDecrement(volatile long *);
extern "C" long __CLRCALL_PURE_OR_CDECL _InterlockedCompareExchange(volatile long *,
    long, long);

 #pragma intrinsic(_InterlockedIncrement)
 #pragma intrinsic(_InterlockedDecrement)
 #pragma intrinsic(_InterlockedCompareExchange)

Thus, it seems it does not matter and either is fine. If you feel uncomfortable with current 'volatile long*' change otherwise.


2. OK to remove Standard_Decrement_Test(). I will make my own copy as needed.

3. __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
Let's keep the current version as is, i.e. to only check for explicit presence of __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4. Thus, the modification will preserve current OCC behavior.
Adding -march should be a separate issue.

I have removed confusing // || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 1).
but left the rest of the comment for information, if anyone tries to use these built-in functions on earlier gcc versions.

4. Standard_Atomic_Add()
4a. Added inline - initially was my oversight, sorry about that. This was a copy past of non-inlined functions in Standard_Atomic.hxx - note missing 'inline' there too ;-).
4b. There is no "misconception" - Standard_Atomic_Add() is used by Standard_Atomic_Increment() and Standard_Atomic_Decrement() which are defined in this very file. Please elaborate if you meant something different.

5. Hints
a. Code structure. I intentionally separated implementations per OSes/compilers for better code clarity instead of putting everything inside the same function. Single line function is better to read than contamination of platform-specific codes, when only a declaration line is common. Another advantage is that you can put comments common for both Increment() and Decrement() only once at the top of the specific section.
However this can be subjective, so I'd delegate final resolution to ABV.

b. MacOS.
Thanks, I have added these to be chosen if gcc built-ins are not available (which is likely for MacOS 10.x given that gcc version is < 4.4). But this was not tested on my side. Please review and amend during real port.



Thanks again for detailed comments. Hope these amendments address your concerns. Please let me know if there is anything you would like to follow up on.

Patch is ready for testing.
Please check compilation on Mac OS X as well.

Dear BugMaster,
Branch CR22980 (and products from GIT master) was compiled on Linux and
Windows platforms and tested.

Regressions:
Not detected

Improvements:
Not detected

Testing cases:
bugs fclasses(002) bug22980 - KO.
http://occt-tests/CR22980-master-occt/Windows-32-VC9/bugs/fclasses/bug22980.html

The corrected test is pushed to CR22980, please test and integrate

Dear BugMaster,

Branch CR22980 (and products from GIT master) was compiled on Linux and
Windows platforms.

There is compilation error on Linux:
../../../src/QABugs/QABugs_19.cxx:206: error: 'argv' was not declared in this scope

Fix for building with TBB disabled pushed to branch CR22980, please test. Please make sure to have TBB enabled for testing!

Dear BugMaster,

Branch CR22980 (and products from GIT master) was compiled on Linux and Windows platforms and tested.

Regressions:
Not detected

Improvements:
Not detected

Testing cases:
bugs fclasses(002) bug22980 - OK.