0025545: TopLoc_Location::Transformation() provokes data races

Get rid of postponed calculation of transformation.
Remove unused methods.
Add command OCC25545 to reproduce the bug with data races.

- Get rid of C++11 lambda construction
- make code compilable with no HAVE_TBB defined
- add test case bugs/fclasses/bug25545

src/QABugs/QABugs_19.cxx

@@ -49,6 +49,7 @@
 #include <TCollection_HAsciiString.hxx>
 #include <GeomFill_Trihedron.hxx>
 #include <BRepOffsetAPI_MakePipe.hxx>
+#include <Standard_Atomic.hxx>
 
 #include <Standard_Version.hxx>
 
@@ -158,7 +159,6 @@ static Standard_Integer OCC23237 (Draw_Interpretor& di, Standard_Integer /*argc*
 
 #ifdef HAVE_TBB
 
-#include <Standard_Atomic.hxx>
 #include <tbb/blocked_range.h>
 #include <tbb/parallel_for.h>
 
@@ -3126,6 +3126,76 @@ static Standard_Integer OCC25446 (Draw_Interpretor& theDI,
   return 0;
 }
 
+//====================================================
+// Auxiliary functor class for the command OCC25545;
+// it gets access to a vertex with the given index and
+// checks that X coordinate of the point is equal to index;
+// if it is not so then a data race is reported.
+//====================================================
+struct OCC25545_Functor
+{
+  OCC25545_Functor(const std::vector<TopoDS_Shape>& theShapeVec)
+    : myShapeVec(&theShapeVec),
+      myIsRaceDetected(0)
+  {}
+
+  void operator()(size_t i) const
+  {
+    if (!myIsRaceDetected) {
+      const TopoDS_Vertex& aV = TopoDS::Vertex (myShapeVec->at(i));
+      gp_Pnt aP = BRep_Tool::Pnt (aV);
+      if (aP.X () != static_cast<double> (i)) {
+        Standard_Atomic_Increment(&myIsRaceDetected);
+      }
+    }
+  }
+
+  const std::vector<TopoDS_Shape>* myShapeVec;
+  mutable volatile int myIsRaceDetected;
+};
+
+//=======================================================================
+//function : OCC25545
+//purpose  : Tests data race when concurrently accessing TopLoc_Location::Transformation()
+//=======================================================================
+#ifdef HAVE_TBB
+static Standard_Integer OCC25545 (Draw_Interpretor& di, 
+                                  Standard_Integer, 
+                                  const char **)
+{
+  // Place vertices in a vector, giving the i-th vertex the
+  // transformation that translates it on the vector (i,0,0) from the origin.
+  size_t n = 1000;
+  std::vector<TopoDS_Shape> aShapeVec (n);
+  std::vector<TopLoc_Location> aLocVec (n);
+  TopoDS_Shape aShape = BRepBuilderAPI_MakeVertex (gp::Origin ());
+  aShapeVec[0] = aShape;
+  for (size_t i = 1; i < n; ++i) {
+    gp_Trsf aT;
+    aT.SetTranslation (gp_Vec (1, 0, 0));
+    aLocVec[i] = aLocVec[i - 1] * aT;
+    aShapeVec[i] = aShape.Moved (aLocVec[i]);
+  }
+
+  // Evaluator function will access vertices geometry
+  // concurrently
+  OCC25545_Functor aFunc(aShapeVec);
+
+  //concurrently process
+  tbb::parallel_for (size_t (0), n, aFunc, tbb::simple_partitioner ());
+  QVERIFY (!aFunc.myIsRaceDetected);
+  return 0;
+}
+#else
+static Standard_Integer OCC25545 (Draw_Interpretor&, 
+                                  Standard_Integer, 
+                                  const char **argv)
+{
+  cout << "Test skipped: command " << argv[0] << " requires TBB library" << endl;
+  return 0;
+}
+#endif
+
 //=======================================================================
 //function : OCC25547
 //purpose  :
@@ -3266,6 +3336,10 @@ void QABugs::Commands_19(Draw_Interpretor& theCommands) {
   theCommands.Add ("OCC25348", "OCC25348", __FILE__, OCC25348, group);
   theCommands.Add ("OCC25413", "OCC25413 shape", __FILE__, OCC25413, group);
   theCommands.Add ("OCC25446", "OCC25446 res b1 b2 op", __FILE__, OCC25446, group);
+  theCommands.Add ("OCC25545", 
+                   "no args; tests data race when concurrently accessing \n"
+                   "\t\tTopLoc_Location::Transformation()",
+                   __FILE__, OCC25545, group);
   theCommands.Add ("OCC25547", "OCC25547", __FILE__, OCC25547, group);
   return;
 }

src/TopLoc/TopLoc.cdl

@@ -34,7 +34,6 @@ uses
     gp
 
 is
-    pointer TrsfPtr to Trsf from gp;
     class Datum3D;
     
     private class ItemLocation;

src/TopLoc/TopLoc_ItemLocation.cdl

@@ -30,31 +30,21 @@ private class ItemLocation from TopLoc
 
 uses
     Datum3D   from TopLoc,
-    Trsf      from gp,
+    Trsf      from gp
-    TrsfPtr from TopLoc
     
 is
     Create(D : Datum3D      from TopLoc; 
-    	   P : Integer      from Standard;
+    	   P : Integer      from Standard) returns ItemLocation from TopLoc;
-           fromTrsf : Boolean from Standard = Standard_False) returns ItemLocation from TopLoc;
 	---Purpose: Sets the elementary Datum to <D>
 	--          Sets the exponent to <P>
 
-    Create(anOther : ItemLocation from TopLoc) returns ItemLocation from TopLoc;
-    
-    Assign(me : in out; anOther : ItemLocation from TopLoc) returns ItemLocation from TopLoc;
-    ---C++: alias operator=
-    ---C++: return &
-
-    Destroy(me : in out);
-    ---C++: alias ~
-
 fields
-    myDatum  : Datum3D   from TopLoc;
+    myDatum     : Datum3D   from TopLoc;
-    myPower  : Integer   from Standard;
+    myPower     : Integer   from Standard;
-    myTrsf   : TrsfPtr   from TopLoc;
+    myTrsf      : Trsf      from gp;
 
 friends
-    class Location from TopLoc
+    class Location from TopLoc,
-    
+    class SListOfItemLocation from TopLoc
+
 end ItemLocation;

src/TopLoc/TopLoc_ItemLocation.cxx

@@ -23,45 +23,9 @@
 
 TopLoc_ItemLocation::TopLoc_ItemLocation 
   (const Handle(TopLoc_Datum3D)& D, 
-   const Standard_Integer P,
+   const Standard_Integer P) :
-//   const Standard_Boolean fromtrsf) :
-   const Standard_Boolean ) :
   myDatum(D),
   myPower(P),
-  myTrsf(NULL)
+  myTrsf (D->Transformation().Powered (P))
 {
 }
-
-
-TopLoc_ItemLocation::TopLoc_ItemLocation(const TopLoc_ItemLocation& anOther): myTrsf(NULL)
-{
-  if (anOther.myTrsf != NULL) {
-    myTrsf = new gp_Trsf;  
-    *myTrsf = *(anOther.myTrsf);
-  }
-  myDatum = anOther.myDatum;
-  myPower = anOther.myPower;
-}
-
-TopLoc_ItemLocation& TopLoc_ItemLocation::Assign(const TopLoc_ItemLocation& anOther)
-{
-  if (anOther.myTrsf == NULL) {
-    if (myTrsf != NULL) delete myTrsf;
-    myTrsf = NULL;
-  }
-  else if (myTrsf != anOther.myTrsf) {
-    if (myTrsf == NULL) myTrsf = new gp_Trsf;  
-    *myTrsf = *(anOther.myTrsf);
-  }
-  myDatum = anOther.myDatum;
-  myPower = anOther.myPower;
-
-  return *this;
-}
-
-void TopLoc_ItemLocation::Destroy()
-{
-  if (myTrsf != NULL) delete myTrsf;
-  myTrsf = NULL;
-}
-

src/TopLoc/TopLoc_Location.cxx

@@ -22,22 +22,9 @@
 #include <TopLoc_SListOfItemLocation.hxx>
 #include <TopLoc_ItemLocation.hxx>
 #include <gp_Trsf.hxx>
-#include <TopLoc_TrsfPtr.hxx>
 
 static const gp_Trsf TheIdentity;
 
-
-static Standard_Boolean IsInternalIdentity(const TopLoc_Location& loc)
-{
-  if (loc.IsIdentity()) {
-    return Standard_True;
-  }
-//  if (loc.FirstDatum()->Transformation().Form() == gp_Identity) {
-//    return Standard_True;
-//  }
-  return Standard_False;
-}
-
 //=======================================================================
 //function : TopLoc_Location
 //purpose  : constructor Identity
@@ -74,19 +61,10 @@ TopLoc_Location::TopLoc_Location(const gp_Trsf& T)
 
 const gp_Trsf& TopLoc_Location::Transformation() const
 {
-  if (IsInternalIdentity(*this))
+  if (IsIdentity())
     return TheIdentity;
-  else {
+  else
-    if (myItems.Value().myTrsf == NULL) {
+    return myItems.Value().myTrsf;
-      TopLoc_ItemLocation *I = (TopLoc_ItemLocation*) (void*) &this->myItems.Value();
-      // CLE
-      if (I->myTrsf == NULL) I->myTrsf = new gp_Trsf;
-      *(I->myTrsf) = I->myDatum->Transformation();
-      I->myTrsf->Power(I->myPower);
-      I->myTrsf->PreMultiply(NextLocation().Transformation());
-    }
-    return *(myItems.Value().myTrsf);
-  }
 }
 
 TopLoc_Location::operator gp_Trsf() const
@@ -172,7 +150,7 @@ TopLoc_Location TopLoc_Location::Predivided (const TopLoc_Location& Other)
 
 TopLoc_Location TopLoc_Location::Powered (const Standard_Integer pwr) const
 {
-  if (IsInternalIdentity(*this)) return *this;
+  if (IsIdentity()) return *this;
   if (pwr == 1) return *this;
   if (pwr == 0) return TopLoc_Location();
   

src/TopLoc/TopLoc_SListNodeOfItemLocation.cdl

@@ -23,10 +23,6 @@ uses
 is
    Create(I : ItemLocation from TopLoc; aTail :  SListOfItemLocation from TopLoc) returns SListNodeOfItemLocation from TopLoc;
    ---C++:inline
-
-   Count(me) returns Integer;
-   ---C++:inline
-   ---C++: return &
    
    Tail(me) returns SListOfItemLocation from TopLoc;
    ---C++:inline

src/TopLoc/TopLoc_SListOfItemLocation.cdl

@@ -31,20 +31,6 @@ private class SListOfItemLocation from TopLoc
 	-- SListOfItemLocation Iterator;
 	-- for (Iterator = S; Iterator.More(); Iterator.Next())
 	--   X = Iterator.Value();
-	-- 
-        --  Memory usage  is  automatically managed for  SListOfItemLocations
-	--  (using reference counts).
-      	---Example:
-	-- If S1 and S2 are SListOfItemLocations :
-	-- if S1.Value() is X.
-	-- 
-	-- And the following is done :
-	-- S2 = S1;
-	-- S2.SetValue(Y);
-	-- 
-	-- S1.Value() becomes also Y.   So SListOfItemLocation must be used
-	-- with   care.  Mainly  the SetValue()    method  is
-	-- dangerous. 
 
 uses
     SListNodeOfItemLocation from TopLoc,
@@ -56,6 +42,7 @@ raises
 is
     Create returns SListOfItemLocation from TopLoc;
 	---Purpose: Creates an empty List.
+	---C++: inline
 	
     Create(anItem : ItemLocation from TopLoc; aTail : SListOfItemLocation from TopLoc)
     returns SListOfItemLocation from TopLoc;
@@ -64,6 +51,7 @@ is
     Create(Other : SListOfItemLocation from TopLoc)
     returns SListOfItemLocation from TopLoc;
 	---Purpose: Creates a list from an other one. The lists  are shared. 
+	---C++: inline
 	
     Assign(me : in out; Other : SListOfItemLocation from TopLoc)
     returns SListOfItemLocation from TopLoc
@@ -83,6 +71,7 @@ is
         ---Level: Public
 	---Purpose: Sets the list to be empty.
 	---C++: alias ~
+	---C++: inline
     is static;
 	
     Value(me) returns any ItemLocation from TopLoc
@@ -94,26 +83,6 @@ is
     	NoSuchObject from Standard
     is static;
     
-    ChangeValue(me : in out) returns any ItemLocation from TopLoc
-        ---Level: Public
-	---Purpose: Returns the current value of the list. An error is
-	-- raised if the  list  is empty.   This value may be
-	-- modified.   A   method modifying the  value can be
-	-- called. The value will be modified in the list.
-	---Example: AList.ChangeValue().Modify()
-	---C++: return &
-    raises
-    	NoSuchObject from Standard
-    is static;
-    
-    SetValue(me : in out; anItem : ItemLocation from TopLoc)
-        ---Level: Public
-	---Purpose: Changes the current value in the list. An error is
-	-- raised if the list is empty.
-    raises
-    	NoSuchObject from Standard
-    is static;
-    
     Tail(me) returns SListOfItemLocation from TopLoc
         ---Level: Public
 	---Purpose: Returns the current tail of  the list. On an empty
@@ -121,21 +90,6 @@ is
 	---C++: return const &
     is static;
     
-    ChangeTail(me : in out) returns SListOfItemLocation from TopLoc
-        ---Level: Public
-	---Purpose: Returns the current  tail of the list.   This tail
-	-- may be modified.  A method modifying the  tail can
-	-- be called. The tail will be modified in the list.
-	---Example: AList.ChangeTail().Modify()
-	---C++: return &
-    is static;
-    
-    SetTail(me : in out; aList : SListOfItemLocation from TopLoc)
-        ---Level: Public
-	---Purpose: Changes the current tail  in the list. On an empty
-	-- list SetTail is Assign.
-    is static;
-    
     Construct(me : in out; anItem : ItemLocation from TopLoc)  
         ---Level: Public
 	---Purpose: Replaces the list by a list with <anItem> as Value
@@ -143,25 +97,11 @@ is
 	---C++: inline
     is static;
     
-    Constructed(me; anItem : ItemLocation from TopLoc) returns SListOfItemLocation from TopLoc
-        ---Level: Public
-	---Purpose: Returns a new list  with  <anItem> as Value an the
-	-- list <me> as tail.
-	---C++: inline
-    is static;
-    
     ToTail(me :  in out)
         ---Level: Public
 	---Purpose: Replaces the list <me> by its tail.
 	---C++: inline
     is static;
-        
-    Initialize(me : in out; aList : SListOfItemLocation from TopLoc)
-        ---Level: Public
-	---Purpose: Sets  the iterator  to iterate   on the content of
-	-- <aList>. This is Assign().
-	---C++: inline
-    is static;
     
     More(me) returns Boolean
         ---Level: Public

src/TopLoc/TopLoc_SListOfItemLocation.cxx

@@ -23,27 +23,14 @@
 //purpose  : 
 //=======================================================================
 
-TopLoc_SListOfItemLocation::TopLoc_SListOfItemLocation()
-{}
-
-//=======================================================================
-//function : TopLoc_SListOfItemLocation
-//purpose  : 
-//=======================================================================
-
 TopLoc_SListOfItemLocation::TopLoc_SListOfItemLocation(const TopLoc_ItemLocation& anItem,
 				     const TopLoc_SListOfItemLocation& aTail) : 
        myNode(new TopLoc_SListNodeOfItemLocation(anItem,aTail))
-{}
-
-//=======================================================================
-//function : TopLoc_SListOfItemLocation
-//purpose  : 
-//=======================================================================
-
-TopLoc_SListOfItemLocation::TopLoc_SListOfItemLocation(const TopLoc_SListOfItemLocation& Other) : 
-       myNode(Other.myNode)
 {
+  if (!myNode->Tail().IsEmpty()) {
+    const gp_Trsf& aT = myNode->Tail().Value().myTrsf;
+    myNode->Value().myTrsf.PreMultiply (aT);
+  }
 }
 
 //=======================================================================
@@ -60,18 +47,6 @@ TopLoc_SListOfItemLocation& TopLoc_SListOfItemLocation::Assign(const TopLoc_SLis
   return *this;
 }
 
-//=======================================================================
-//function : Clear
-//purpose  : 
-//=======================================================================
-
-void TopLoc_SListOfItemLocation::Clear()
-{
-  if (!myNode.IsNull()) {
-    myNode.Nullify();
-  }
-}
-
 //=======================================================================
 //function : Value
 //purpose  : 
@@ -83,28 +58,6 @@ const TopLoc_ItemLocation& TopLoc_SListOfItemLocation::Value() const
   return myNode->Value();
 }
 
-//=======================================================================
-//function : ChangeValue
-//purpose  : 
-//=======================================================================
-
-TopLoc_ItemLocation& TopLoc_SListOfItemLocation::ChangeValue()
-{
-  Standard_NoSuchObject_Raise_if(myNode.IsNull(),"TopLoc_SListOfItemLocation::Value");
-  return myNode->Value();
-}
-
-//=======================================================================
-//function : SetValue
-//purpose  : 
-//=======================================================================
-
-void TopLoc_SListOfItemLocation::SetValue(const TopLoc_ItemLocation& anItem)
-{
-  Standard_NoSuchObject_Raise_if(myNode.IsNull(),"TopLoc_SListOfItemLocation::Value");
-  myNode->Value() = anItem;
-}
-
 //=======================================================================
 //function : Tail
 //purpose  : 
@@ -117,29 +70,3 @@ const TopLoc_SListOfItemLocation& TopLoc_SListOfItemLocation::Tail() const
   else
     return *this;
 }
-
-//=======================================================================
-//function : ChangeTail
-//purpose  : 
-//=======================================================================
-
-TopLoc_SListOfItemLocation& TopLoc_SListOfItemLocation::ChangeTail()
-{
-  if (!myNode.IsNull()) 
-    return myNode->Tail();
-  else
-    return *this;
-}
-
-//=======================================================================
-//function : SetTail
-//purpose  : 
-//=======================================================================
-
-void TopLoc_SListOfItemLocation::SetTail(const TopLoc_SListOfItemLocation& aList)
-{
-  if (!myNode.IsNull())
-    myNode->Tail() = aList;
-  else
-    Assign(aList);
-}

src/TopLoc/TopLoc_SListOfItemLocation.lxx

@@ -14,6 +14,24 @@
 // Alternatively, this file may be used under the terms of Open CASCADE
 // commercial license or contractual agreement.
 
+//=======================================================================
+//function : TopLoc_SListOfItemLocation
+//purpose  : 
+//=======================================================================
+
+inline TopLoc_SListOfItemLocation::TopLoc_SListOfItemLocation()
+{}
+
+//=======================================================================
+//function : TopLoc_SListOfItemLocation
+//purpose  : 
+//=======================================================================
+
+inline TopLoc_SListOfItemLocation::TopLoc_SListOfItemLocation(const TopLoc_SListOfItemLocation& Other) : 
+       myNode(Other.myNode)
+{
+}
+
 //=======================================================================
 //function : IsEmpty
 //purpose  : 
@@ -25,10 +43,17 @@ inline Standard_Boolean TopLoc_SListOfItemLocation::IsEmpty() const
 }
 
 //=======================================================================
-//function : Construct
+//function : Clear
 //purpose  : 
 //=======================================================================
 
+inline void TopLoc_SListOfItemLocation::Clear()
+{
+  if (!myNode.IsNull()) {
+    myNode.Nullify();
+  }
+}
+
 //=======================================================================
 //function : Construct
 //purpose  : 
@@ -39,16 +64,6 @@ inline void TopLoc_SListOfItemLocation::Construct(const TopLoc_ItemLocation& anI
   Assign(TopLoc_SListOfItemLocation(anItem,*this));
 }
 
-//=======================================================================
-//function : Constructed
-//purpose  : 
-//=======================================================================
-
-inline TopLoc_SListOfItemLocation TopLoc_SListOfItemLocation::Constructed(const TopLoc_ItemLocation& anItem) const
-{
-  return TopLoc_SListOfItemLocation(anItem,*this);
-}
-
 //=======================================================================
 //function : ToTail
 //purpose  : 
@@ -59,16 +74,6 @@ inline void TopLoc_SListOfItemLocation::ToTail()
   Assign(Tail());
 }
 
-//=======================================================================
-//function : Initialize
-//purpose  : 
-//=======================================================================
-
-inline void TopLoc_SListOfItemLocation::Initialize(const TopLoc_SListOfItemLocation& aList)
-{
-  Assign(aList);
-}
-
 //=======================================================================
 //function : More
 //purpose  : 

src/gp/gp_Trsf.cdl

@@ -343,7 +343,7 @@ is
 
   Power (me : in out; N : Integer)   raises ConstructionError   is static;
 
-  Powered (me : in out; N : Integer)  returns Trsf
+  Powered (me; N : Integer)  returns Trsf
         ---C++: inline
         --- Purpose :
         --  Computes the following composition of transformations

src/gp/gp_Trsf.lxx

@@ -92,7 +92,7 @@ inline gp_Trsf gp_Trsf::Multiplied (const gp_Trsf& T) const
   return Tresult;
 }
 
-inline gp_Trsf gp_Trsf::Powered (const Standard_Integer N)
+inline gp_Trsf gp_Trsf::Powered (const Standard_Integer N) const
 {
   gp_Trsf T = *this;
   T.Power (N);

tests/bugs/fclasses/bug25545

@@ -0,0 +1,11 @@
+puts "============"
+puts "OCC25545"
+puts "============"
+puts ""
+#######################################################################
+#  TopLoc_Location::Transformation() provokes data races
+#######################################################################
+
+pload QAcommands
+
+OCC25545