BUG: fixed thread safety errors found by Thread Sanitizer

std::vector<bool> has less thread safety than vectors of other types, because multiple boolean bits can be packed into the same byte(s). C++11 standard § 23.2.2, paragraph 2 describes this futher. So instead, just use uint8_t instead of bool. This allows enough thread safety for the usage at hand, as evidenced by various tests now passing under thread sanitizer.
InsightSoftwareConsortium · Jan 5, 2022 · b66133a · b66133a
1 parent b2c2c34
commit b66133a
Show file tree

Hide file tree

Showing 12 changed files with 29 additions and 18 deletions.
diff --git a/Modules/Core/FiniteDifference/include/itkDenseFiniteDifferenceImageFilter.h b/Modules/Core/FiniteDifference/include/itkDenseFiniteDifferenceImageFilter.h
@@ -172,7 +172,10 @@ class ITK_TEMPLATE_EXPORT DenseFiniteDifferenceImageFilter
     DenseFiniteDifferenceImageFilter * Filter;
     TimeStepType                       TimeStep;
     std::vector<TimeStepType>          TimeStepList;
-    std::vector<bool>                  ValidTimeStepList;
+
+    // NB: although semantically boolean, vector<bool> is not thread safe due to the possibility of multiple bits being
+    // packed together in the same memory location.
+    std::vector<uint8_t> ValidTimeStepList;
   };
 
   /** This callback method uses ImageSource::SplitRequestedRegion to acquire an

diff --git a/Modules/Core/FiniteDifference/include/itkFiniteDifferenceImageFilter.h b/Modules/Core/FiniteDifference/include/itkFiniteDifferenceImageFilter.h
@@ -330,12 +330,12 @@ class ITK_TEMPLATE_EXPORT FiniteDifferenceImageFilter : public InPlaceImageFilte
    *
    * \param timeStepList The set of time changes compiled from all the threaded calls
    * to ThreadedGenerateData.
-   * \param valid The set of flags indicating which of "list" elements are
-   *  valid
+   * \param valid The set of flags indicating which of "timeStepList" elements are
+   *  valid. Although they are uint8_t, they should be treated like bools.
    *
    * The default is to return the minimum value in the list. */
   virtual TimeStepType
-  ResolveTimeStep(const std::vector<TimeStepType> & timeStepList, const std::vector<bool> & valid) const;
+  ResolveTimeStep(const std::vector<TimeStepType> & timeStepList, const std::vector<uint8_t> & valid) const;
 
   /** Set the number of elapsed iterations of the filter. */
   itkSetMacro(ElapsedIterations, IdentifierType);

diff --git a/Modules/Core/FiniteDifference/include/itkFiniteDifferenceImageFilter.hxx b/Modules/Core/FiniteDifference/include/itkFiniteDifferenceImageFilter.hxx
@@ -167,7 +167,7 @@ FiniteDifferenceImageFilter<TInputImage, TOutputImage>::GenerateInputRequestedRe
 template <typename TInputImage, typename TOutputImage>
 typename FiniteDifferenceImageFilter<TInputImage, TOutputImage>::TimeStepType
 FiniteDifferenceImageFilter<TInputImage, TOutputImage>::ResolveTimeStep(const std::vector<TimeStepType> & timeStepList,
-                                                                        const std::vector<bool> &         valid) const
+                                                                        const std::vector<uint8_t> &      valid) const
 {
   TimeStepType oMin = NumericTraits<TimeStepType>::ZeroValue();
   bool         flag = false;

diff --git a/Modules/Core/FiniteDifference/include/itkFiniteDifferenceSparseImageFilter.h b/Modules/Core/FiniteDifference/include/itkFiniteDifferenceSparseImageFilter.h
@@ -197,7 +197,10 @@ class ITK_TEMPLATE_EXPORT FiniteDifferenceSparseImageFilter
     FiniteDifferenceSparseImageFilter * Filter;
     TimeStepType                        TimeStep;
     std::vector<TimeStepType>           TimeStepList;
-    std::vector<bool>                   ValidTimeStepList;
+
+    // NB: although semantically boolean, vector<bool> is not thread safe due to the possibility of multiple bits being
+    // packed together in the same memory location.
+    std::vector<uint8_t> ValidTimeStepList;
   };
 
 private:

diff --git a/Modules/Core/FiniteDifference/include/itkFiniteDifferenceSparseImageFilter.hxx b/Modules/Core/FiniteDifference/include/itkFiniteDifferenceSparseImageFilter.hxx
@@ -167,7 +167,7 @@ FiniteDifferenceSparseImageFilter<TInputImageType, TSparseOutputImageType>::Calc
   // Initialize the list of time step values that will be generated by the
   // various threads.  There is one distinct slot for each possible thread,
   // so this data structure is thread-safe.  All of the time steps calculated
-  // in each thread will be combined in the ResolveTimeStepMethod.
+  // in each thread will be combined in the ResolveTimeStep method.
   ThreadIdType workUnitCount = this->GetMultiThreader()->GetNumberOfWorkUnits();
 
   str.TimeStepList.resize(workUnitCount, false);

diff --git a/Modules/Core/GPUFiniteDifference/include/itkGPUFiniteDifferenceImageFilter.h b/Modules/Core/GPUFiniteDifference/include/itkGPUFiniteDifferenceImageFilter.h
@@ -240,12 +240,12 @@ class ITK_TEMPLATE_EXPORT GPUFiniteDifferenceImageFilter
    *
    * \param timeStepList The set of time changes compiled from all the threaded calls
    * to ThreadedGenerateData.
