threading: use std::chrono for timestamps #9566

Open
wants to merge 2 commits into
from
Jump to file or symbol
Failed to load files and symbols.
+130 −17
Split
View
@@ -516,6 +516,59 @@ fi
AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h stdio.h stdlib.h unistd.h strings.h sys/types.h sys/stat.h sys/select.h sys/prctl.h])
+dnl gmtime checks taken from libmicrohttpd
+AC_CHECK_FUNCS_ONCE([gmtime_r])
+AC_CHECK_DECL([gmtime_s],
+ [
+ AC_MSG_CHECKING([[whether gmtime_s is in C11 form]])
+ AC_LINK_IFELSE(
+ [ AC_LANG_PROGRAM(
+ [[
+#define __STDC_WANT_LIB_EXT1__ 1
+#include <time.h>
+#ifdef __cplusplus
+extern "C"
+#endif
+ struct tm* gmtime_s(const time_t* time, struct tm* result);
+ ]], [[
+ struct tm res;
+ time_t t;
+ gmtime_s (&t, &res);
+ ]])
+ ],
+ [
+ AC_DEFINE([HAVE_C11_GMTIME_S], [1], [Define to 1 if you have the `gmtime_s' function in C11 form.])
+ AC_MSG_RESULT([[yes]])
+ ],
+ [
+ AC_MSG_RESULT([[no]])
+ AC_MSG_CHECKING([[whether gmtime_s is in W32 form]])
+ AC_LINK_IFELSE(
+ [ AC_LANG_PROGRAM(
+ [[
+#include <time.h>
+#ifdef __cplusplus
+extern "C"
+#endif
+errno_t gmtime_s(struct tm* _tm, const time_t* time);
+ ]], [[
+ struct tm res;
+ time_t t;
+ gmtime_s (&res, &t);
+ ]])
+ ],
+ [
+ AC_DEFINE([HAVE_W32_GMTIME_S], [1], [Define to 1 if you have the `gmtime_s' function in W32 form.])
+ AC_MSG_RESULT([[yes]])
+ ],
+ [AC_MSG_RESULT([[no]])
+ ])
+ ])
+ ], [],
+ [[#define __STDC_WANT_LIB_EXT1__ 1
+#include <time.h>]]
+)
+
AC_CHECK_DECLS([strnlen])
# Check for daemon(3), unrelated to --with-daemon (although used by it)
View
@@ -687,6 +687,10 @@ bool InitSanityCheck(void)
if (!glibc_sanity_test() || !glibcxx_sanity_test())
return false;
+ if (!ChronoSanityCheck()) {
+ InitError("Clock epoch mismatch");
+ return false;
+ }
return true;
}
@@ -4,6 +4,7 @@
#include "compat/sanity.h"
#include "key.h"
+#include "utiltime.h"
#include "test/test_bitcoin.h"
#include <boost/test/unit_test.hpp>
@@ -15,6 +16,7 @@ BOOST_AUTO_TEST_CASE(basic_sanity)
BOOST_CHECK_MESSAGE(glibc_sanity_test() == true, "libc sanity test");
BOOST_CHECK_MESSAGE(glibcxx_sanity_test() == true, "stdlib sanity test");
BOOST_CHECK_MESSAGE(ECC_InitSanityCheck() == true, "openssl ECC test");
+ BOOST_CHECK_MESSAGE(ChronoSanityCheck() == true, "chrono epoch test");
}
BOOST_AUTO_TEST_SUITE_END()
View
@@ -3,26 +3,85 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#define __STDC_WANT_LIB_EXT1__ 1 // for gmtime_s
+
#if defined(HAVE_CONFIG_H)
#include "config/bitcoin-config.h"
#endif
#include "utiltime.h"
+#include <chrono>
+#include <thread>
+#include <locale>
+#include <sstream>
+#include <string.h>
+#include <time.h>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/thread.hpp>
using namespace std;
static int64_t nMockTime = 0; //!< For unit testing
+static inline const tm gmtime_int(time_t time)
+{
+ tm out = {};
+#if defined(HAVE_W32_GMTIME_S)
+ gmtime_s(&out, &time);
+#elif defined(HAVE_C11_GMTIME_S)
+ gmtime_s(&time, &out);
+#elif defined(HAVE_GMTIME_R)
+ gmtime_r(&time, &out);
+#else
+ // Not thread-safe!
+ out = *gmtime(&time);
+#endif
+ return out;
+}
+
+bool ChronoSanityCheck()
@laanwj

laanwj Jan 17, 2017 edited

Owner

Needing something like this makes me wonder if we're not better off just using the C time functions, which guarantee being based on the UNIX epoch.

Especially as we're already probing for gmtime_int anyway. This seems double.

Do we need something that is only offered by std::chrono? (or needs much less code with std::chrono)

@theuni

theuni Jan 17, 2017

Member

@laanwj Best I can tell from google and reading the c spec, it's not guaranteed to be based on the UNIX epoch either :(. Of course, I've never seen an implementation that isn't, as that would likely break years worth of assumptions. And based on that, I would assume that c++ implementations will be using the underlying c implementation.

So my logic was: if we can run a quick sanity check here to be sure that c/c++/unix epoch are all aligned, then we can use c apis and std::chrono interchangeably throughout the codebase without worry.

As for why chrono here, it was to avoid gettimeofday portability issues. If you'd prefer to work around those instead, no problem.

