Skip to content

Conversation

@hidmic
Copy link
Contributor

@hidmic hidmic commented Aug 10, 2020

This pull request deals with binary API nuances. Fixes regressions introduced by #272.

CI up to rcutils:

  • Linux (default) Build Status
  • Linux (release) Build Status

Deal with binary API nuances.

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic hidmic added the bug Something isn't working label Aug 10, 2020

/// Patch vsnprintf with the given `replacement` in the given `scope`.
#define patch_vsnprintf(scope, replacement) patch( \
scope, __vsnprintf_chk, [&](char * __str, size_t, int, size_t __size, \
Copy link
Member

Choose a reason for hiding this comment

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

I think this kind of mocking is really fragile.

I prefer having less coverage and not doing this, but I guess it's not my call.

Copy link
Contributor Author

@hidmic hidmic Aug 10, 2020

Choose a reason for hiding this comment

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

I think this kind of mocking is really fragile.

Indeed mocking system libraries this way is fragile, but as of today the alternative is not having coverage at all. Perhaps, eventually we can migrate towards fault injection (which has its own share of ifs and buts, like Release binary and Debug binaries not being the same).

I prefer having less coverage

Generally speaking, I don't agree with reducing test coverage simply because it's inconvenient. I'd much rather look for a replacement if this becomes a pain point.

@hidmic
Copy link
Contributor Author

hidmic commented Aug 10, 2020

Thanks for the reviews!

@hidmic hidmic merged commit 4c9355e into master Aug 10, 2020
@clalancette clalancette deleted the hidmic/fix-vsnprintf-mocks-in-release branch August 10, 2020 19:52
@nuclearsandwich
Copy link
Member

@hidmic the change introduced here is causing build failures in Release configuration on CentOS:

There are several related errors but here's the first one:

In file included from /home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/test_shared_library.cpp:20:
/home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/test_shared_library.cpp: In member function ‘virtual void TestSharedLibrary_basic_load_Test::TestBody()’:
/home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/./mocking_utils/stdio.hpp:67:12: error: ‘__vsnprintf_chk’ was not declared in this scope
     scope, __vsnprintf_chk, [&](char * __str, size_t, int, size_t __size, \
            ^~~~~~~~~~~~~~~
/home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/./mocking_utils/patch.hpp:334:36: note: in definition of macro ‘patch’
   make_patch<__COUNTER__, decltype(function)>( \
                                    ^~~~~~~~
/home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/test_shared_library.cpp:64:32: note: in expansion of macro ‘patch_vsnprintf’
     auto mock = mocking_utils::patch_vsnprintf(
                                ^~~~~~~~~~~~~~~
/home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/./mocking_utils/stdio.hpp:67:12: note: suggested alternative: ‘__asprintf’
     scope, __vsnprintf_chk, [&](char * __str, size_t, int, size_t __size, \
            ^~~~~~~~~~~~~~~
/home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/./mocking_utils/patch.hpp:334:36: note: in definition of macro ‘patch’
   make_patch<__COUNTER__, decltype(function)>( \
                                    ^~~~~~~~
/home/jenkins-agent/workspace/nightly_linux-centos_extra_rmw_release/ws/src/ros2/rcutils/test/test_shared_library.cpp:64:32: note: in expansion of macro ‘patch_vsnprintf’
     auto mock = mocking_utils::patch_vsnprintf(
                                ^~~~~~~~~~~~~~~

The failure does not occur in Debug or None configurations, only Release and removing this commit unbreaks the build. However since this is a fix for an issue on tier 1 platforms I am not proposing a revert.

@nuclearsandwich
Copy link
Member

This change also does not appear to address the test failures in the nightly_linux_thread_sanitizer job.

@hidmic
Copy link
Contributor Author

hidmic commented Aug 11, 2020

Hmm, this is becoming increasingly painful. I'm surprised about tsan changing binary APIs, though at this point it seems clear that portability in system libraries applies to sources only. I'll check how much coverage worsens without this and revert if we're still in the clear.

ahcorde pushed a commit that referenced this pull request Oct 2, 2020
ahcorde pushed a commit that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants