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

Fix stringop-overread compile issues #381

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

svwilliams
Copy link
Contributor

@svwilliams svwilliams commented Sep 16, 2024

When compiling ROS2 Rolling on Ubuntu Noble 24.04 using GCC 13.2.0, there are many instances of the following compiler error:

/usr/lib/gcc/x86_64-linux-gnu/13/include/emmintrin.h:169:19: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ reading 16 or more bytes from a region of size 8 [-Werror=stringop-overread]
  169 |   *(__m128d *)__P = __A;

This seems similar to a previously encountered compiler bug in GCC 12

# include/avxintrin.h:893:24: error: array subscript ‘__m256d_u[0]’ is partly outside array bounds of ‘Eigen::internal::plain_matrix_type<Eigen::Product<Eigen::Matrix<double, 3, 3, 1>, Eigen::Map<Eigen::Matrix<double, -1, 1> >, 0>, Eigen::Dense>::type [1]’ {aka ‘Eigen::Matrix<double, 3, 1> [1]’} [-Werror=array-bounds]
#   893 |   return *(__m256d_u *)__P;

But I do not see any open bug tickets about this issue in either Eigen or GCC. This issue may be entirely harmless; I'm really not sure.

However, every instance seems to occur when an Eigen Fixed-sized object is being assigned to an Eigen Dynamic-sized object. E.g. some code along the lines of:

fuse_core::Vector3d a;
a << 1, 2, 3;
fuse_core::VectorXd b = a;

A vast majority of these assignments occur in unit tests, so fixing those is trivial. I have no concerns about runtime at all:

fuse_core::VectorXd a(3);
a << 1, 2, 3;
fuse_core::VectorXd b = a;

There are a couple of places in actual library code where I update what type of Eigen object is being created, in sensor_proc.hpp and unicycle_2d_ignition.cpp. Both of those files are in the fuse_models package. I do not expect a meaningful performance impact from either of these changes.

And one instance in normal_delta_orientation_3d_cost_functor.hpp in the fuse_constraints package. That one should probably be classified as a bug. The size is known at compile-time; the Eigen::Map should never have be configured for a dynamic size.

Also, the CI build tests are going to fail. There is an API change to the rclcpp::Waitable class that is being fixed here: #379

Copy link
Contributor

@DavidLocus DavidLocus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@henrygerardmoore henrygerardmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying this on top of #379 allowed me to build in release and debug, and fixed the stringop-overread compile problems. 👍

henrygerardmoore added a commit to PickNikRobotics/fuse that referenced this pull request Sep 16, 2024
henrygerardmoore added a commit to PickNikRobotics/fuse that referenced this pull request Sep 16, 2024
@svwilliams svwilliams force-pushed the stringop-overread-compiler-warnings branch from f42650c to 90daa71 Compare September 18, 2024 22:20
@svwilliams svwilliams merged commit b625a17 into rolling Sep 18, 2024
2 checks passed
@svwilliams svwilliams deleted the stringop-overread-compiler-warnings branch September 18, 2024 22:58
giafranchini pushed a commit to giafranchini/fuse that referenced this pull request Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants