# HG changeset patch # User Peter D. Barnes, Jr. # Date 1376676619 25200 # Node ID 73dfb0c88ed644d158e68d2850adced2f42d4628 # Parent d7b91afc71c63cc5d465fa8a006043bc8f4ac810 [Bug 954] Protect g_markingTimes with a mutex. diff -r d7b91afc71c6 -r 73dfb0c88ed6 src/core/model/nstime.h --- 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::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 MakeTimeChecker (const Time min, const Time max); diff -r d7b91afc71c6 -r 73dfb0c88ed6 src/core/model/time.cc --- 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 #include // 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::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;