Set explicitly callback type

[mauropasse]

This is to fix seg-faults happening when compiling with -O2 or -O3 optimizations.
They happened because we did:

rclcpp/rclcpp/include/rclcpp/client.hpp

Line 289 in 338eed0

auto new_callback =

assuming that auto was deduced to be a std::function<void(size_t)>.

And then, to the trampoline, we say we were passing a function returning void* and with an arg of type size_t

rclcpp/rclcpp/include/rclcpp/client.hpp

Lines 314 to 316 in 338eed0

	set_on_new_response_callback(
	rclcpp::detail::cpp_callback_trampoline<const void *, size_t>,
	static_cast<const void *>(&new_callback));

But when compiling with optimizations, the auto new_callback = ... signature seems to change to other type. So then the trampoline function failed when did the cast back:

rclcpp/rclcpp/include/rclcpp/detail/cpp_callback_trampoline.hpp

Line 60 in 338eed0

auto & actual_callback = *reinterpret_cast<const std::function<ReturnT(Args...)> *>(user_data);

[fujitatomoya]

@mauropasse interesting.

This is to fix seg-faults happening when compiling with -O2 or -O3 optimizations.

could you share the segmentation fault information detail? that might be helpful for other members.

[mauropasse]

This is the backtrace. I think the important part is the #2 where the trampoline does the cast back to std::function<void(size_t)>

#0  std::_Function_handler<void(long unsigned int, int), rclcpp::executors::EventsExecutorEntitiesCollector::create_waitable_callback(void*)::<lambda(size_t, int)> >::_M_invoke(const std::_Any_data &, unsigned long &&, int &&) (
    __functor=..., __args#0=@0x7fffffffdc80: 0, 
    __args#1=<error reading variable: Cannot access memory at address 0x0>)
    at /usr/include/c++/11/bits/std_function.h:290
#1  0x000055555557c959 in std::function<void (unsigned long)>::operator()(unsigned long) const (
    __args#0=<optimized out>, this=<optimized out>) at /usr/include/c++/11/bits/std_function.h:590
#2  rclcpp::detail::cpp_callback_trampoline<void const*, unsigned long, void> (user_data=<optimized out>)
    at /root/callback_issue/install/include/rclcpp/rclcpp/detail/cpp_callback_trampoline.hpp:61
#3  0x00007ffff74e9b7c in PubListener::set_on_new_event_callback(rmw_event_type_e, void const*, void (*)(void const*, unsigned long)) () from /opt/ros/humble/lib/librmw_fastrtps_shared_cpp.so
#4  0x00007ffff74ee44d in rmw_fastrtps_shared_cpp::__rmw_event_set_callback(rmw_event_s*, void (*)(void const*, unsigned long), void const*) () from /opt/ros/humble/lib/librmw_fastrtps_shared_cpp.so
#5  0x00007ffff7ee3472 in rclcpp::QOSEventHandlerBase::set_on_new_event_callback (this=this@entry=0x55555585cff0, 
    callback=callback@entry=0x55555557c930 <rclcpp::detail::cpp_callback_trampoline<void const*, unsigned long, void>(void const*, unsigned long)>, user_data=user_data@entry=0x7fffffffdda0)
    at /root/callback_issue/src/rclcpp/rclcpp/src/rclcpp/qos_event.cpp:79
#6  0x0000555555580a57 in rclcpp::QOSEventHandlerBase::set_on_ready_callback(std::function<void (unsigned long, int)>) (this=this@entry=0x55555585cff0, callback=...)
    at /root/callback_issue/install/include/rclcpp/rclcpp/qos_event.hpp:187

[fujitatomoya]

rclcpp::detail::cpp_callback_trampoline<void const*, unsigned long, void> (user_data=)

this seems correct to me.

