g2o uses Eigen as if it is compiled with C++14 even if a newer standard is available

[TimPushkin]

This line in g2o's main CMake file makes g2o use Eigen's macros as if they are compiled with only C++14 supported, even if C++17 or newer is available. If think it works like this because the macros are expanded the g2o's files which have C++ standard manually set to 14 by the line mentioned above.

The exact problem I've stumbled upon is that g2o uses EIGEN_MAKE_ALIGNED_OPERATOR_NEW macro which generates destructors that Address Sanitizer reports as causing buffer overflow. The thing is, my project uses C++17 which is configured as a target property of my library, and with C++17 available EIGEN_MAKE_ALIGNED_OPERATOR_NEW should do nothing. However, because of the CMake line mentioned above, in g2o this macro generates faulty code (I'm not sure if it is truly faulty or if this is just a false positive, but I would like to make Eigen use features of the newer compiler anyway).

I see the following approach to solve this.

# Set CMAKE_* cache vars only when top-level not to pollute the outer project, if any
if(PROJECT_IS_TOP_LEVEL)
  set(CMAKE_CXX_STANDARD 14 CACHE STRING "Default C++ standard")
elif(NOT DEFINED CMAKE_CXX_STANDARD)
  set(CMAKE_CXX_STANDARD 14)
endif()

if(CMAKE_CXX_STANDARD LESS 14)
  message(FATAL_ERROR "C++ standard is too old, g2o requires C++14")
endif()

For me this is not ideal, as I don't have CMAKE_CXX_STANDARD defined, but I can still set it just in g2o's scope.

I can create a PR for this, but I'm not sure if there is a better way of setting a minimum standard boundary.

[TimPushkin]

I've just noticed that g2o's core target actually has a C++14 requirement here. This should be enough by itself, so I think the lines that set CMAKE_CXX_STANDARD and CMAKE_CXX_STANDARD_REQUIRED can just be removed. Doing so solves my issue.

[RainerKuemmerle]

I was planing on moving to c++17 on master branch. But I am a little confused now about EIGEN_MAKE_ALIGNED_OPERATOR_NEW.

Looking when EIGEN_HAS_CXX17_OVERALIGN would be 1. It's somewhat a bit more narrow than just c++17? https://gitlab.com/libeigen/eigen/-/blob/master/Eigen/src/Core/util/Macros.h#L752

The doc on https://eigen.tuxfamily.org/dox/group__TopicStructHavingEigenMembers.html seems to simplify this?

Also, when a file is compiled with c++14, EIGEN_MAKE_ALIGNED_OPERATOR_NEW would expand to actual code but when you include with c++17 it would be empty?

[TimPushkin]

EIGEN_MAKE_ALIGNED_OPERATOR_NEW should expand to nothing when the project is compiled in C++17 mode and the compiler supports "Dynamic memory allocation for over-aligned data" feature of C++17 and to the actual code otherwise. On my system EIGEN_MAKE_ALIGNED_OPERATOR_NEW expands to the mentioned faulty code when compiled for C++14 and to nothing when compiled for C++17.

With EIGEN_HAS_CXX17_OVERALIGN Eigen seems to check just that. To be 100% safe when moving to C++17 it is best to either leave EIGEN_MAKE_ALIGNED_OPERATOR_NEW or place static_assert(EIGEN_HAS_CXX17_OVERALIGN) somewhere (config.h?). But CMake's developers seem to think that it is save to assume that if a compiler supports C++17, it supports all the required features, so they don't provide ways to request individual features like they do for C++ 11 and 14.

[RainerKuemmerle]

Thanks for the further explanation. This means the branch switching to c++17 would address the issue and we leave the EIGEN_MAKE_ALIGNED_OPERATOR_NEW in place for now.

[RainerKuemmerle]

I merged to master the change to use c++17 and require Eigen 3.4 as well. By this I close this issue, let me know if further changes would be needed. Thanks.