timespec_to_long may return incorrect value in 32bit OS

[y-okumura-isp]

Bug report

Required Info:

• Operating System:
  • Raspbian OS(32bit)
• Installation type:
  • build Eloquent
  • master has the same issue
• DDS implementation:
  • FastRTPS
• Client library (if applicable):
  • rttest

Steps to reproduce issue

• rttest timespec_to_long returns incorrect value in Raspbian OS.
  • https://github.com/ros2/realtime_support/blob/master/rttest/include/rttest/utils.h#L76
• It's not a direct but potential bug.
  • rttest runs normaly in my environment.
  • When I called timespec_to_long in my code, this issue occured.

Here is an example code with my suggestions.
For simplicity, timespec_to_long is copied from utils.h.

#include <iostream>
#include <time.h>

// copied code from rttest/include/rttest/utils.h:76
#define NSEC_PER_SEC 1000000000
static inline uint64_t timespec_to_long(const struct timespec * t)
{
  return t->tv_sec * NSEC_PER_SEC + t->tv_nsec;
}

// my suggestion 1: tv_sec, tv_nsec seems long int in linux, use long long int.
static inline long long int timespec_to_long2(const struct timespec * t)
{
  return (long long int)t->tv_sec * NSEC_PER_SEC + (long long int)t->tv_nsec;
}

// my suggestion 2: specify int64_t explicitly (uint64_t may be OK)
static inline int64_t timespec_to_long3(const struct timespec * t)
{
  return (int64_t)t->tv_sec * NSEC_PER_SEC + (int64_t)t->tv_nsec;
}

int main(int argc, char *argv[])
{
  struct timespec t;
  clock_gettime(CLOCK_MONOTONIC_RAW, &t);

  // check tv_sec, tv_nsec size and value
  std::cout << "sizeof(t.tv_sec): " << sizeof(t.tv_sec)
            << " value: " << t.tv_sec
            << std::endl;
  std::cout << "sizeof(t.tv_nsec): " << sizeof(t.tv_nsec)
            << " value: " << t.tv_nsec
            << std::endl;

  // print timespec_to_long return value 
  std::cout << "timespec_to_long(&t): " << timespec_to_long(&t) << std::endl;
  std::cout << "timespec_to_long2(&t): " << timespec_to_long2(&t) << std::endl;;
  std::cout << "timespec_to_long3(&t): " << timespec_to_long3(&t) << std::endl;;
}

• Compile by g++ above_code.cpp and ./a.out.

Expected behavior

Result in Ubuntu 18.04 x86_64:

sizeof(t.tv_sec): 8 value: 11940218
sizeof(t.tv_nsec): 8 value: 372300334
timespec_to_long(&t): 11940218372300334
timespec_to_long2(&t): 11940218372300334
timespec_to_long3(&t): 11940218372300334

Actual behavior

Result in Raspbian OS:

sizeof(t.tv_sec): 4 value: 246386     // size is 4 not 8 in Raspbian OS
sizeof(t.tv_nsec): 4 value: 585632521 // size is 4 not 8 in Raspbian OS
timespec_to_long(&t): 1491730185      // oops
timespec_to_long2(&t): 246386585632521
timespec_to_long3(&t): 246386585632521

[clalancette]

From what I can tell, struct timespec looks like the following:

           struct timespec {
               time_t   tv_sec;        /* seconds */
               long     tv_nsec;       /* nanoseconds */
           };

It looks like time_t is defined to be a long int, so on 32-bit platforms you'll have a 32-bit integer for tv_sec and a 32-bit integer for tv_nsec. On a 64-bit platform, you'll have a 64-bit integer for tv_sec and a 64-bit integer for tv_nsec.

However, https://www.gnu.org/software/libc/manual/html_node/Time-Types.html says that this always represents a duration since a fixed point of time in the past. Further, it says that nanoseconds is always between 0 and 1,000,000. All of that implies that this can only ever be a positive number.

Thus, my suggestion is that we both rename the method and make it do the right thing on 32-bit platforms. Here would be my suggestion:

#define NSEC_PER_SEC 1000000000
static inline uint64_t timespec_to_uint64(const struct timespec * t)
{
  return static_cast<uint64_t>(t->tv_sec) * NSEC_PER_SEC + static_cast<uint64_t>(t->tv_nsec);
}

And then fix all of the callers so that it uses the right name. While we are at it, we should also probably rename and fix long_to_timespec to something like:

static inline void uint64_to_timespec(const uint64_t input, struct timespec * t)
{
  uint64_t nsecs = input % 1000000000;
  uint64_t secs = (input - nsecs) / 1000000000;
  t->tv_sec = static_cast<time_t>(secs);
  t->tv_nsec = static_cast<long>(nsecs);
}

