View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0022980CommunityOCCT:Foundation Classespublic2012-02-07 12:332013-04-29 15:21
ReporterRoman Lygin Assigned ToRoman Lygin  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
PlatformAOSL 
Product Version6.5.2 
Target Version6.6.0Fixed in Version6.6.0 
Summary0022980: Fixed Standard_Atomic.hxx
DescriptionIn 6.5.0 and 6.5.1 Standard_Atomic_Increment() on gcc/Linux (and MacOS) lacked
clobber statement "memory" what resulted in wrong results when working in highly
contended mode. Also Standard_Atomic_Decrement() used wrong int volatile* which
also resulted in wrong result in highly contended mode.

The fix includes merge with 6.5.2 and the following:
- correct use of volatile int* instead of int volatile*;
- use of __GNUC__ macros instead of LIN (which is non-standard OCC-specific macro), to also enable use on MacOS/Intel;
- minimized code (use of fetch-and-add semantic Standard_Atomic_Add() which is called by Increment() and Decrement()) inspired by boost/atomic.hpp. Performance impact should be marginal, if at all comparing to code in 6.5.2 (using xaddl and incl);
- note that built-in atomic routines are available as of gcc4.1, even if macro
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is not defined but we do not use it (see comments in Standard_Atomic.hxx for reasoning);
- minor grammar mistakes

The fix can be applied to 6.5.0, 6.5.1, 6.5.2.

Reproducer:
see Standard_AtomicTest
Steps To ReproduceSee Standard_Atomic_Test.cxx
TagsNo tags attached.
Test case numberbugs fclasses(002) bug22980

Attached Files

  • Standard_Atomic_fix.zip (2,147 bytes)

Relationships

parent of 0023657 closeddbv Open CASCADE Fails compilation with WOK after patch 22980 

Activities

Roman Lygin

2012-02-07 12:33

developer  

Standard_Atomic_fix.zip (2,147 bytes)

kgv

2012-08-14 09:43

developer   ~0021235

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

Roman Lygin

2012-11-28 16:36

developer   ~0022413

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.

kgv

2012-12-02 17:50

developer   ~0022499

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

mkv

2012-12-07 14:52

tester   ~0022624

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

abv

2012-12-11 00:56

manager   ~0022642

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

mkv

2012-12-12 09:44

tester   ~0022665

Last edited: 2012-12-12 09:46

 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

abv

2012-12-12 10:36

manager   ~0022667

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

mkv

2012-12-18 15:15

tester   ~0022745

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.

Related Changesets

occt: master 1365140b

2012-12-18 18:35:43

Roman Lygin

Details Diff		 0022980: Fixed Standard_Atomic.hxx

Standard_Atomic, prefer gcc built-ins rather than WinAPI calls (revert previous change of order)
Raise compiler error if no implementation found for atomic operations.
Add new draw command for testing this bug
Fix for compilation with TBB disabled
Adding CSF_TBB_INCLUDES macro in edl files
Correction of *.edl files
Update of EXTERNLIB files with CSF_TBB
Adding EXTERNLIB file		 Affected Issues
0022980
mod - src/Draw/Draw_CMPLRS.edl Diff File
mod - src/Draw/EXTERNLIB Diff File
add - src/QABugs/EXTERNLIB Diff File
mod - src/QABugs/FILES Diff File
mod - src/QANCollection/QANCollection1.cxx Diff File
mod - src/QANCollection/QANCollection2.cxx Diff File
mod - src/QANCollection/QANCollection3.cxx Diff File
mod - src/Standard/Standard_Atomic.hxx Diff File
mod - src/Standard/Standard_CMPLRS.edl Diff File
mod - src/TKDraw/EXTERNLIB Diff File
mod - src/TKQADraw/EXTERNLIB Diff File
add - tests/bugs/fclasses/bug22980 Diff File

Issue History

Date Modified Username Field Change
2012-02-07 12:33 Roman Lygin New Issue
2012-02-07 12:33 Roman Lygin Assigned To => abv
2012-02-07 12:33 Roman Lygin File Added: Standard_Atomic_fix.zip
2012-05-04 13:00 abv Status new => assigned
2012-08-14 09:10 kgv Assigned To abv => Roman Lygin
2012-08-14 09:43 kgv Note Added: 0021235
2012-10-24 17:32 abv Target Version => 6.6.0
2012-11-28 16:36 Roman Lygin Note Added: 0022413
2012-11-28 16:36 Roman Lygin Assigned To Roman Lygin => kgv
2012-11-28 16:36 Roman Lygin Status assigned => resolved
2012-12-02 17:50 kgv Note Added: 0022499
2012-12-02 17:50 kgv Assigned To kgv => bugmaster
2012-12-02 17:50 kgv Status resolved => reviewed
2012-12-03 18:58 mkv Assigned To bugmaster => mkv
2012-12-07 14:52 mkv Note Added: 0022624
2012-12-07 14:53 mkv Test case number => bugs fclasses(002) bug22980
2012-12-07 14:53 mkv Assigned To mkv => kgv
2012-12-07 14:53 mkv Status reviewed => assigned
2012-12-10 16:19 abv Assigned To kgv => abv
2012-12-11 00:56 abv Note Added: 0022642
2012-12-11 00:56 abv Assigned To abv => mkv
2012-12-11 00:56 abv Status assigned => resolved
2012-12-11 00:56 abv Status resolved => reviewed
2012-12-12 09:44 mkv Note Added: 0022665
2012-12-12 09:45 mkv Assigned To mkv => abv
2012-12-12 09:45 mkv Status reviewed => assigned
2012-12-12 09:46 mkv Note Edited: 0022665
2012-12-12 10:36 abv Note Added: 0022667
2012-12-12 10:36 abv Status assigned => resolved
2012-12-12 10:36 abv Assigned To abv => mkv
2012-12-12 10:36 abv Status resolved => reviewed
2012-12-18 15:15 mkv Note Added: 0022745
2012-12-18 15:15 mkv Assigned To mkv => bugmaster
2012-12-18 15:15 mkv Status reviewed => tested
2012-12-21 15:58 Roman Lygin Changeset attached => occt master 1365140b
2012-12-21 15:58 Roman Lygin Assigned To bugmaster => Roman Lygin
2012-12-21 15:58 Roman Lygin Status tested => verified
2012-12-21 15:58 Roman Lygin Resolution open => fixed
2013-01-09 02:51 abv Relationship added parent of 0023657
2013-04-23 13:36 aiv Status verified => closed
2013-04-29 15:21 aiv Fixed in Version => 6.6.0