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

Print warning if incompatible alignment option chosen #6184

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mvieth
Copy link
Member

@mvieth mvieth commented Nov 26, 2024

Users repeatedly report segmentation faults coming from Memory.h in the Eigen code (aligned_free/handmade_aligned_free). This is because PCL uses Eigen's handmade_aligned_malloc but user code does not use the corresponding handmade_aligned_free (or the other way around), due to chosen SSE/AVX flags and the resulting alignment requirements. PCL's CMake config handles this by automatically setting SSE/AVX options in the user project as they are used in PCL, but not everyone uses CMake. This commit adds a check within PCL's header files for this kind of incompatibility.

At first I considered solving theses incompatibilities automatically by setting EIGEN_MALLOC_ALREADY_ALIGNED and EIGEN_MAX_ALIGN_BYTES, but that would have been more complex and error-prone, so I decided only for checking and warning.

I would have preferred #warning which however is not available on MSVC. #pragma message is an option on MSVC, but this is too easily overlooked IMO. Users have the option to silence this by defining the PCL_SILENCE_MALLOC_WARNING macro (in case of false positive).

Most often, the culprit is the points member in pcl::PointCloud: Aligned memory is allocated within PCL and freed in user code (for example #6167 , #6164 , #6176 ). Maybe we should consider replacing Eigen's aligned_allocator in pcl::PointCloud with a custom allocator that does not decide the alignment based on SSE/AVX options.

@mvieth mvieth added changelog: enhancement Meta-information for changelog generation module: common module: cmake labels Nov 26, 2024
@mvieth mvieth force-pushed the aligned_malloc_warning branch 2 times, most recently from 4391589 to d1af944 Compare November 27, 2024 20:14
Users repeatedly report segmentation faults coming from Memory.h in the Eigen code (aligned_free/handmade_aligned_free). This is because PCL uses Eigen's handmade_aligned_malloc but user code does not use the corresponding handmade_aligned_free (or the other way around), due to chosen SSE/AVX flags and the resulting alignment requirements. PCL's CMake config handles this by automatically setting SSE/AVX options in the user project as they are used in PCL, but not everyone uses CMake. This commit adds a check within PCL's header files for this kind of incompatibility.

At first I considered solving theses incompatibilities automatically by setting EIGEN_MALLOC_ALREADY_ALIGNED and EIGEN_MAX_ALIGN_BYTES, but that would have been more complex and error-prone, so I decided only for checking and warning.

I would have preferred `#warning` which however is not available on MSVC. `#pragma message` is an option on MSVC, but this is too easily overlooked IMO. Users have the option to silence this by defining the `PCL_SILENCE_MALLOC_WARNING` macro (in case of false positive).
@mvieth mvieth marked this pull request as ready for review November 30, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: cmake module: common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant