[Bug 954] Protect g_markingTimes with a mutex.
--- 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;