Skip to content

Avoid Mimick argument matching for unconditional patches #3221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ukis666 wants to merge 1 commit into
base: rolling
Choose a base branch
from
+84 −50
Open

Avoid Mimick argument matching for unconditional patches #3221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 60 additions & 36 deletions rclcpp/test/mocking_utils/patch.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#endif

#include <functional>
#include <stdexcept>
#include <string>
#include <type_traits>
#include <utility>
Expand Down Expand Up @@ -386,49 +387,49 @@ template<size_t ID, typename ReturnT, typename ... ArgTs>
class Patch<ID, ReturnT(ArgTs...)>
{
public:
using mock_type = typename PatchTraits<ID, ReturnT(ArgTs...)>::mock_type;

/// Construct a patch.
/**
* \param[in] target Symbol target string, using Mimick syntax
* i.e. "symbol(@scope)?", where scope may be "self" to target the current
* binary, "lib:library_name" to target a given library, "file:path/to/library"
* to target a given file, or "sym:other_symbol" to target the first library
* that defines said symbol.
* \param[in] proxy An indirection to call the target function.
* This indirection must ensure this call goes through the function's
* trampoline, as setup by the dynamic linker.
* \param[in] proxy An indirection that keeps a reference to the target symbol
* in the main executable.
* \return a mocking_utils::Patch instance.
*/
explicit Patch(const std::string & target, std::function<ReturnT(ArgTs...)> proxy)
: target_(target), proxy_(proxy)
{
}
: target_(target), proxy_(std::move(proxy))
{}

// Copy construction and assignment are disabled.
Patch(const Patch &) = delete;
Patch & operator=(const Patch &) = delete;

Patch(Patch && other)
{
mock_ = other.mock_;
other.mock_ = nullptr;
}
Patch(Patch && other) noexcept
: target_stub_(std::exchange(other.target_stub_, MMK_STUB_INVALID)),
self_stub_(std::exchange(other.self_stub_, MMK_STUB_INVALID)),
target_(std::move(other.target_)),
proxy_(std::move(other.proxy_)),
configured_(other.configured_)
{}

Patch & operator=(Patch && other)
Patch & operator=(Patch && other) noexcept
{
if (mock_) {
mmk_reset(mock_);
if (this != &other) {
reset();
target_stub_ = std::exchange(other.target_stub_, MMK_STUB_INVALID);
self_stub_ = std::exchange(other.self_stub_, MMK_STUB_INVALID);
target_ = std::move(other.target_);
proxy_ = std::move(other.proxy_);
configured_ = other.configured_;
}
mock_ = other.mock_;
other.mock_ = nullptr;
return *this;
}

~Patch()
{
if (mock_) {
mmk_reset(mock_);
}
reset();
}

/// Inject a @p replacement for the patched function.
Expand All @@ -446,35 +447,58 @@ class Patch<ID, ReturnT(ArgTs...)>
}

private:
// Helper for template parameter pack expansion using `mmk_any`
// macro as pattern.
template<typename T>
T any() {return mmk_any(T);}

void replace_with(std::function<ReturnT(ArgTs...)> replacement)
{
if (mock_) {
if (configured_) {
throw std::logic_error("Cannot configure patch more than once");
}
auto type_erased_trampoline =
reinterpret_cast<mmk_fn>(prepare_trampoline<ID>(replacement));
auto MMK_MANGLE(mock_type, create) =
PatchTraits<ID, ReturnT(ArgTs...)>::MMK_MANGLE(mock_type, create);
mock_ = mmk_mock(target_.c_str(), mock_type);
mmk_when(proxy_(any<ArgTs>()...), .then_call = type_erased_trampoline);
const auto scope_separator = target_.find('@');
const bool patch_self =
scope_separator != std::string::npos && target_.substr(scope_separator + 1) != "self";
const auto self_target = patch_self ? target_.substr(0, scope_separator) : std::string{};

configured_ = true;
target_stub_ = mmk_stub_create(target_.c_str(), type_erased_trampoline, nullptr);
if (target_stub_ == MMK_STUB_INVALID) {
throw std::runtime_error("Failed to create patch for '" + target_ + "'");
}
if (patch_self) {
self_stub_ = mmk_stub_create(self_target.c_str(), type_erased_trampoline, nullptr);
if (self_stub_ == MMK_STUB_INVALID) {
reset();
throw std::runtime_error("Failed to create patch for '" + target_ + "'");
}
}
}

void reset() noexcept
{
if (self_stub_ != MMK_STUB_INVALID) {
mmk_stub_destroy(self_stub_);
self_stub_ = MMK_STUB_INVALID;
}
if (target_stub_ != MMK_STUB_INVALID) {
mmk_stub_destroy(target_stub_);
target_stub_ = MMK_STUB_INVALID;
}
}

mock_type mock_{nullptr};
struct mmk_stub * target_stub_{MMK_STUB_INVALID};
struct mmk_stub * self_stub_{MMK_STUB_INVALID};
std::string target_;
std::function<ReturnT(ArgTs...)> proxy_;
bool configured_{false};
};

/// Make a patch for a `target` function.
/**
* Useful for type deduction during \ref mocking_utils::Patch construction.
*
* \param[in] target Symbol target string, using Mimick syntax.
* \param[in] proxy An indirection to call the target function.
* \param[in] proxy An indirection that keeps a reference to the target symbol
* in the main executable.
* \return a mocking_utils::Patch instance.
*
* \tparam ID Numerical identifier for this patch. Ought to be unique.
Expand All @@ -490,8 +514,8 @@ auto make_patch(const std::string & target, std::function<SignatureT> proxy)

/// Define a dummy operator `op` for a given `type`.
/**
* Useful to enable patching functions that take arguments whose types
* do not define basic comparison operators, as required by Mimick.
* Retained for compatibility with tests that define comparison operators
* for types passed through the mocking utility.
*/
#define MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(type_, op) \
template<typename T> \
Expand All @@ -509,7 +533,7 @@ auto make_patch(const std::string & target, std::function<SignatureT> proxy)

/// A transparent forwarding proxy to a given `function`.
/**
* Useful to ensure a call to `function` goes through its trampoline.
* Useful to keep the target symbol reachable in the main executable.
*/
#define MOCKING_UTILS_PATCH_PROXY(function) \
[] (auto && ... args)->decltype(auto) { \
Expand Down
38 changes: 24 additions & 14 deletions rclcpp/test/rclcpp/test_utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,8 @@
#include "rclcpp/exceptions.hpp"
#include "rclcpp/utilities.hpp"

#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wordered-compare-function-pointers"
#endif
// TODO(ahcorde): the function mocking_utils::patch_and_return called with
// rcl_logging_configure_with_output_handler is returning: "Comparison between pointer and integer"
// Disabling this warning is fine for now.
// Related issue https://github.com/ros2/rclcpp/issues/2488
#include "../mocking_utils/patch.hpp"

#ifdef __clang__
#pragma clang diagnostic pop
#endif

#include "../utils/rclcpp_gtest_macros.hpp"

TEST(TestUtilities, remove_ros_arguments) {
Expand Down Expand Up @@ -171,6 +159,17 @@ TEST(TestUtilities, test_context_basic_access_const_methods) {
EXPECT_EQ(0u, context1->get_pre_shutdown_callbacks().size());
}

TEST(TestUtilities, patch_install_failure_is_reported) {
auto mock = mocking_utils::make_patch<__COUNTER__, rcl_ret_t(void)>(
"mocking_utils_symbol_that_does_not_exist@self", nullptr);
EXPECT_THROW(
mock.then_call([]() {return RCL_RET_OK;}),
std::runtime_error);
EXPECT_THROW(
mock.then_call([]() {return RCL_RET_OK;}),
std::logic_error);
}

MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(rcl_guard_condition_options_t, ==)
MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(rcl_guard_condition_options_t, !=)
MOCKING_UTILS_BOOL_OPERATOR_RETURNS_FALSE(rcl_guard_condition_options_t, >)
Expand All @@ -187,10 +186,21 @@ TEST(TestUtilities, test_context_init_shutdown_fails) {

{
auto context_fail_init = std::make_shared<rclcpp::contexts::DefaultContext>();
auto mock = mocking_utils::patch_and_return(
"lib:rclcpp", rcl_logging_configure_with_output_handler, RCL_RET_ERROR);
bool replacement_called = false;
rcl_logging_output_handler_t received_output_handler = nullptr;
auto mock = mocking_utils::patch(
"lib:rclcpp", rcl_logging_configure_with_output_handler,
([&replacement_called, &received_output_handler](
const rcl_arguments_t *, const rcl_allocator_t *,
rcl_logging_output_handler_t output_handler) {
replacement_called = true;
received_output_handler = output_handler;
return RCL_RET_ERROR;
}));
EXPECT_THROW(context_fail_init->init(0, nullptr), rclcpp::exceptions::RCLError);
EXPECT_FALSE(context_fail_init->is_valid());
EXPECT_TRUE(replacement_called);
EXPECT_NE(nullptr, received_output_handler);
}

{
Expand Down