[Bug 954] Protect g_markingTimes with a mutex.
authorPeter D. Barnes, Jr. <barnes26@llnl.gov>
Fri, 16 Aug 2013 11:10:19 -0700
changeset 10176 73dfb0c88ed6
parent 10175 d7b91afc71c6
child 10177 c44ccde1d5ab
[Bug 954] Protect g_markingTimes with a mutex.
src/core/model/nstime.h
src/core/model/time.cc
--- a/src/core/model/nstime.h	Wed Aug 14 16:55:47 2013 -0700
+++ b/src/core/model/nstime.h	Fri Aug 16 11:10:19 2013 -0700
@@ -69,17 +69,17 @@
  * 1. It is possible to either increase or decrease the resolution and the
  * code tries really hard to make this easy.
  *
- * If your resolution is X (say, nanoseconds) and if you create Time objects 
- * with a lower resolution (say, picoseconds), don't expect that this 
- * code will return 1: PicoSeconds (1).GetPicoSeconds (). It will most 
- * likely return 0 because the Time object has only 64 bits of fractional 
+ * If your resolution is X (say, nanoseconds) and if you create Time objects
+ * with a lower resolution (say, picoseconds), don't expect that this
+ * code will return 1: PicoSeconds (1).GetPicoSeconds (). It will most
+ * likely return 0 because the Time object has only 64 bits of fractional
  * precision which means that PicoSeconds (1) is stored as a 64-bit aproximation
- * of 1/1000 in the Time object. If you later multiply it again by the exact 
+ * of 1/1000 in the Time object. If you later multiply it again by the exact
  * value 1000, the result is unlikely to be 1 exactly. It will be close to
  * 1 but not exactly 1.
- * 
+ *
  * In general, it is thus a really bad idea to try to use time objects of a
- * resolution higher than the global resolution controlled through 
+ * resolution higher than the global resolution controlled through
  * Time::SetResolution. If you do need to use picoseconds, it's thus best
  * to switch the global resolution to picoseconds to avoid nasty surprises.
  *
@@ -87,7 +87,7 @@
  * global resolution, you also implicitely decrease the range of your simulation.
  * i.e., the global simulation time is stored in a 64 bit integer whose interpretation
  * will depend on the global resolution so, 2^64 picoseconds which is the maximum
- * duration of your simulation if the global resolution is picoseconds 
+ * duration of your simulation if the global resolution is picoseconds
  * is smaller than 2^64 nanoseconds which is the maximum duration of your simulation
  * if the global resolution is nanoseconds.
  *
@@ -226,7 +226,7 @@
   {
     return Time (std::numeric_limits<int64_t>::max ());
   }
-  
+
   /**
    *  Destructor
    */
@@ -237,7 +237,7 @@
         Clear (this);
       }
   }
-  
+
   /**
    * \return true if the time is zero, false otherwise.
    */
@@ -350,7 +350,7 @@
   /**
    * \param resolution the new resolution to use
    *
-   * Change the global resolution used to convert all 
+   * Change the global resolution used to convert all
    * user-provided time values in Time objects and Time objects
    * in user-expected time units.
    */
@@ -399,7 +399,7 @@
       }
     else
       {
-        v /= info->factor; 
+        v /= info->factor;
       }
     return v;
   }
@@ -430,7 +430,7 @@
     struct Information *info = PeekInformation (timeUnit);
     // DO NOT REMOVE this temporary variable. It's here
     // to work around a compiler bug in gcc 3.4
-    int64x64_t retval = from; 
+    int64x64_t retval = from;
     if (info->fromMul)
       {
         retval *= info->timeFrom;
@@ -478,11 +478,11 @@
    */
   struct Information
   {
-    bool toMul;                     //!< Multiply when converting To, otherwise divide  
+    bool toMul;                     //!< Multiply when converting To, otherwise divide
     bool fromMul;                   //!< Multiple when converting From, otherwise divide
-    int64_t factor;                 //!< Ratio of this unit / current unit		  
-    int64x64_t timeTo;              //!< Multiplier to convert to this unit		  
-    int64x64_t timeFrom;            //!< Multiplier to convert from this unit		  
+    int64_t factor;                 //!< Ratio of this unit / current unit
+    int64x64_t timeTo;              //!< Multiplier to convert to this unit
+    int64x64_t timeFrom;            //!< Multiplier to convert from this unit
   };
   /**
    * Current time unit, and conversion info.
@@ -534,7 +534,7 @@
    *  \intern
    *
    *  Use a classic static variable so we can check in Time ctors
-   *  without a function call.  
+   *  without a function call.
    *
    *  We'd really like to initialize this here, but we don't want to require
    *  C++0x, so we init in time.cc.  To ensure that happens before first use,
@@ -587,11 +587,11 @@
   friend Time Max (const Time &ta, const Time &tb);
   friend Time Min (const Time &ta, const Time &tb);
 
-  
+
   int64_t m_data;                   //!< Virtual time value, in the current unit.
-  
+
 };  // class Time
-  
+
 
 // Force static initialization of Time
 static bool NS_UNUSED_GLOBAL (g_TimeStaticInit) = Time::StaticInit ();
@@ -789,7 +789,7 @@
 /**
  * \see Seconds(double)
  * \relates ns3::Time
- */ 
+ */
 inline Time Seconds (int64x64_t seconds)
 {
   return Time::From (seconds, Time::S);
@@ -797,7 +797,7 @@
 /**
  * \see MilliSeconds(uint64_t)
  * \relates ns3::Time
- */ 
+ */
 inline Time MilliSeconds (int64x64_t ms)
 {
   return Time::From (ms, Time::MS);
@@ -805,7 +805,7 @@
 /**
  * \see MicroSeconds(uint64_t)
  * \relates ns3::Time
- */ 
+ */
 inline Time MicroSeconds (int64x64_t us)
 {
   return Time::From (us, Time::US);
@@ -813,7 +813,7 @@
 /**
  * \see NanoSeconds(uint64_t)
  * \relates ns3::Time
- */ 
+ */
 inline Time NanoSeconds (int64x64_t ns)
 {
   return Time::From (ns, Time::NS);
@@ -821,7 +821,7 @@
 /**
  * \see PicoSeconds(uint64_t)
  * \relates ns3::Time
- */ 
+ */
 inline Time PicoSeconds (int64x64_t ps)
 {
   return Time::From (ps, Time::PS);
@@ -829,7 +829,7 @@
 /**
  * \see FemtoSeconds(uint64_t)
  * \relates ns3::Time
- */ 
+ */
 inline Time FemtoSeconds (int64x64_t fs)
 {
   return Time::From (fs, Time::FS);
@@ -854,7 +854,7 @@
  * \brief Helper to make a Time checker with bounded range.
  * Both limits are inclusive
  *
- * \return the AttributeChecker 
+ * \return the AttributeChecker
  */
 Ptr<const AttributeChecker> MakeTimeChecker (const Time min, const Time max);
 
--- a/src/core/model/time.cc	Wed Aug 14 16:55:47 2013 -0700
+++ b/src/core/model/time.cc	Fri Aug 16 11:10:19 2013 -0700
@@ -26,6 +26,7 @@
 #include "string.h"
 #include "object.h"
 #include "config.h"
+#include "system-mutex.h"
 #include "log.h"
 #include <cmath>
 #include <iomanip>  // showpos
@@ -39,18 +40,37 @@
 // static
 Time::MarkedTimes * Time::g_markingTimes = 0;
 
+/**
+ * Get mutex for critical sections around modification of Time::g_markingTimes
+ *
+ * \relates Time
+ */
+SystemMutex &
+GetMarkingMutex ()
+{
+  static SystemMutex * g_markingMutex = new SystemMutex;
+  return *g_markingMutex;
+}
+
+
 // Function called to force static initialization
 // static
 bool Time::StaticInit ()
 {
   static bool firstTime = true;
 
+  CriticalSection critical (GetMarkingMutex ());
+
   if (firstTime)
     {
       if (! g_markingTimes)
-	{
-	  g_markingTimes = new Time::MarkedTimes;
-	}
+        {
+          g_markingTimes = new Time::MarkedTimes;
+        }
+      else
+        {
+          NS_LOG_ERROR ("firstTime but g_markingTimes != 0");
+        }
 
       // Schedule the cleanup.
       // We'd really like:
@@ -149,7 +169,9 @@
 {
   NS_LOG_FUNCTION (resolution);
   if (convert)
-    { // We have to convert old values
+    {
+      // We have to convert existing Times with the old
+      // conversion values, so do it first
       ConvertTimes (unit);
     }
 
@@ -191,6 +213,23 @@
 void
 Time::ClearMarkedTimes ()
 {
+  /**
+   * \internal
+   *
+   * We're called by Simulator::Run, which knows nothing about the mutex,
+   * so we need a critical section here.
+   *
+   * It would seem natural to use this function at the end of
+   * ConvertTimes, but that function already has the mutex.
+   * Our SystemMutex throws a fatal error if we try to lock it more than
+   * once in the same thread (at least in the unix implementation),
+   * so calling this function from ConvertTimes is a bad idea.
+   *
+   * Instead, we copy this body into ConvertTimes.
+   */
+
+  CriticalSection critical (GetMarkingMutex ());
+
   NS_LOG_FUNCTION_NOARGS ();
   if (g_markingTimes)
     {
@@ -205,9 +244,13 @@
 void
 Time::Mark (Time * const time)
 {
+  CriticalSection critical (GetMarkingMutex ());
+
   NS_LOG_FUNCTION (time);
   NS_ASSERT (time != 0);
 
+  // Repeat the g_markingTimes test here inside the CriticalSection,
+  // since earlier test was outside and might be stale.
   if (g_markingTimes)
     {
       std::pair< MarkedTimes::iterator, bool> ret;
@@ -227,26 +270,28 @@
 void
 Time::Clear (Time * const time)
 {
+  CriticalSection critical (GetMarkingMutex ());
+
   NS_LOG_FUNCTION (time);
   NS_ASSERT (time != 0);
 
   if (g_markingTimes)
     {
       NS_ASSERT_MSG (g_markingTimes->count (time) == 1,
-		     "Time object " << time <<
-		     " registered " << g_markingTimes->count (time) <<
-		     " times (should be 1)." );
+                     "Time object " << time <<
+                     " registered " << g_markingTimes->count (time) <<
+                     " times (should be 1)." );
 
       MarkedTimes::size_type num = g_markingTimes->erase (time);
       if (num != 1)
-	{
-	  NS_LOG_WARN ("unexpected result erasing " << time << "!");
-	  NS_LOG_WARN ("got " << num << ", expected 1");
-	}
+        {
+          NS_LOG_WARN ("unexpected result erasing " << time << "!");
+          NS_LOG_WARN ("got " << num << ", expected 1");
+        }
       else
-	{
-	  NS_LOG_LOGIC ("\t[" << g_markingTimes->size () << "] removing  " << time);
-	}
+        {
+          NS_LOG_LOGIC ("\t[" << g_markingTimes->size () << "] removing  " << time);
+        }
     }
 }  // Time::Clear ()
 
@@ -255,15 +300,17 @@
 void
 Time::ConvertTimes (const enum Unit unit)
 {
+  CriticalSection critical (GetMarkingMutex ());
+
   NS_LOG_FUNCTION_NOARGS();
 
   NS_ASSERT_MSG (g_markingTimes != 0,
-		 "No MarkedTimes registry. "
-		 "Time::SetResolution () called more than once?");
+                 "No MarkedTimes registry. "
+                 "Time::SetResolution () called more than once?");
 
   for ( MarkedTimes::iterator it = g_markingTimes->begin();
-	it != g_markingTimes->end();
-	it++ )
+        it != g_markingTimes->end();
+        it++ )
     {
       Time * const tp = *it;
       if ( ! (    (tp->m_data == std::numeric_limits<int64_t>::min ())
@@ -277,7 +324,12 @@
 
   NS_LOG_LOGIC ("logged " << g_markingTimes->size () << " Time objects.");
 
-  ClearMarkedTimes ();
+  // Body of ClearMarkedTimes
+  // Assert above already guarantees g_markingTimes != 0
+  NS_LOG_LOGIC ("clearing MarkedTimes");
+  delete g_markingTimes;
+  g_markingTimes = 0;
+
 }  // Time::ConvertTimes ()
 
 
@@ -321,16 +373,16 @@
       break;
     }
   int64_t v = time.ToInteger (res);
-  
+
   std::ios_base::fmtflags ff = os.flags ();
   { // See bug 1737:  gcc libstc++ 4.2 bug
     if (v == 0)
-      { 
-	os << '+';
+      {
+        os << '+';
       }
     else
       {
-	os << std::showpos;
+        os << std::showpos;
       }
   }
   os << v << ".0" << unit;