Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix conversion of negative durations to messages #1188

Merged
merged 6 commits into from
Jun 29, 2020
Merged

Fix conversion of negative durations to messages #1188

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
597817d
Fix conversion from negative Duration or Time to the respective messa…
meyerj Jun 19, 2020
cf6c8c3
Add -Wconversion compiler option and fix implicit conversions that mi…
meyerj Jun 19, 2020
3628064
Fix expected exception type in TestDuration.conversions test case
meyerj Jun 20, 2020
683ac64
Fix usage of fixture class in some unit tests
meyerj Jun 20, 2020
f9cf569
Address PR comments and unify argument name clock_type in Time() cons…
meyerj Jun 25, 2020
f5fed09
Add compiler option -Wno-sign-conversion to fix build with Clang on m…
meyerj Jun 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rclcpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ if(NOT CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 14)
endif()
if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wpedantic -Wnon-virtual-dtor -Woverloaded-virtual)
add_compile_options(-Wall -Wextra -Wconversion -Wpedantic -Wnon-virtual-dtor -Woverloaded-virtual)
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
endif()

set(${PROJECT_NAME}_SRCS
Expand Down
35 changes: 19 additions & 16 deletions rclcpp/src/rclcpp/duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,14 @@ Duration::Duration(std::chrono::nanoseconds nanoseconds)
rcl_duration_.nanoseconds = nanoseconds.count();
}

Duration::Duration(const Duration & rhs)
{
rcl_duration_.nanoseconds = rhs.rcl_duration_.nanoseconds;
}
Duration::Duration(const Duration & rhs) = default;

Duration::Duration(
const builtin_interfaces::msg::Duration & duration_msg)
{
rcl_duration_.nanoseconds =
static_cast<rcl_duration_value_t>(RCL_S_TO_NS(duration_msg.sec));
rcl_duration_.nanoseconds += duration_msg.nanosec;
RCL_S_TO_NS(static_cast<rcl_duration_value_t>(duration_msg.sec));
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
rcl_duration_.nanoseconds += static_cast<rcl_duration_value_t>(duration_msg.nanosec);
}

Duration::Duration(const rcl_duration_t & duration)
Expand All @@ -69,24 +66,26 @@ Duration::Duration(const rcl_duration_t & duration)
Duration::operator builtin_interfaces::msg::Duration() const
{
builtin_interfaces::msg::Duration msg_duration;
msg_duration.sec = static_cast<std::int32_t>(RCL_NS_TO_S(rcl_duration_.nanoseconds));
msg_duration.nanosec =
static_cast<std::uint32_t>(rcl_duration_.nanoseconds % (1000 * 1000 * 1000));
constexpr rcl_duration_value_t kRemainder = RCL_S_TO_NS(1);
meyerj marked this conversation as resolved.
Show resolved Hide resolved
const auto result = std::div(rcl_duration_.nanoseconds, kRemainder);
if (result.rem >= 0) {
msg_duration.sec = static_cast<std::int32_t>(result.quot);
msg_duration.nanosec = static_cast<std::uint32_t>(result.rem);
} else {
msg_duration.sec = static_cast<std::int32_t>(result.quot - 1);
msg_duration.nanosec = static_cast<std::uint32_t>(kRemainder + result.rem);
}
return msg_duration;
}

Duration &
Duration::operator=(const Duration & rhs)
{
rcl_duration_.nanoseconds = rhs.rcl_duration_.nanoseconds;
return *this;
}
Duration::operator=(const Duration & rhs) = default;

Duration &
Duration::operator=(const builtin_interfaces::msg::Duration & duration_msg)
{
rcl_duration_.nanoseconds = RCL_S_TO_NS(static_cast<int64_t>(duration_msg.sec));
rcl_duration_.nanoseconds += duration_msg.nanosec;
rcl_duration_.nanoseconds = RCL_S_TO_NS(static_cast<rcl_duration_value_t>(duration_msg.sec));
rcl_duration_.nanoseconds += static_cast<rcl_duration_value_t>(duration_msg.nanosec);
ivanpauno marked this conversation as resolved.
Show resolved Hide resolved
return *this;
}

