Skip to content

Fix vsnprintf mocks for Release builds.#274

Merged
hidmic merged 1 commit intomasterfrom
hidmic/fix-vsnprintf-mocks-in-release
Aug 10, 2020
Merged

Fix vsnprintf mocks for Release builds.#274
hidmic merged 1 commit intomasterfrom
hidmic/fix-vsnprintf-mocks-in-release

Conversation

@hidmic
Copy link
Copy Markdown

@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

@hidmic
Fix vsnprintf mocks for Release builds.
16842b2 
Deal with binary API nuances.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added the bug Something isn't working label Aug 10, 2020
@hidmic hidmic mentioned this pull request Aug 10, 2020
Merged
ivanpauno
ivanpauno approved these changes Aug 10, 2020
View reviewed changes
test/mocking_utils/stdio.hpp

/// 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
Copy Markdown
Member

@ivanpauno ivanpauno 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.

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

Copy link
Copy Markdown
Author

@hidmic hidmic Aug 10, 2020 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Member

nuclearsandwich commented Aug 11, 2020

@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:

Details

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
Copy Markdown
Member

nuclearsandwich commented Aug 11, 2020

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

@hidmic
Copy link
Copy Markdown
Author

hidmic commented Aug 11, 2020 •
edited
Loading

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
@hidmic @ahcorde
Fix vsnprintf mocks for Release builds. (#274)
870a3e9 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 6, 2020
@hidmic @ahcorde
Fix vsnprintf mocks for Release builds. (#274)
769899c 
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

+1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hidmic @nuclearsandwich @dirk-thomas @ivanpauno