Set explicitly callback type by mauropasse · Pull Request #2059 · ros2/rclcpp

mauropasse · 2022-12-08T20:53:38Z

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);

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

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 · 2022-12-09T17:40:48Z

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 · 2022-12-10T00:35:29Z

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 · 2022-12-10T15:12:43Z

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 · 2022-12-12T01:01:37Z

@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 · 2022-12-12T03:31:26Z

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,

the document should be updated to align with this PR, otherwise someone might use auto again in the future.
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));

This reverts commit dfb4c54. Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

mauropasse · 2022-12-12T16:58:01Z

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!

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

fujitatomoya

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

ivanpauno

lgtm!

fujitatomoya · 2022-12-13T18:03:52Z

CI:

Linux
Linux-aarch64
Windows

alsora · 2022-12-16T09:44:47Z

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

* Set explicitly callback type Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Revert "Set explicitly callback type" This reverts commit dfb4c54. Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Add type info to cpp_callback_trampoline Signed-off-by: Mauro Passerino <mpasserino@irobot.com> Signed-off-by: Mauro Passerino <mpasserino@irobot.com> Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

Set explicitly callback type

dfb4c54

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

mauropasse requested review from hidmic, ivanpauno and wjwwood as code owners December 8, 2022 20:53

alsora approved these changes Dec 9, 2022

View reviewed changes

fujitatomoya reviewed Dec 9, 2022

View reviewed changes

fujitatomoya approved these changes Dec 12, 2022

View reviewed changes

Revert "Set explicitly callback type"

ebb8c35

This reverts commit dfb4c54. Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

mauropasse force-pushed the mauro/fix-callback-signature branch 3 times, most recently from 155804a to 2d36f0c Compare December 12, 2022 16:45

Add type info to cpp_callback_trampoline

ccbfe5c

Signed-off-by: Mauro Passerino <mpasserino@irobot.com>

mauropasse force-pushed the mauro/fix-callback-signature branch from 2d36f0c to ccbfe5c Compare December 12, 2022 19:43

fujitatomoya approved these changes Dec 12, 2022

View reviewed changes

ivanpauno approved these changes Dec 13, 2022

View reviewed changes

alsora merged commit a20a295 into ros2:rolling Dec 16, 2022

mauropasse deleted the mauro/fix-callback-signature branch December 16, 2022 14:06

mauropasse mentioned this pull request Dec 16, 2022

Add support for timers on reset callback #1979

Merged

mauropasse mentioned this pull request Apr 3, 2023

create_waitable_callback causing segmentation fault irobot-ros/events-executor#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set explicitly callback type#2059

Set explicitly callback type#2059
alsora merged 3 commits intoros2:rollingfrom
mauropasse:mauro/fix-callback-signature

mauropasse commented Dec 8, 2022 •

edited

Loading

Uh oh!

fujitatomoya left a comment

Uh oh!

mauropasse commented Dec 9, 2022

Uh oh!

fujitatomoya commented Dec 10, 2022

Uh oh!

mauropasse commented Dec 10, 2022

Uh oh!

fujitatomoya commented Dec 12, 2022

Uh oh!

iuhilnehc-ynos commented Dec 12, 2022 •

edited

Loading

Uh oh!

mauropasse commented Dec 12, 2022

Uh oh!

fujitatomoya left a comment

Uh oh!

ivanpauno left a comment

Uh oh!

fujitatomoya commented Dec 13, 2022

Uh oh!

alsora commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

Conversation

mauropasse commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

mauropasse commented Dec 9, 2022

Uh oh!

fujitatomoya commented Dec 10, 2022

Uh oh!

mauropasse commented Dec 10, 2022

Uh oh!

fujitatomoya commented Dec 12, 2022

Uh oh!

iuhilnehc-ynos commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mauropasse commented Dec 12, 2022

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

fujitatomoya commented Dec 13, 2022

Uh oh!

alsora commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mauropasse commented Dec 8, 2022 •

edited

Loading

iuhilnehc-ynos commented Dec 12, 2022 •

edited

Loading