@y-okumura-isp What do you think? Also, can you propose a patch implementing this? Thanks.

[y-okumura-isp]

@clalancette Thank you for reply.

Thus, my suggestion is that we both rename the method and make it do the right thing on 32-bit platform

Exactly!
Bit length of tv_sec, tv_nsec and long change as you said.
I felt uint64_t timespec_to_long returned uint64 not long (because signiture is not long timespec_to_long), and internal calcuration was done as uint64_t.

Also, can you propose a patch implementing this?

I will be happy to do.
But I'm on vocation next week so please give me some time.

[clalancette]

I will be happy to do.
But I'm on vocation next week so please give me some time.

Sounds good, thank you.

[hidmic]

@y-okumura-isp are you still planning to push this?

[y-okumura-isp]

@hidmic Yes, but sorry for late work.
After vacation I'm a little busy now, so not yet started.
If you have a suggestion/comment/PR, please tell me.

[y-okumura-isp]

I'm wring a patch, but I have a few questions.
Please see the commits bellow (it's successfuly built, but not tested yet).
y-okumura-isp@f016e2c

Here are my questions.

(1) Influence on pedulum_control

timespec_to_long and long_to_timespec are used by pendulum_control.

pendulum_control/include/pendulum_control/pendulum_motor.hpp:    long_to_timespec(physics_update_period_.count(), &physics_update_timespec_);
pendulum_control/src/pendulum_logger.cpp:      fstream << i << " " << timespec_to_long(&timestamp) <<

As these functions will be renamed, pendulum_control will not be built (so, colcon build for ros2 will also fail).
How do I send a PR? (I usually send PR indivisually in my projects, but I don't know ROS 2 style, so I asked just in case)
As far as I checked as bellow, other packages do not use these function.

cd src/ros2
for i in *; do cd $i; echo "=== $i ==="; git grep "timespec_to_long" ;cd ..; done
for i in *; do cd $i; echo "=== $i ==="; git grep "long_to_timespec" ;cd ..; done

(2) Failure for colcon test

I run colcon test --packages-select rttest and colcon test-result --verbose.
I found following two warning for my changes.
For your reference, there are many other warnings but I won't touch it now.

(2.1) cpplint for rtutils.h

This warning is for t->tv_nsec = static_cast<long>(nsecs);.
As tv_nsec is long, I cannot avoid long. Is there any good way?

rttest.cpplint runtime/int [4] (src/ros2/realtime_support/rttest/include/rttest/utils.h:81)
  <<< failure message
    Use int16/int64/etc, rather than the C type long
  >>>

(2.2) uncrustify

I got following error.
When I run ament_uncrustify --language C++ realtime_support/rttest/include/rttest/utils.h, I get no problems found.
So uncrustify may treat utils.h as C-style.
Should I rename utils.h to utils.hpp or explicitly specify C++ to uncrustify (I don't know it possible)?

Code style divergence in file 'src/ros2/realtime_support/rttest/include/rttest/utils.h':

--- src/ros2/realtime_support/rttest/include/rttest/utils.h
+++ src/ros2/realtime_support/rttest/include/rttest/utils.h.uncrustify
@@ -73 +73,2 @@
-  return static_cast<uint64_t>(t->tv_sec) * NSEC_PER_SEC + static_cast<uint64_t>(t->tv_nsec);
+  return static_cast < uint64_t > (t->tv_sec) * NSEC_PER_SEC + static_cast < uint64_t >
+         (t->tv_nsec);
@@ -80,2 +81,2 @@
-  t->tv_sec = static_cast<time_t>(secs);
-  t->tv_nsec = static_cast<long>(nsecs);
+  t->tv_sec = static_cast < time_t > (secs);
+  t->tv_nsec = static_cast < long > (nsecs);

Regards.

[clalancette]

How do I send a PR? (I usually send PR indivisually in my projects, but I don't know ROS 2 style, so I asked just in case)

Yes, just send another PR to pendulum_control as well, and link it back to the one you open against this repository.

This warning is for t->tv_nsec = static_cast<long>(nsecs);.
As tv_nsec is long, I cannot avoid long. Is there any good way?

You can use // NOLINT at the end of the line to have cpplint ignore the line.

Should I rename utils.h to utils.hpp or explicitly specify C++ to uncrustify (I don't know it possible)?

Both are possible; there is a CMake directive to force uncrustify to treat all files as C++.

But since our style is to have all C++ header files end in .hpp, I'll suggest we just rename it instead.

Thanks for working on this!

[clalancette]

I think we fixed this in #96, so I'm going to go ahead and close this out. Feel free to reopen if there is further work to do here.