-   * \param valid The set of flags indicating which of "list" elements are
-   *  valid
+   * \param valid The set of flags indicating which of "timeStepList" elements are
+   *  valid. Although they are uint8_t, they should be treated like bools.
    *
    * The default is to return the minimum value in the list. */
   TimeStepType
-  ResolveTimeStep(const std::vector<TimeStepType> & timeStepList, const std::vector<bool> & valid) const override;
+  ResolveTimeStep(const std::vector<TimeStepType> & timeStepList, const std::vector<uint8_t> & valid) const override;
 
   /** This method is called after the solution has been generated to allow
    * subclasses to apply some further processing to the output. */

diff --git a/Modules/Core/GPUFiniteDifference/include/itkGPUFiniteDifferenceImageFilter.hxx b/Modules/Core/GPUFiniteDifference/include/itkGPUFiniteDifferenceImageFilter.hxx
@@ -183,7 +183,7 @@ template <typename TInputImage, typename TOutputImage, typename TParentImageFilt
 typename GPUFiniteDifferenceImageFilter<TInputImage, TOutputImage, TParentImageFilter>::TimeStepType
 GPUFiniteDifferenceImageFilter<TInputImage, TOutputImage, TParentImageFilter>::ResolveTimeStep(
   const std::vector<TimeStepType> & timeStepList,
-  const std::vector<bool> &         valid) const
+  const std::vector<uint8_t> &      valid) const
 {
   TimeStepType oMin = NumericTraits<TimeStepType>::ZeroValue();
   bool         flag = false;

diff --git a/Modules/Nonunit/Review/include/itkMultiphaseFiniteDifferenceImageFilter.h b/Modules/Nonunit/Review/include/itkMultiphaseFiniteDifferenceImageFilter.h
@@ -515,11 +515,12 @@ class ITK_TEMPLATE_EXPORT MultiphaseFiniteDifferenceImageFilter : public InPlace
    * \param timeStepList The set of time changes compiled from all the threaded
    *        calls to ThreadedGenerateData.
    *
-   * \param valid The set of flags indicating which of "list" elements are valid
+   * \param valid The set of flags indicating which of "timeStepList" elements are
+   *  valid. Although they are uint8_t, they should be treated like bools.
    *
    * The default is to return the minimum value in the list. */
   inline TimeStepType
-  ResolveTimeStep(const TimeStepVectorType & timeStepList, const std::vector<bool> & valid);
+  ResolveTimeStep(const TimeStepVectorType & timeStepList, const std::vector<uint8_t> & valid);
 
   /** This method is called after the solution has been generated to allow
    * subclasses to apply some further processing to the output. */

diff --git a/Modules/Nonunit/Review/include/itkMultiphaseFiniteDifferenceImageFilter.hxx b/Modules/Nonunit/Review/include/itkMultiphaseFiniteDifferenceImageFilter.hxx
@@ -201,7 +201,7 @@ typename MultiphaseFiniteDifferenceImageFilter<TInputImage,
                                                TFiniteDifferenceFunction,
                                                TIdCell>::TimeStepType
 MultiphaseFiniteDifferenceImageFilter<TInputImage, TFeatureImage, TOutputImage, TFiniteDifferenceFunction, TIdCell>::
-  ResolveTimeStep(const TimeStepVectorType & timeStepList, const std::vector<bool> & valid)
+  ResolveTimeStep(const TimeStepVectorType & timeStepList, const std::vector<uint8_t> & valid)
 {
   TimeStepType        oMin = NumericTraits<TimeStepType>::ZeroValue();
   const SizeValueType size = timeStepList.size();

diff --git a/Modules/Numerics/NarrowBand/include/itkNarrowBandImageFilterBase.h b/Modules/Numerics/NarrowBand/include/itkNarrowBandImageFilterBase.h
@@ -292,7 +292,9 @@ class ITK_TEMPLATE_EXPORT NarrowBandImageFilterBase : public FiniteDifferenceIma
 
   bool m_Touched;
 
-  std::vector<bool> m_TouchedForThread;
+  // NB: although semantically boolean, vector<bool> is not thread safe due to the possibility of multiple bits being
+  // packed together in the same memory location.
+  std::vector<uint8_t> m_TouchedForThread;
 
   ValueType m_IsoSurfaceValue;
 

diff --git a/Modules/Numerics/NarrowBand/include/itkNarrowBandImageFilterBase.hxx b/Modules/Numerics/NarrowBand/include/itkNarrowBandImageFilterBase.hxx
@@ -86,7 +86,7 @@ NarrowBandImageFilterBase<TInputImage, TOutputImage>::GenerateData()
 
   TimeStepType              timeStep;
   std::vector<TimeStepType> timeStepList(numberOfWorkUnits, NumericTraits<TimeStepType>::ZeroValue());
-  std::vector<bool>         validTimeStepList(numberOfWorkUnits, true);
+  std::vector<uint8_t>      validTimeStepList(numberOfWorkUnits, true);
 
   // Implement iterative loop in thread function
   // ThreadedApplyUpdate and ThreadedCalculateChanged

diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h
@@ -712,8 +712,10 @@ class ITK_TEMPLATE_EXPORT ParallelSparseFieldLevelSetImageFilter
 
   /** Thread-specific data */
   std::vector<TimeStepType> m_TimeStepList;
-  std::vector<bool>         m_ValidTimeStepList;
-  TimeStepType              m_TimeStep;
+  // NB: although semantically boolean, vector<bool> is not thread safe due to the possibility of multiple bits being
+  // packed together in the same memory location.
+  std::vector<uint8_t> m_ValidTimeStepList;
+  TimeStepType         m_TimeStep;
 
   /** The number of work units to use. */
   ThreadIdType m_NumOfWorkUnits{ 0 };