-
Notifications
You must be signed in to change notification settings - Fork 412
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.
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
Set explicitly callback type #2059
Conversation
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
This is the backtrace. I think the important part is the
|
this seems correct to me.
So does this line.
callback function can be accessible, but cannot access 1st argument? any thoughts? @iuhilnehc-ynos |
Check this example:
|
@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, |
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
cpp_callback_trampoline can deal with the lambda.
To cast the lambda to std::function by
What I can think of are as follows,
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>
155804a
to
2d36f0c
Compare
Thanks @fujitatomoya for the further investigation. Cool to see the function types from GDB. |
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
2d36f0c
to
ccbfe5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iuhilnehc-ynos 's suggestion makes more sense to me, lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
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 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 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 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 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>
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
assuming that
auto
was deduced to be astd::function<void(size_t)>
.And then, to the trampoline, we say we were passing a function returning
void*
and with an arg of typesize_t
rclcpp/rclcpp/include/rclcpp/client.hpp
Lines 314 to 316 in 338eed0
But when compiling with optimizations, the
auto new_callback = ...
signature seems to change to other type. So then thetrampoline
function failed when did the cast back:rclcpp/rclcpp/include/rclcpp/detail/cpp_callback_trampoline.hpp
Line 60 in 338eed0