cppcoreguidelines-pro-bounds-array-to-pointer-decay on RCLCPP_* macros

[tylerjw]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • binaries
• Version or commit hash:
  • rolling
• DDS implementation:
  • default
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

1. Enable cppcoreguidelines-pro-bounds-array-to-pointer-decay in clang-tidy
2. Run clang-tidy over anything that has a RCLCPP_* macro in it

Expected behavior

No clang-tidy warnings.

Actual behavior

error: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay,-warnings-as-errors]
    RCLCPP_WARN(logger_, "yay, clang-tidy warning!");
    ^
/opt/ros/rolling/include/rclcpp/logging.hpp:970:5: note: expanded from macro 'RCLCPP_WARN'
    RCUTILS_LOG_WARN_NAMED( \
    ^
/opt/ros/rolling/include/rcutils/logging_macros.h:747:3: note: expanded from macro 'RCUTILS_LOG_WARN_NAMED'
  RCUTILS_LOG_COND_NAMED( \
  ^
/opt/ros/rolling/include/rcutils/logging_macros.h:68:5: note: expanded from macro 'RCUTILS_LOG_COND_NAMED'
    RCUTILS_LOGGING_AUTOINIT; \
    ^
/opt/ros/rolling/include/rcutils/logging.h:531:39: note: expanded from macro 'RCUTILS_LOGGING_AUTOINIT'
        RCUTILS_SAFE_FWRITE_TO_STDERR(rcutils_get_error_string().str); \
                                      ^

Feature request

This might not be feasible but I was trying to use this clang-tidy config and it the logging macros create a ton of warnings, not just this one. It would be really nice if we could find some way to replace the macros in RCLCPP with functions. Many of the errors in clang-tidy are because these expand into code from the c libraries. Another example is there are some c-style casts in these macros.

---
Checks:          '*,-fuchsia-*,-google-*,-zircon-*,-abseil-*,-modernize-use-trailing-return-type,-llvm*'
WarningsAsErrors: ''
HeaderFilterRegex: ''
FormatStyle:     none

[aprotyas]

Not an actual response, but I believe logging is usually achieved with macros because of readability and because or else you'd need a bunch of overloads to achieve the same functionality.

[tylerjw]

sbdlog, the library that ros2 is using for logging to files doesn't use macros for the actual logging. See their readme here for an example: https://github.com/gabime/spdlog

[clalancette]

There are 2 main reasons why the logging macros have to be, in fact macros:

1. Since they are macros, users are able to build-time disable them.
2. We want to be able to inject the FUNCTION, LINE, and FILE into the log messages themselves. It isn't totally clear, but the way this is done is that the RCLCPP macros actually call out to the RCUTILS macros, which inject that information.

If we can figure out how to maintain those two properties, we could consider switching everything to templates (though we also have consider whether it is worth it to duplicate the RCUTILS macro functionality only for rclcpp).

[tylerjw]

If we can transition to c++20 there is this source_location that can replace FUNCTION, LINE, and FILE: https://en.cppreference.com/w/cpp/utility/source_location

[tylerjw]

For compile-time disabling of logging, we should be able to evaluate that using constexpr if evaluation. This feature is available in C++17, which we are already using. Here are some examples:

https://blog.tartanllama.xyz/if-constexpr/

[tylerjw]

A lot of this comes down to the complexity of trying to maintain a c-library with a c++ interface. Imo we should use these new features and move more functionality out of the rcl and rcutils libraries and into the rclcpp library. __FUNCTION__ and other features of the c language depend heavily on the pre-processor and are therefore inherently not type-safe and make it difficult to use modern static analysis tooling.

Implementing logging using modern C++ features could be done without removing the C implementation which can continue to serve users who want portability to targets that lack a good C++ compiler. Those implementing a rust client library are probably also struggling with this as rust provides type-safe versions of all of this (compile-time evaluation and source location) and I imagine they would much prefer to use the tooling in rust (as I would prefer c++ tooling for this) than having a tight coupling with the c implementation that relies on the pre-processor.

[tylerjw]

In case it is useful for others, here is my current workaround for being able to use the rclcpp logger and enable the clang-tidy features and warnings I want to use:

/**
 * @brief      Logger that calls rclcpp c macros.
 */
class Logger {
  rclcpp::Logger logger_ = rclcpp::get_logger("");

 public:
  Logger(const std::string& log_namespace)
      : logger_{rclcpp::get_logger(log_namespace)} {}

// These logging macros don't play nicely with this compiler warning
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wformat-security"

  void fatal(const std::string& what) const {
    // These are all NOLINT because clang-tidy doesn't like the c-code they
    // exapnd into.
    RCLCPP_FATAL(logger_, what.c_str());  // NOLINT
  }

  void error(const std::string& what) const {
    // These are all NOLINT because clang-tidy doesn't like the c-code they
    // exapnd into.
    RCLCPP_ERROR(logger_, what.c_str());  // NOLINT
  }

  void warn(const std::string& what) const {
    // These are all NOLINT because clang-tidy doesn't like the c-code they
    // exapnd into.
    RCLCPP_WARN(logger_, what.c_str());  // NOLINT
  }

  void info(const std::string& what) const {
    // These are all NOLINT because clang-tidy doesn't like the c-code they
    // exapnd into.
    RCLCPP_INFO(logger_, what.c_str());  // NOLINT
  }

  void debug(const std::string& what) const {
    // These are all NOLINT because clang-tidy doesn't like the c-code they
    // exapnd into.
    RCLCPP_DEBUG(logger_, what.c_str());  // NOLINT
  }

#pragma GCC diagnostic pop
};

[tylerjw]

Another advantage of decoupling more of the C and C++ client libraries is they can both be more first-class users of their respective languages (and therefore work better with the tooling for each).

The largest downside to decoupling the logging implementation and making it either pure C or pure C++ depending on what client library you are using is that the functionality (and interoperability) will diverge and create more of a maintenance burden.

A third option would be for users of rclcpp to just use sbdlog directly and not rely on logging macros at all. This would reduce the maintenance burden as it would remove code from rclcpp. This would be consistent with the recent changes to make ROS 2 use modern canonical cmake instead of providing a bunch of wrapper methods.

The largest downside to the third option is you don't get to add features to sbdlog for a whole ros system like disabling logging at compile time unless sbdlog already implements that (they might, I just don't know).

[clalancette]

If we can transition to c++20 there is this source_location that can replace FUNCTION, LINE, and FILE: https://en.cppreference.com/w/cpp/utility/source_location

Given all of the things we have to support (including embedded and safety-conscious compilers), and the amount of work to do, we can't realistically switch to C++20 for Humble.

A lot of this comes down to the complexity of trying to maintain a c-library with a c++ interface. Imo we should use these new features and move more functionality out of the rcl and rcutils libraries and into the rclcpp library. ``

As a general policy, that is not what we have been doing and I would not be for this. One of the big problems with ROS 1 was that the rospy and roscpp libraries were completely independent, and thus each had their own sets of quirks and bugs. It also meant that anything to be implemented had to be implemented (at least) twice. The point of having rcl split from the user-facing client libraries was to avoid this duplication. In practice, it actually does seem to work fairly well; while rclcpp usually gets features first, it is often much easier to implement them in rclpy since a lot of functionality is already in rcl. And third-party client libraries also get to use the functionality.

In the particular case of logging, there is more argument to use the language-specific versions of the loggers (since most languages have fairly developed logging libraries). However, it is not a slam-dunk for me there either; there are all kinds of details that would need to be kept in sync for the client libraries to behave similarly (where do files get written to, what does the default log message look like, what environment variables are available for controlling things, etc).

If we can circle back to the original issue, though, I think we should be able to comply with the clang-tidy linters more in the current macros. Either we can make changes so that the checks are happier, or we can disable certain of the checks using NOLINT.

[tylerjw]

we can't realistically switch to C++20 for Humble

I'm not suggesting anyone has time to start trying to refactor away from macros before Humble. That seems like a really large lift. I'm more of just trying to understand what the ideal logging system for ROS should be so I can help us move in that direction. I think you said earlier that one of the large problems with the logging system is we don't have good design documentation for it. Before trying to create that I need to understand what all the concerns are.

there are all kinds of details that would need to be kept in sync for the client libraries to behave similarly (where do files get written to, what does the default log message look like, what environment variables are available for controlling things, etc).

This is why I don't think it makes sense to abandon the logging system and just tell people to use a good logger for their language. There are lots of features in the logging system I'd like to preserve. If the common functionality was in the c library but implemented in functions instead of macros I think the linters would be much more content (as they generally don't check the code you link against, just the code in the specific files you ask them to check).

Generally, if we can avoid using macros which are really a c-language feature in c++ it'll greatly help with using linters.

If we can circle back to the original issue, though, I think we should be able to comply with the clang-tidy linters more in the current macros. Either we can make changes so that the checks are happier, or we can disable certain of the checks using NOLINT.

I can see if I can figure out the specific lines that given a NOLINT would make clang-tidy happier. The format-security warning might be harder to silence because it basically always errors with printf when you format a string with input because that can be exploited in some way I don't understand.

[clalancette]

I can see if I can figure out the specific lines that given a NOLINT would make clang-tidy happier. The format-security warning might be harder to silence because it basically always errors with printf when you format a string with input because that can be exploited in some way I don't understand.

We've run into that warning before. I think the issue is the following:

printf(user_supplied_value);

Is unsafe. That's because the user_supplied_value could have printf specifiers (%s, %d, etc) in it, which can cause printf to start reading arbitrary values from the stack. The safe way to do this is:

printf("%s", user_supplied_value);

Because then any specifiers in it are just literal strings.

That said, in a generic piece of logging code like this, I don't know if we can get rid of that warning.

[tylerjw]

That said, in a generic piece of logging code like this, I don't know if we can get rid of that warning.

My understanding is that sbdlog solves this problem by using the fmt library for formatting. The problem with replacing your printf statements with fmt is that fmt is C++ and we have the restriction that with the current design the logging formatting has to be done in C.