You're right though that DateTimeStrFormat is kinda clunky. I only used c++ there to avoid c string size guessing. I'm also happy to switch that if you'd prefer.

@laanwj

laanwj Jan 17, 2017 edited

Owner

Best I can tell from google and reading the c spec, it's not guaranteed to be based on the UNIX epoch either :(

Which ones? The man page of "gmtime" tells me the following:

       The  ctime(),  gmtime()  and localtime() functions all take an argument of data type time_t, which represents calendar time.  When interpreted as an absolute time value, it represents the
       number of seconds elapsed since the Epoch, 1970-01-01 00:00:00 +0000 (UTC).

Same for gettimeofday:

       and gives the number of seconds and microseconds since the Epoch (see time(2)).  The tz argument is a struct timezone:

If using std::chrono makes anything less hassle I'm fine with using it. It just seems like a lot of code necessary to work around issues (and that's just to get started using it!), but you're right that gettimeofday and strftime have their own issues.

@theuni

theuni Jan 17, 2017

Member

@laanwj Posix uses a value since the Unix epoch, but time_t is implementation-defined according to the c spec. But it really doesn't matter, realistically it'll be the Unix epoch everywhere we run. Sorry for the distraction.

I only brought it up because the c++ system_clock's epoch is technically implementation-defined as well, I was just making the case that it's probably just as safe to assume as with c.

+{
+ // std::chrono::system_clock.time_since_epoch and time_t(0) are not guaranteed
+ // to use the Unix epoch timestamp, but in practice they almost certainly will.
+ // Any differing behavior will be assumed to be an error, unless certain
+ // platforms prove to consistently deviate, at which point we'll cope with it
+ // by adding offsets.
+
+ // Create a new clock from time_t(0) and make sure that it represents 0
+ // seconds from the system_clock's time_since_epoch. Then convert that back
+ // to a time_t and verify that it's the same as before.
+ const time_t zeroTime{};
+ auto clock = std::chrono::system_clock::from_time_t(zeroTime);
+ if (std::chrono::duration_cast<std::chrono::seconds>(clock.time_since_epoch()).count() != 0)
@laanwj

laanwj Feb 1, 2017

Owner

nit: bracing style: we decided to always use braces unless the if and the statement can be on one line: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

+ return false;
+
+ time_t nTime = std::chrono::system_clock::to_time_t(clock);
+ if (nTime != zeroTime)
+ return false;
+
+ // Check that the above zero time is actually equal to the known unix timestamp.
+ tm epoch = gmtime_int(nTime);
+ if ((epoch.tm_sec != 0) || \
@laanwj

laanwj Feb 1, 2017

Owner

Why the \ at the end of the line?

+ (epoch.tm_min != 0) || \
+ (epoch.tm_hour != 0) || \
+ (epoch.tm_mday != 1) || \
+ (epoch.tm_mon != 0) || \
+ (epoch.tm_year != 70))
+ return false;
+ return true;
+}
+
+template <typename T>
+static inline int64_t GetCurrentTime()
+{
+ return std::chrono::duration_cast<T>(std::chrono::system_clock::now().time_since_epoch()).count();
+}
+
int64_t GetTime()
{
if (nMockTime) return nMockTime;
-
- time_t now = time(NULL);
- assert(now > 0);
- return now;
+ return GetCurrentTime<std::chrono::seconds>();
}
void SetMockTime(int64_t nMockTimeIn)
@@ -32,18 +91,12 @@ void SetMockTime(int64_t nMockTimeIn)
int64_t GetTimeMillis()
{
- int64_t now = (boost::posix_time::microsec_clock::universal_time() -
- boost::posix_time::ptime(boost::gregorian::date(1970,1,1))).total_milliseconds();
- assert(now > 0);
- return now;
+ return GetCurrentTime<std::chrono::milliseconds>();
}
int64_t GetTimeMicros()
{
- int64_t now = (boost::posix_time::microsec_clock::universal_time() -
- boost::posix_time::ptime(boost::gregorian::date(1970,1,1))).total_microseconds();
- assert(now > 0);
- return now;
+ return GetCurrentTime<std::chrono::microseconds>();
}
/** Return a time useful for the debug log */
@@ -72,13 +125,13 @@ void MilliSleep(int64_t n)
#endif
}
-std::string DateTimeStrFormat(const char* pszFormat, int64_t nTime)
+std::string DateTimeStrFormat(const char* pszFormat, int64_t nSecs)
{
static std::locale classic(std::locale::classic());
- // std::locale takes ownership of the pointer
- std::locale loc(classic, new boost::posix_time::time_facet(pszFormat));
+ time_t nTime = std::chrono::system_clock::to_time_t(std::chrono::system_clock::time_point{std::chrono::seconds{nSecs}});
+ const tm& now = gmtime_int(nTime);
std::stringstream ss;
- ss.imbue(loc);
- ss << boost::posix_time::from_time_t(nTime);
+ ss.imbue(classic);
+ std::use_facet<std::time_put<char>>(ss.getloc()).put(ss.rdbuf(), ss, ' ', &now, pszFormat, pszFormat + strlen(pszFormat));
return ss.str();
}
View
@@ -9,6 +9,7 @@
#include <stdint.h>
#include <string>
+bool ChronoSanityCheck();
int64_t GetTime();
int64_t GetTimeMillis();
int64_t GetTimeMicros();