Disable gtest warning when building in Release #298

v-lopez · 2020-10-28T16:20:27Z

A lot of warnings appear when compiling in release, which becomes a blocking issue if your package uses -Werror=null-dereference.

This removes PR this warning (or error) from the gtest target.

Also discussed but ignored here:
google/googletest#1303

In file included from /root/control/src/pal_statistics/pal_statistics/src/registration_list.cpp:1:
/root/control/src/pal_statistics/pal_statistics/src/registration_list.h: In constructor ‘pal_statistics::RegistrationList::RegistrationList(const std::shared_ptr<rclcpp::Node>&, size_t)’:
/root/control/src/pal_statistics/pal_statistics/src/registration_list.h:70:33: warning: ‘pal_statistics::RegistrationList::node_’ will be initialized after [-Wreorder]
   70 |   std::shared_ptr<rclcpp::Node> node_;
      |                                 ^~~~~
/root/control/src/pal_statistics/pal_statistics/src/registration_list.h:63:7: warning:   ‘int pal_statistics::RegistrationList::last_id_’ [-Wreorder]
   63 |   int last_id_;
      |       ^~~~~~~~
/root/control/src/pal_statistics/pal_statistics/src/registration_list.cpp:5:1: warning:   when initialized here [-Wreorder]
    5 | RegistrationList::RegistrationList(const std::shared_ptr<rclcpp::Node> &node, size_t internal_buffer_capacity)
      | ^~~~~~~~~~~~~~~~
