Skip to content
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

Alignment of long double types #121

Open
thomasmoore-torc opened this issue Nov 14, 2024 · 3 comments
Open

Alignment of long double types #121

thomasmoore-torc opened this issue Nov 14, 2024 · 3 comments

Comments

@thomasmoore-torc
Copy link

Bug report

Required Info:

  • Operating System:
    • Ubuntu 22.04
  • Installation type:
    • Source
  • Version or commit hash:
    • rolling
  • DDS implementation:
    • Fast-RTPS
  • Client library (if applicable):
    • rclcpp

Steps to reproduce issue

While looking into how data is aligned by CDR, I found this issue which points out that long double types are 16 bytes in length but aligned on 8 byte boundaries. As such, I believe the following changes are required in order to properly compute the max serialized size for a type:

https://github.com/ros2/rosidl_typesupport_fastrtps/blob/rolling/rosidl_typesupport_fastrtps_cpp/resource/msg__type_support.cpp.em#L465-L468

     elif type_.typename == 'long double':
       strlist.append('  last_member_size = array_size * sizeof(long double);')
       strlist.append('  current_alignment += array_size * sizeof(long double) +')
-      strlist.append('    eprosima::fastcdr::Cdr::alignment(current_alignment, sizeof(long double));')
+      strlist.append('    eprosima::fastcdr::Cdr::alignment(current_alignment, 8);')

https://github.com/ros2/rosidl_typesupport_fastrtps/blob/rolling/rosidl_typesupport_fastrtps_c/resource/msg__type_support_c.cpp.em#L621-L624

     elif type_.typename == 'long double':
       strlist.append('  last_member_size = array_size * sizeof(long double);')
       strlist.append('  current_alignment += array_size * sizeof(long double) +')
-      strlist.append('    eprosima::fastcdr::Cdr::alignment(current_alignment, sizeof(long double));')
+      strlist.append('    eprosima::fastcdr::Cdr::alignment(current_alignment, 8);')
@clalancette
Copy link
Contributor

We are considering removing long double completely, as it is underspecified in DDS. See ros2/rosidl_dynamic_typesupport#11 for the arguments.

Given that this is the first issue we've seen about long double in a long time, can you specify how you are using it?

@thomasmoore-torc
Copy link
Author

Given that this is the first issue we've seen about long double in a long time, can you specify how you are using it?

We're not. I noticed the discrepancy and figured I'd point it out.

@clalancette
Copy link
Contributor

We're not. I noticed the discrepancy and figured I'd point it out.

Got it, OK. In that case, I'm going to leave this open as a "will be fixed by removing the offending code".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants