From 5153da205001f0afffe7af91c165869da03c87cf Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Fri, 22 Apr 2022 21:16:27 +0000 Subject: [PATCH 1/2] Update rcutils_steady_time_now to return the same data as std::chrono Based on an investigation several years ago, rcutils_steady_time_now has slightly different behavior than std::chrono::now . In particular, on Linux rcutils_steady_time_now was using CLOCK_MONOTONIC_RAW, while std::chrono is using CLOCK_MONOTONIC. On macOS, rcutils_steady_time_now was using SYSTEM_CLOCK, while std::chrono was using CLOCK_MONOTONIC_RAW. Fix both of these so they match what std::chrono does, and in the case of macOS, significantly simplify the code by switching to clock_gettime. Signed-off-by: Chris Lalancette --- src/time_unix.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/time_unix.c b/src/time_unix.c index 47c996c8..b2723dd0 100644 --- a/src/time_unix.c +++ b/src/time_unix.c @@ -78,21 +78,13 @@ rcutils_steady_time_now(rcutils_time_point_value_t * now) // If clock_gettime is available or on OS X, use a timespec. struct timespec timespec_now; #if defined(__MACH__) - // On OS X use clock_get_time. - clock_serv_t cclock; - mach_timespec_t mts; - host_get_clock_service(mach_host_self(), SYSTEM_CLOCK, &cclock); - clock_get_time(cclock, &mts); - mach_port_deallocate(mach_task_self(), cclock); - timespec_now.tv_sec = mts.tv_sec; - timespec_now.tv_nsec = mts.tv_nsec; + // On macOS, use clock_get_time(CLOCK_MONOTONIC_RAW), which matches + // the clang implementation + // (https://github.com/llvm/llvm-project/blob/baebe12ad0d6f514cd33e418d6504075d3e79c0a/libcxx/src/chrono.cpp) + clock_gettime(CLOCK_MONOTONIC_RAW, ×pec_now); #else // defined(__MACH__) // Otherwise use clock_gettime. -#if defined(CLOCK_MONOTONIC_RAW) - clock_gettime(CLOCK_MONOTONIC_RAW, ×pec_now); -#else // defined(CLOCK_MONOTONIC_RAW) clock_gettime(CLOCK_MONOTONIC, ×pec_now); -#endif // defined(CLOCK_MONOTONIC_RAW) #endif // defined(__MACH__) if (__WOULD_BE_NEGATIVE(timespec_now.tv_sec, timespec_now.tv_nsec)) { RCUTILS_SET_ERROR_MSG("unexpected negative time"); From aaa21db8c69f57b7c781f896092c2e59b1ec8c18 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 26 Apr 2022 18:48:27 +0000 Subject: [PATCH 2/2] More fixes for macOS. With this, all of the tests seem to pass. Signed-off-by: Chris Lalancette --- src/time_unix.c | 14 +++++--------- test/test_time.cpp | 14 -------------- 2 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/time_unix.c b/src/time_unix.c index b2723dd0..790cb091 100644 --- a/src/time_unix.c +++ b/src/time_unix.c @@ -51,14 +51,10 @@ rcutils_system_time_now(rcutils_time_point_value_t * now) RCUTILS_CHECK_ARGUMENT_FOR_NULL(now, RCUTILS_RET_INVALID_ARGUMENT); struct timespec timespec_now; #if defined(__MACH__) - // On OS X use clock_get_time. - clock_serv_t cclock; - mach_timespec_t mts; - host_get_clock_service(mach_host_self(), CALENDAR_CLOCK, &cclock); - clock_get_time(cclock, &mts); - mach_port_deallocate(mach_task_self(), cclock); - timespec_now.tv_sec = mts.tv_sec; - timespec_now.tv_nsec = mts.tv_nsec; + // On macOS, use clock_gettime(CLOCK_REALTIME), which matches + // the clang implementation + // (https://github.com/llvm/llvm-project/blob/baebe12ad0d6f514cd33e418d6504075d3e79c0a/libcxx/src/chrono.cpp) + clock_gettime(CLOCK_REALTIME, ×pec_now); #else // defined(__MACH__) // Otherwise use clock_gettime. clock_gettime(CLOCK_REALTIME, ×pec_now); @@ -78,7 +74,7 @@ rcutils_steady_time_now(rcutils_time_point_value_t * now) // If clock_gettime is available or on OS X, use a timespec. struct timespec timespec_now; #if defined(__MACH__) - // On macOS, use clock_get_time(CLOCK_MONOTONIC_RAW), which matches + // On macOS, use clock_gettime(CLOCK_MONOTONIC_RAW), which matches // the clang implementation // (https://github.com/llvm/llvm-project/blob/baebe12ad0d6f514cd33e418d6504075d3e79c0a/libcxx/src/chrono.cpp) clock_gettime(CLOCK_MONOTONIC_RAW, ×pec_now); diff --git a/test/test_time.cpp b/test/test_time.cpp index 43ab6daa..4cf6af1a 100644 --- a/test/test_time.cpp +++ b/test/test_time.cpp @@ -193,17 +193,7 @@ TEST_F(TestTimeFixture, test_rcutils_steady_time_now) { #if !defined(_WIN32) -// For mocking purposes -#if defined(__MACH__) -#include -#include -#define clock_gettime clock_get_time -#endif - -// Tests rcutils_system_time_now() and rcutils_steady_time_now() functions -// when system clocks misbehave. TEST_F(TestTimeFixture, test_rcutils_with_bad_system_clocks) { -#if !defined (__MACH__) // as tv_sec is an unsigned integer there { auto mock = mocking_utils::patch( "lib:rcutils", clock_gettime, @@ -222,7 +212,6 @@ TEST_F(TestTimeFixture, test_rcutils_with_bad_system_clocks) { EXPECT_EQ(RCUTILS_RET_ERROR, ret); rcutils_reset_error(); } -#endif { auto mock = mocking_utils::patch( "lib:rcutils", clock_gettime, @@ -243,9 +232,6 @@ TEST_F(TestTimeFixture, test_rcutils_with_bad_system_clocks) { } } -#if defined(__MACH__) -#undef clock_gettime -#endif #endif // !defined(_WIN32) // Tests the rcutils_time_point_value_as_nanoseconds_string() function.