In file included from /opt/ros/foxy/src/gtest_vendor/src/gtest-all.cc:41:
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc: In static member function ‘static void testing::internal::PrettyUnitTestResultPrinter::PrintFailedTests(const testing::UnitTest&)’:
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3262:30: error: potential null pointer dereference [-Werror=null-dereference]
 3262 |     if (!test_case.should_run() || (test_case.failed_test_count() == 0)) {
      |          ~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3262:30: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3267:32: error: potential null pointer dereference [-Werror=null-dereference]
 3267 |       if (!test_info.should_run() || !test_info.result()->Failed()) {
      |            ~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3267:32: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc: In static member function ‘static void testing::internal::PrettyUnitTestResultPrinter::PrintSkippedTests(const testing::UnitTest&)’:
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3287:30: error: potential null pointer dereference [-Werror=null-dereference]
 3287 |     if (!test_case.should_run() || (test_case.skipped_test_count() == 0)) {
      |          ~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3287:30: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3292:32: error: potential null pointer dereference [-Werror=null-dereference]
 3292 |       if (!test_info.should_run() || !test_info.result()->Skipped()) {
      |            ~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/foxy/src/gtest_vendor/src/gtest.cc:3292:32: error: potential null pointer dereference [-Werror=null-dereference]

clalancette · 2020-10-28T16:42:50Z

ament_cmake_gtest/ament_cmake_gtest-extras.cmake

@@ -87,6 +87,7 @@ macro(_ament_cmake_gtest_find_gtest)
        # mark gtest targets with EXCLUDE_FROM_ALL to only build
        # when tests are built which depend on them
        set_target_properties(gtest gtest_main PROPERTIES EXCLUDE_FROM_ALL 1)
+	target_compile_options(gtest PRIVATE -Wno-null-dereference)


There are a couple of problems here.

The small problem is that the indentation is wrong.

The larger problem is that this won't work in a cross-platform way. It might work on clang on macOS, but it definitely will not work on MSVC on Windows. So we can't do it this way. My preference would be to actually fix the problem in googletest itself with a patch to https://github.com/google/googletest. If we can't get traction there, then a patch to https://github.com/ament/googletest would also work.

Which brings me to my last question. We recently updated the Rolling version of Googletest to 1.10.0. Is the issue fixed there?

I apologize for the indentation error, I was careless.

Regarding the MSVC/macOS, I could check the compiler/OS before adding that flag.

I'm currently working on Foxy, I'll give it a shot in rolling.

I'm using the docker ros:rolling, but the last version of gtest_vendor I can install is 1.8.9000-2focal.20200910.213617
How could I try the latest version?

How could I try the latest version?

That's right, it was released after the latest Rolling sync. The easiest thing to do is to enable the rolling testing repository, by changing your /etc/apt/sources.list.d/ros2-latest.list to look like:

deb http://packages.ros.org/ros2-testing/ubuntu focal main

v-lopez · 2020-10-28T18:00:37Z

Thanks.

Yes it is still an issue in 1.10, slightly different line, probably due to some added code:

$ dpkg -l | grep gtest-v
ii  ros-rolling-gtest-vendor                           1.10.9000-1focal.20201020.203927     amd64        The package provides GoogleTest.

In file included from /opt/ros/rolling/src/gtest_vendor/src/gtest-all.cc:41:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc: In static member function ‘static void testing::internal::PrettyUnitTestResultPrinter::PrintFailedTests(const testing::UnitTest&)’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3297:31: error: potential null pointer dereference [-Werror=null-dereference]
 3297 |     if (!test_suite.should_run() || (test_suite.failed_test_count() == 0)) {
      |          ~~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3297:31: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3302:32: error: potential null pointer dereference [-Werror=null-dereference]
 3302 |       if (!test_info.should_run() || !test_info.result()->Failed()) {
      |            ~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3302:32: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc: In static member function ‘static void testing::internal::PrettyUnitTestResultPrinter::PrintSkippedTests(const testing::UnitTest&)’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3322:31: error: potential null pointer dereference [-Werror=null-dereference]
 3322 |     if (!test_suite.should_run() || (test_suite.skipped_test_count() == 0)) {

If I restricted this change to Linux or gcc/clang would it be acceptable?

clalancette · 2020-10-28T18:37:54Z

If I restricted this change to Linux or gcc/clang would it be acceptable?

I still think we should fix this in the upstream googletest (or worst case, our googletest fork). Otherwise, we are disabling this flag for all code that is in tests, which could lead to some bugs.

v-lopez · 2020-10-28T19:03:41Z

I believe it will only affect the gtest library since it's target_compile_options and not the rest of files that link with it.

I've tested forcing a nulptr dereference with this changes and it was detected.

Also fixing it is not trivial, I just added a few of the errors in there, but there are quite a few:

/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3297:31: error: potential null pointer dereference [-Werror=null-dereference]
 3297 |     if (!test_suite.should_run() || (test_suite.failed_test_count() == 0)) {
      |          ~~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3297:31: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3302:32: error: potential null pointer dereference [-Werror=null-dereference]
 3302 |       if (!test_info.should_run() || !test_info.result()->Failed()) {
      |            ~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3302:32: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc: In static member function ‘static void testing::internal::PrettyUnitTestResultPrinter::PrintSkippedTests(const testing::UnitTest&)’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3322:31: error: potential null pointer dereference [-Werror=null-dereference]
 3322 |     if (!test_suite.should_run() || (test_suite.skipped_test_count() == 0)) {
      |          ~~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3322:31: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3327:32: error: potential null pointer dereference [-Werror=null-dereference]
 3327 |       if (!test_info.should_run() || !test_info.result()->Skipped()) {
      |            ~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3327:32: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc: In static member function ‘static void testing::internal::XmlUnitTestResultPrinter::PrintXmlTestSuite(std::ostream*, const testing::TestSuite&)’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3879:49: error: potential null pointer dereference [-Werror=null-dereference]
 3879 |     if (test_suite.GetTestInfo(i)->is_reportable())
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:3879:49: error: potential null pointer dereference [-Werror=null-dereference]
In file included from /opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:122,
                 from /opt/ros/rolling/src/gtest_vendor/src/gtest-all.cc:41:
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h: In static member function ‘static void testing::internal::XmlUnitTestResultPrinter::PrintXmlUnitTest(std::ostream*, const testing::UnitTest&)’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:62: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:43: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
      |                                           ^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:62: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:43: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
      |                                           ^~
In file included from /opt/ros/rolling/src/gtest_vendor/src/gtest-all.cc:41:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc: In static member function ‘static void testing::internal::JsonUnitTestResultPrinter::PrintJsonTestSuite(std::ostream*, const testing::TestSuite&)’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:4257:49: error: potential null pointer dereference [-Werror=null-dereference]
 4257 |     if (test_suite.GetTestInfo(i)->is_reportable()) {
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:4257:49: error: potential null pointer dereference [-Werror=null-dereference]
In file included from /opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:122,
                 from /opt/ros/rolling/src/gtest_vendor/src/gtest-all.cc:41:
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h: In static member function ‘static void testing::internal::JsonUnitTestResultPrinter::PrintJsonUnitTest(std::ostream*, const testing::UnitTest&)’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:62: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:43: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
      |                                           ^~
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:62: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
/opt/ros/rolling/src/gtest_vendor/./src/gtest-internal-inl.h:284:43: error: potential null pointer dereference [-Werror=null-dereference]
  284 |   for (typename Container::const_iterator it = c.begin(); it != c.end(); ++it) {
      |                                           ^~
In file included from /opt/ros/rolling/src/gtest_vendor/src/gtest-all.cc:41:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc: In function ‘void testing::TestSuite::Run()’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:2658:8: error: potential null pointer dereference [-Werror=null-dereference]
 2658 |   if (!should_run_) return;
      |        ^~~~~~~~~~~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:2658:8: error: potential null pointer dereference [-Werror=null-dereference]
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc: In member function ‘bool testing::internal::UnitTestImpl::RunAllTests()’:
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:2796:8: error: potential null pointer dereference [-Werror=null-dereference]
 2796 |   if (!should_run_) return;
      |        ^~~~~~~~~~~
/opt/ros/rolling/src/gtest_vendor/./src/gtest.cc:2796:8: error: potential null pointer dereference [-Werror=null-dereference]

v-lopez · 2020-10-29T08:55:29Z

I've rewritten the changes following https://github.com/ament/ament_cmake/blob/master/ament_cmake_gmock/ament_cmake_gmock-extras.cmake#L88 but changing it to target_compile_flags which is more modern CMake.

The PRIVATE keyword ensures that this flags don't extend to libraries that link against gtest/gmock .

https://cmake.org/cmake/help/v3.5/command/target_compile_options.html

hidmic · 2020-10-30T20:37:54Z

@v-lopez FYI, DCO check is failing.

hidmic · 2020-10-30T21:01:05Z

While I understand the need (and convenience), this looks a lot like a problem resulting from not scoping CMake target configurations properly. Arguably because ament_cmake_gtest and ament_cmake_gmock introduce non-imported targets into your package, and your package uses directory-wide (as opposed to target-specific) settings.

We shouldn't have to change how we build gtest and gmock to compensate. What about documenting these targets? That way users can safely reconfigure them externally if need be.

v-lopez · 2020-10-30T22:09:13Z

In my case I'm trying to apply company wide stricter build flags, which I do by asking my colleagues to find_package() a package which runs add_compile_options that, like you said, applies directory-wide settings, and is affecting gtest and gmock targets.

I'm not familiar enough with imported targets so I don't know how it would affect the issue here.

I could also ask my colleagues to add the compile options per target, but it's much harder to enforce on our side, but it would remove the need of this PR.

But since some warnings were disabled too here https://github.com/ament/ament_cmake/blob/master/ament_cmake_gmock/ament_cmake_gmock-extras.cmake#L88 I was hoping we could disable this one too.

I'll fix the DCO right away.

hidmic · 2020-11-02T13:59:12Z

But since some warnings were disabled too here https://github.com/ament/ament_cmake/blob/master/ament_cmake_gmock/ament_cmake_gmock-extras.cmake#L88 I was hoping we could disable this one too.

Hmm, #94 suggests so (although it seems like a workaround that was due to break, and it did for you). @clalancette WDYT?

clalancette · 2020-12-16T21:19:31Z

Hmm, #94 suggests so (although it seems like a workaround that was due to break, and it did for you). @clalancette WDYT?

That's a fair argument. As long as this is scoped just to the compile of gtest itself, I'm OK with it. Here is CI on this:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2020-12-16T21:35:38Z

Oh right, this doesn't work as-is because you can't specify this compiler flag on Windows. This will have to be surrounded with if(NOT WIN32).

google/googletest#1303 Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

clalancette · 2020-12-17T14:14:12Z

New CI:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

v-lopez · 2020-12-17T14:16:55Z

I added the if(NOT WIN32).

clalancette · 2020-12-17T19:56:41Z

All right, the unstable Windows builds are unstable in the nightlies too, so this is good to go. I'll go ahead and merge.

clalancette reviewed Oct 28, 2020

View reviewed changes

v-lopez force-pushed the master branch from d220d7e to cb5ee44 Compare October 28, 2020 17:07

v-lopez force-pushed the master branch 2 times, most recently from 16b56d5 to 0c2e78d Compare October 29, 2020 08:53

v-lopez force-pushed the master branch 2 times, most recently from 543218e to 4695c97 Compare October 30, 2020 22:14

clalancette self-assigned this Nov 18, 2020

Disable gtest warning when building in Release

b86cfcb

google/googletest#1303 Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>

v-lopez force-pushed the master branch from 4695c97 to b86cfcb Compare December 17, 2020 08:41

clalancette approved these changes Dec 17, 2020

View reviewed changes

clalancette merged commit 106e9c5 into ament:master Dec 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable gtest warning when building in Release #298

Disable gtest warning when building in Release #298

v-lopez commented Oct 28, 2020

clalancette Oct 28, 2020

v-lopez Oct 28, 2020

clalancette Oct 28, 2020

v-lopez commented Oct 28, 2020

clalancette commented Oct 28, 2020

v-lopez commented Oct 28, 2020

v-lopez commented Oct 29, 2020

hidmic commented Oct 30, 2020

hidmic commented Oct 30, 2020 •

edited

Loading

v-lopez commented Oct 30, 2020

hidmic commented Nov 2, 2020

clalancette commented Dec 16, 2020

clalancette commented Dec 16, 2020

clalancette commented Dec 17, 2020

v-lopez commented Dec 17, 2020

clalancette commented Dec 17, 2020

Disable gtest warning when building in Release #298

Disable gtest warning when building in Release #298

Conversation

v-lopez commented Oct 28, 2020

clalancette Oct 28, 2020

Choose a reason for hiding this comment

v-lopez Oct 28, 2020

Choose a reason for hiding this comment

clalancette Oct 28, 2020

Choose a reason for hiding this comment

v-lopez commented Oct 28, 2020

clalancette commented Oct 28, 2020

v-lopez commented Oct 28, 2020

v-lopez commented Oct 29, 2020

hidmic commented Oct 30, 2020

hidmic commented Oct 30, 2020 • edited Loading

v-lopez commented Oct 30, 2020

hidmic commented Nov 2, 2020

clalancette commented Dec 16, 2020

clalancette commented Dec 16, 2020

clalancette commented Dec 17, 2020

v-lopez commented Dec 17, 2020

clalancette commented Dec 17, 2020

hidmic commented Oct 30, 2020 •

edited

Loading