Expand Down Expand Up @@ -230,6 +229,10 @@ Duration::seconds() const
rmw_time_t
Duration::to_rmw_time() const
{
if (rcl_duration_.nanoseconds < 0) {
throw std::runtime_error("rmw_time_t cannot be negative");
}

// reuse conversion logic from msg creation
builtin_interfaces::msg::Duration msg = *this;
rmw_time_t result;
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ __lockless_has_parameter(
// see https://en.cppreference.com/w/cpp/types/numeric_limits/epsilon
RCLCPP_LOCAL
bool
__are_doubles_equal(double x, double y, size_t ulp = 100)
__are_doubles_equal(double x, double y, double ulp = 100.0)
{
return std::abs(x - y) <= std::numeric_limits<double>::epsilon() * std::abs(x + y) * ulp;
}
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/src/rclcpp/node_options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ NodeOptions::get_domain_id_from_env() const
_dupenv_s(&ros_domain_id, &ros_domain_id_size, env_var);
#endif
if (ros_domain_id) {
uint32_t number = strtoul(ros_domain_id, NULL, 0);
uint32_t number = static_cast<uint32_t>(strtoul(ros_domain_id, NULL, 0));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This static_cast just makes explicit what the compiler would also have done implicitly.

The check below may also not do what is expected: strtoul() converts the string to unsigned long, and returns ULONG_MAX in case of an overflow. On my machine (Linux 64-bit) unsigned long is a 64 bit type, which is also why the implicit conversion to uint32_t triggered a compiler warning with -Wconversion. But that also means that only values exceeding 18446744073709551617 and all multiples of 4294967296 minus 1 trigger the run-time exception. 4294967296 is silently cast to 0, 4294967297 to 1 etc.

Illegal values like "foo" would also silently be interpreted as 0 without an error, because strtoul simply stops parsing at the first invalid character. You would have to pass a char ** as the second argument str_end instead of NULL and check whether it points to the end of the string (the terminating null character).

The conversions should probably use C++ stringstreams or the C++11 equivalent std::stoul with an additional range check, instead of error-prone C functions. Or, if the function must be implemented in C to avoid code duplication with rcl, as stated in @wjwwood's comment above, the solution would be to use unsigned long instead of uint32_t without a cast, check that the str_end actually points to the terminating null character after conversion, and to add an explicit range check before the cast to the desired output type.

Apparently the issue is already discussed in ros2/rcl#689.

if (number == (std::numeric_limits<uint32_t>::max)()) {
#ifdef _WIN32
// free the ros_domain_id before throwing, if getenv was used on Windows
Expand Down
23 changes: 11 additions & 12 deletions rclcpp/src/rclcpp/time.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,7 @@ Time::Time(int64_t nanoseconds, rcl_clock_type_t clock_type)
rcl_time_.nanoseconds = nanoseconds;
}

Time::Time(const Time & rhs)
: rcl_time_(rhs.rcl_time_)
{
rcl_time_.nanoseconds = rhs.rcl_time_.nanoseconds;
}
Time::Time(const Time & rhs) = default;

Time::Time(
const builtin_interfaces::msg::Time & time_msg,
Expand Down Expand Up @@ -95,17 +91,20 @@ Time::~Time()
Time::operator builtin_interfaces::msg::Time() const
{
builtin_interfaces::msg::Time msg_time;
msg_time.sec = static_cast<std::int32_t>(RCL_NS_TO_S(rcl_time_.nanoseconds));
msg_time.nanosec = static_cast<std::uint32_t>(rcl_time_.nanoseconds % (1000 * 1000 * 1000));
constexpr rcl_time_point_value_t kRemainder = RCL_S_TO_NS(1);
const auto result = std::div(rcl_time_.nanoseconds, kRemainder);
if (result.rem >= 0) {
msg_time.sec = static_cast<std::int32_t>(result.quot);
msg_time.nanosec = static_cast<std::uint32_t>(result.rem);
} else {
msg_time.sec = static_cast<std::int32_t>(result.quot - 1);
msg_time.nanosec = static_cast<std::uint32_t>(kRemainder + result.rem);
}
return msg_time;
}

Time &
Time::operator=(const Time & rhs)
{
rcl_time_ = rhs.rcl_time_;
return *this;
}
Time::operator=(const Time & rhs) = default;

Time &
Time::operator=(const builtin_interfaces::msg::Time & time_msg)
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ TEST_F(TestMultiThreadedExecutor, timer_over_take) {

{
std::lock_guard<std::mutex> lock(last_mutex);
double diff = std::abs((now - last).nanoseconds()) / 1.0e9;
double diff = static_cast<double>(std::abs((now - last).nanoseconds())) / 1.0e9;
last = now;

if (diff < PERIOD - TOLERANCE) {
Expand Down
105 changes: 94 additions & 11 deletions rclcpp/test/rclcpp/test_duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ class TestDuration : public ::testing::Test
{
};

// TEST(TestDuration, conversions) {
// TODO(tfoote) Implement conversion methods
// }

TEST(TestDuration, operators) {
TEST_F(TestDuration, operators) {
rclcpp::Duration old(1, 0);
rclcpp::Duration young(2, 0);

Expand Down Expand Up @@ -67,7 +63,7 @@ TEST(TestDuration, operators) {
EXPECT_TRUE(time == assignment_op_duration);
}

TEST(TestDuration, chrono_overloads) {
TEST_F(TestDuration, chrono_overloads) {
int64_t ns = 123456789l;
auto chrono_ns = std::chrono::nanoseconds(ns);
auto d1 = rclcpp::Duration(ns);
Expand All @@ -86,7 +82,7 @@ TEST(TestDuration, chrono_overloads) {
EXPECT_EQ(chrono_float_seconds, d5.to_chrono<decltype(chrono_float_seconds)>());
}

TEST(TestDuration, overflows) {
TEST_F(TestDuration, overflows) {
rclcpp::Duration max(std::numeric_limits<rcl_duration_value_t>::max());
rclcpp::Duration min(std::numeric_limits<rcl_duration_value_t>::min());

Expand All @@ -107,7 +103,7 @@ TEST(TestDuration, overflows) {
EXPECT_THROW(base_d_neg * 4, std::underflow_error);
}

TEST(TestDuration, negative_duration) {
TEST_F(TestDuration, negative_duration) {
rclcpp::Duration assignable_duration = rclcpp::Duration(0) - rclcpp::Duration(5, 0);

{
Expand All @@ -130,26 +126,113 @@ TEST(TestDuration, negative_duration) {
}
}

TEST(TestDuration, maximum_duration) {
TEST_F(TestDuration, maximum_duration) {
rclcpp::Duration max_duration = rclcpp::Duration::max();
rclcpp::Duration max(std::numeric_limits<int32_t>::max(), 999999999);

EXPECT_EQ(max_duration, max);
}

static const int64_t HALF_SEC_IN_NS = 500 * 1000 * 1000;
static const int64_t ONE_SEC_IN_NS = 1000 * 1000 * 1000;
static const int64_t ONE_AND_HALF_SEC_IN_NS = 3 * HALF_SEC_IN_NS;

TEST(TestDuration, from_seconds) {
TEST_F(TestDuration, from_seconds) {
EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration::from_seconds(0.0));
EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration::from_seconds(0));
EXPECT_EQ(rclcpp::Duration(1, HALF_SEC_IN_NS), rclcpp::Duration::from_seconds(1.5));
EXPECT_EQ(rclcpp::Duration(-ONE_AND_HALF_SEC_IN_NS), rclcpp::Duration::from_seconds(-1.5));
}

TEST(TestDuration, std_chrono_constructors) {
TEST_F(TestDuration, std_chrono_constructors) {
EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration(0.0s));
EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration(0s));
EXPECT_EQ(rclcpp::Duration(1, HALF_SEC_IN_NS), rclcpp::Duration(1.5s));
EXPECT_EQ(rclcpp::Duration(-1, 0), rclcpp::Duration(-1s));
}

TEST_F(TestDuration, conversions) {
{
const rclcpp::Duration duration(HALF_SEC_IN_NS);
const auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, 0);
EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), HALF_SEC_IN_NS);

const auto rmw_time = duration.to_rmw_time();
EXPECT_EQ(rmw_time.sec, 0u);
EXPECT_EQ(rmw_time.nsec, static_cast<uint64_t>(HALF_SEC_IN_NS));

const auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), HALF_SEC_IN_NS);
}

{
const rclcpp::Duration duration(ONE_SEC_IN_NS);
const auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, 1);
EXPECT_EQ(duration_msg.nanosec, 0u);
EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), ONE_SEC_IN_NS);

const auto rmw_time = duration.to_rmw_time();
EXPECT_EQ(rmw_time.sec, 1u);
EXPECT_EQ(rmw_time.nsec, 0u);

const auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), ONE_SEC_IN_NS);
}

{
const rclcpp::Duration duration(ONE_AND_HALF_SEC_IN_NS);
auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, 1);
EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), ONE_AND_HALF_SEC_IN_NS);

auto rmw_time = duration.to_rmw_time();
EXPECT_EQ(rmw_time.sec, 1u);
EXPECT_EQ(rmw_time.nsec, static_cast<uint64_t>(HALF_SEC_IN_NS));

auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), ONE_AND_HALF_SEC_IN_NS);
}

{
rclcpp::Duration duration(-HALF_SEC_IN_NS);
auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, -1);
EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), -HALF_SEC_IN_NS);

EXPECT_THROW(duration.to_rmw_time(), std::runtime_error);

auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), -HALF_SEC_IN_NS);
}

{
rclcpp::Duration duration(-ONE_SEC_IN_NS);
auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, -1);
EXPECT_EQ(duration_msg.nanosec, 0u);
EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), -ONE_SEC_IN_NS);

EXPECT_THROW(duration.to_rmw_time(), std::runtime_error);

auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), -ONE_SEC_IN_NS);
}

{
rclcpp::Duration duration(-ONE_AND_HALF_SEC_IN_NS);
auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
EXPECT_EQ(duration_msg.sec, -2);
EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), -ONE_AND_HALF_SEC_IN_NS);

EXPECT_THROW(duration.to_rmw_time(), std::runtime_error);

auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
EXPECT_EQ(chrono_duration.count(), -ONE_AND_HALF_SEC_IN_NS);
}
}
Loading