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

Change to minimum required c++ std, but don't force to lower. #5648

Closed

Conversation

larshg
Copy link
Contributor

@larshg larshg commented Apr 1, 2023

Implements suggestion from #5457

fixes #5457
fixes #5644
fixes #5647

@larshg larshg added module: cmake changelog: fix Meta-information for changelog generation labels Apr 1, 2023
@mvieth
Copy link
Member

mvieth commented Apr 5, 2023

I have thought about this and the situation is unfortunately quite complex:

  1. I don't know if we should merge this PR before the 1.13.1 release, since then for some compilers (e.g. GCC 12), PCL will be compiled as C++17 instead of C++14 and I am not sure if that would be an ABI break
  2. This PR does not fully solve the problem: Let's take GCC 12 as an example, the default is C++17 so PCL will be compiled with that standard. If someone compiles/forces their downstream project as C++14 (e.g. because older PCL version were compiled as C++14), they also get an error ("invalid read" in my tests). So this PR would "only" solve the problem for users who do not explicitly set the C++ standard for their downstream project and use a compiler for PCL and their downstream project that defaults to C++17 (e.g. GCC 11, GCC 12, Clang 16).
  3. I may have found a solution that hopefully removes the dependency between the C++ standards used for PCL and the downstream project: using hidden visibility for symbols by default for gcc (and clang as well, I think). MSVC does this automatically, as far as I know (which is why this problem should not happen with MSVC). However, this is something I would not merge until after PCL 1.13.1 (definitely an ABI break).
  4. A solution we could add for PCL 1.13.1 is a warning in pcl_config.h.in if the C++ standard (of the downstream project) is higher than C++14 (plus the option to silence the warning by defining a preprocessor symbol). On the other hand, this could often be a false alarm because in many cases, a C++14/C++17 mismatch does not seem to cause any trouble
  5. Another option would be to not do anything in this matter until PCL 1.13.1 is released, be extra aware of this problem for all new issues, and to not wait too long with releasing PCL 1.14.0 where we fix the problem for good 🙂
  6. For PCL 1.14.0, we could consider switching to C++17 anyway to have access to if constexpr, replace boost::filesystem, etc (if all relevant compilers support it)

@larshg
Copy link
Contributor Author

larshg commented Apr 6, 2023

Hmm.

According to this SO post it should be ABI compatiable as long as it is the same compiler version used.

I guess it might be a preprocessor value that changes, which eigen doesn't like, or have you pinpointed exactly when and where it happends?

I have reasoned, that it happend if a pointcloud is constructed within a PCL method (C++14), but released in user code (c++17)?

@mvieth
Copy link
Member

mvieth commented Apr 7, 2023

The problem is how Eigen creates aligned pointers, which changes between C++14 and C++17. We have to add the macro EIGEN_MAKE_ALIGNED_OPERATOR_NEW under certain conditions, for example for PointCloud it is here. If I understood correctly, in C++14 and lower, the special new-operator allocates memory (unaligned pointer) and adds some bytes until it has a properly aligned pointer. When freeing the object, it must not call free() on the aligned pointer, but has to subtract the same number of bytes to get the original, unaligned pointer, and call free() on that one. Calling free() on the wrong pointer would lead exactly to the double-free-or-corruption errors. In C++17 these extra steps to get aligned pointers are not necessary any more, and no bytes will be added/subtracted.
In the three cases we have observed so far, the problem was not a point cloud constructed within PCL and destructed in user code. Instead, both construction and destruction happened within PCL code. For example, in the MomentOfInertiaEstimation case, projected_cloud was created and destroyed in the same function The problem is that when the user code is compiled, a point cloud destructor (C++17) is created for a cloud in the user code, and during linking, this C++17 point cloud destructor is chosen to be called from within PCL code instead of the C++14 destructor from PCL. This is where my solution from point 3 from my last comment solves the problem: when using hidden visibility by default, the linker chooses the correct C++14 destructor.
Whether we can also have a problem when a point cloud is constructed within PCL and destructed in user code I am not sure, I will try to test this.

@larshg
Copy link
Contributor Author

larshg commented Apr 11, 2023

In the three cases we have observed so far, the problem was not a point cloud constructed within PCL and destructed in user code.

Yeah, I somehow thought wrong about it, it is indeed when constructed and destructed within PCL.

Lets just inform users if they don't find existing issues already. And then focus on a solution for 1.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Meta-information for changelog generation module: cmake
Projects
None yet
2 participants