0x000055555557c959 in std::function<void (unsigned long)>::operator()(unsigned long) const (

So does this line.

__args#1=<error reading variable: Cannot access memory at address 0x0>)

callback function can be accessible, but cannot access 1st argument?

any thoughts? @iuhilnehc-ynos

[mauropasse]

Check this example:
https://onlinegdb.com/pdJ6u2gUs

  auto auto_function = [](size_t n){cout << n << endl;};
  std::function<void(size_t)> explicit_function = [](size_t n){cout << n << endl;};

  if (std::is_same<decltype(auto_function), decltype(explicit_function)>::value)
  {
    cout<<"Functions are same type" << endl;
  } else {
    cout<<"Functions are different type" << endl; # This is printed
  }

[fujitatomoya]

@mauropasse thanks for the study.

(gdb) whatis auto_function
type = <lambda(size_t)>

(gdb) whatis explicit_function
type = std::function<void(long unsigned int)>

(gdb) ptype auto_function
type = struct <lambda(size_t)> {
}

(gdb) ptype explicit_function
type = class std::function<void(long unsigned int)> [with _Signature = void (unsigned long)]
        : public std::_Maybe_unary_or_binary_function<void, unsigned long>, private std::_Function_base {
  private:
    _Invoker_type _M_invoker;

  public:
    function(void);
    function(std::nullptr_t);
    function(const std::function<void(long unsigned int)> &);
    function(std::function<void(long unsigned int)> &&);
    std::function<void(long unsigned int)> & operator=(const std::function<void(long unsigned int)> &);
    std::function<void(long unsigned int)> & operator=(std::function<void(long unsigned int)> &&);
    std::function<void(long unsigned int)> & operator=(std::nullptr_t);
    void swap(std::function<void(long unsigned int)> &);
    operator bool(void) const;
    void operator()(unsigned long) const;
    const std::type_info & target_type(void) const;
    void function<main()::<lambda(size_t)> >(<lambda(size_t)>);

  private:
    typedef void (*_Invoker_type)(const std::_Any_data &, unsigned long &&);
}

reference: https://stackoverflow.com/questions/36030589/i-cannot-pass-lambda-as-stdfunction,

[iuhilnehc-ynos]

This fix seems good to me, but I think it's just a workaround.

According to

rclcpp/rclcpp/include/rclcpp/detail/cpp_callback_trampoline.hpp

Lines 37 to 38 in 2e39e09

	* This should allow you to use free functions, lambdas with and without
	* captures, and various kinds of std::bind instances.

, I think that the original intention of cpp_callback_trampoline can deal with the lambda.

To cast the lambda to std::function by

rclcpp/rclcpp/include/rclcpp/detail/cpp_callback_trampoline.hpp

Line 60 in 2e39e09

auto & actual_callback = *reinterpret_cast<const std::function<ReturnT(Args...)> *>(user_data);

is incorrect.

What I can think of are as follows,

1. the document should be updated to align with this PR, otherwise someone might use auto again in the future.

2. or another workaround to add type information to the template, such as

template<
  typename UserDataRealT,
  typename UserDataT,
  typename ... Args,
  typename ReturnT = void
>
ReturnT
cpp_callback_trampoline(UserDataT user_data, Args ... args) noexcept
{
  auto & actual_callback = *static_cast<const UserDataRealT *>(user_data);
  return actual_callback(args ...);
}

...
    set_on_new_event_callback(
      rclcpp::detail::cpp_callback_trampoline<decltype(new_callback), const void *, size_t>,
      static_cast<const void *>(&new_callback));

[mauropasse]

Thanks @fujitatomoya for the further investigation. Cool to see the function types from GDB.
@iuhilnehc-ynos I've reverted the first commit and applied your suggestion, so we still support what the documentation says. Thanks for that!

[fujitatomoya]

@iuhilnehc-ynos 's suggestion makes more sense to me, lgtm.

[ivanpauno]

lgtm!

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[alsora]

I'm merging this, since we have 3 approvals and green CI.