Logging macros aren't safe in all contexts

[clalancette]

Consider the following structure:

if (condition)
    RCUTILS_LOG_ERROR("error");
else
{
    RCUTILS_LOG_INFO("info");
}

With the current ROS2 rcutils, this code will fail to compile with something like:

/home/ubuntu/teleop_twist_ws/src/badmacro/src/badmacro.cpp: In member function ‘int BadMacro::main()’:
/home/ubuntu/teleop_twist_ws/src/badmacro/src/badmacro.cpp:21:7: error: expected ‘}’ before ‘else’
       else
       ^~~~
In file included from /opt/ros/bouncy/include/rclcpp/client.hpp:39:0,
                 from /opt/ros/bouncy/include/rclcpp/callback_group.hpp:23,
                 from /opt/ros/bouncy/include/rclcpp/node_interfaces/node_base_interface.hpp:25,
                 from /opt/ros/bouncy/include/rclcpp/executor.hpp:31,
                 from /opt/ros/bouncy/include/rclcpp/executors/multi_threaded_executor.hpp:24,
                 from /opt/ros/bouncy/include/rclcpp/executors.hpp:21,
                 from /opt/ros/bouncy/include/rclcpp/rclcpp.hpp:144,
                 from /home/ubuntu/teleop_twist_ws/src/badmacro/src/badmacro.cpp:3:

I have a minimal reproducer of this problem over at https://github.com/clalancette/badmacro . We probably need to wrap all of these macros in the do { ... } while(0) construct to make them safe in these circumstances.

[dirk-thomas]

We can certainly make the macros not robust but calling a macro like this (without the curly braces) is simply "wrong" / very bad practice. The logging macro could even be compiled out for performance in which case the calling code really needs to use curly braces (if you don't want to replace the macro with a no-op).

[clalancette]

We can certainly make the macros not robust but calling a macro like this (without the curly braces) is simply "wrong" / very bad practice. The logging macro could even be compiled out for performance in which case the calling code really needs to use curly braces (if you don't want to replace the macro with a no-op).

Ah, good point, I had forgotten about the fact that the macros could be compiled out. I ended up opening a PR on the original problem code to add the braces: ros-drivers/joystick_drivers#126 . I still think it is a good idea to make the macros more robust, so I'll leave this open (though it is low priority).

[dirk-thomas]

I still think it is a good idea to make the macros more robust

If you think it is an improvement (I completely agree) I would suggest to implement the suggested change now than adding it to the (already huge) backlog (under the assumption that it is a fairly small change anddoesn't take much time).

[mikaelarguedas]

It will require a semi-colon after each macro call, doesn't it ? (not saying it shouldn't be done just that it may be more work than just updating the macro definitions)

[dirk-thomas]

It will require a semi-colon after each macro call, doesn't it ?

Why would it? It depends on what the macro expands to. Since optionally it could expand to nothing I don't think it the macro "call" should be followed by a semicolon since that would be left standing on its own in that case.

[clalancette]

It will require a semi-colon after each macro call, doesn't it ? (not saying it shouldn't be done just that it may be more work than just updating the macro definitions)

It's more subtle than that, unfortunately. The two-line change I just made was to add do...while(0); (note the trailing semicolon) to the RCUTILS_LOG_COND_NAMED macro. That suffices to wrap the whole macro expansion, but it has two problems:

1. If any invocations of RCUTILS_LOG_* do have a semicolon on the end, they will now get a double semicolon (which I think trips up at least one of our compilers)
2. If any invocations of RCUTILS_LOG_* do have a semicolon on the end, they will still not be safe in a curly-brace free expression, since they will evaluate to two statements: do { } while(0); and the empty statement

The other way to do it is to remove the semi-colon from RCUTILS_LOG_COND_NAMED, and then update all users of the macros to have semi-colons. That's actually my preferred solution, but is a much bigger change as @mikaelarguedas points out.

Why would it? It depends on what the macro expands to. Since optionally it could expand to nothing I don't think it the macro "call" should be followed by a semicolon since that would be left standing on its own in that case.

But what's nice about requiring the semicolon at the call site is that it is always syntactically valid, even without curly-braces. That is, given the following code:

if (condition)
    RCUTILS_LOG_ERROR(...);
else {
    do_something();
}

If RCUTILS_LOG_ERROR expands to what we have today plus the do..while(0) loop, it will look like:

if (condition)
    do { rcutils_logging_initialize() ... } while(0);
else {
    do_something();
}

On the other hand, if If RCUTILS_LOG_ERROR expands to empty, then it looks like:

if (condition)
    ;
else {
    do_something();
}

which is still syntactically valid.

[mikaelarguedas]

Yeah that's what I meant by when saying it will require a semi-colon. 👍
I still think it's valid to make them safe and require the trailing semi-colon.

On the other hand our styleguide and linters are meant to prevent the "no curly-braces" situation from happening.

[dirk-thomas]

A semicolon after the macro only works when you can make assumptions about how the macro is being implemented. Otherwise you end up with an "extra" semicolon - which is fine in some cases (e.g. after a do/while) but not in others. That kind of coupling is to be avoided imo.

[sloretz]

A semicolon after the macro only works when you can make assumptions about how the macro is being implemented. Otherwise you end up with an "extra" semicolon - which is fine in some cases (e.g. after a do/while) but not in others. That kind of coupling is to be avoided imo.

I disagree, I think that assumption/coupling should be encouraged. If a macro looks like a function call then it should behave like one by requiring a ; after it.

[dirk-thomas]

Just a brief list of references I found ad hoc of our current practice not to place a semicolon after a macro:

• ros2/demos@fb69a37
• ros2/demos@47752ea
• ros2/rcl@424baba
• ros2/rcl@f5a75e2
• ros2/rclcpp@196b5a3
• ros2/rmw_implementation@c685508
• ros2/turtlebot2_demo@de389bc

Imo we chose this for a reason and don't think we should change this now for this specific ticket. The feature described in this ticket should be implemented following our current practice.

We could certainly open a discussion about the topic in a separate ticket (I don't think we should but I encourage everyone to do that and describe the pros and cons to convince the team that a change should happen). But it should prevent moving forward on this item since the decision is pretty much orthogonal (if a change is made the whole code base would need to be updated accordingly).

[clalancette]

But it should prevent moving forward on this item since the decision is pretty much orthogonal (if a change is made the whole code base would need to be updated accordingly).

Unfortunately, it is not completely orthogonal. The problem is that we have two choices on how to update the macro:

1. do { ... } while(0); - This will work with the current codebase, but will cause anywhere that does use RCUTILS_LOG_ERROR(); to have a double semicolon. There are examples of this in the ROS2 core (https://github.com/ros2/demos/blob/master/demo_nodes_cpp_native/src/talker.cpp#L47), plus we will almost certainly have examples that come in during ports. These are (potentially) vulnerable to the problem that I pointed out in point 2 of Logging macros aren't safe in all contexts #113 (comment).
2. do { ... } while(0) - This will not compile with the current codebase, since we'll have unterminated statements all over the place. In my opinion, this is actually the correct thing to do; it makes it so that the macros are valid in basically all circumstances. While our linters and style can call for not using the semicolon, those will not apply to downstream packages which are now potentially unsafe.

[clalancette]

After we discussed this, the general consensus was to go with option 2 in #113 (comment); that is, add semicolons everywhere. I'm going to use this issue as the meta-issue to track all of the PRs to make this happen. Here is the list:

• Add a semicolon to the end of all RCUTILS and RCLCPP macros. turtlebot2_demo#95
• Add semicolons to all RCLCPP and RCUTILS macros. rviz#357
• Add semicolons to all RCLCPP and RCUTILS macros. rmw_opensplice#244
• Add semicolons to all RCLCPP and RCUTILS macros. rmw_fastrtps#229
• Add semicolons to all RCLCPP and RCUTILS macros. rmw_connext#304
• Add semicolons to RCLCPP and RCUTILS macros. rmw#150
• Add macro semicolons #120
• Add semicolons to all RCLCPP and RCUTILS macros. rclcpp#565
• Add macro semicolons rcl#303
• Add semicolons to RCUTILS macros. navigation#34
• Add semicolons to all RCLCPP and RCUTILS macros. examples#214
• Add semicolons to all RCLCPP and RCUTILS macros. demos#278

And here is CI for the whole lot:

• Linux
• Linux-aarch64
• macOS
• Windows
• Linux Turtlebot

[mjcarroll]

All looks good to me, pending CI.

[clalancette]

Just to be sure, I checked out all packages that are currently released into Bouncy to see which ones would be affected by this change. Outside of the PRs above, no additional Bouncy packages would need changes. This doesn't mean other, unreleased packages won't need to be changed, but the number should be small enough that we can deal with it as they come in. With that, I've merged all of the PRs for this, so I'll close this out.