Needs C++17 over-aligned new operator

[jlblancoc]

While building tests:

/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h: In instantiation of ‘gtsam::Expression<T>::Expression(const T&) [with T = gtsam::Unit3]’:
/home/jlblanco/code/gtsam-jlblancoc/gtsam/slam/tests/testEssentialMatrixFactor.cpp:149:39:   required from here
/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h:32:11: warning: ‘new’ of type ‘gtsam::internal::ConstantExpression<gtsam::Unit3>’ with extended alignment 32 [-Waligned-new=]
     root_(new internal::ConstantExpression<T>(value)) {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h:32:11: note: uses ‘void* operator new(std::size_t)’, which does not have an alignment parameter
/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h:32:11: note: use ‘-faligned-new’ to enable C++17 over-aligned new support

Need to investigate a bit more, but it's likely due to Eigen memory constraints.

Are you ok with adding -faligned-new as a private/public flag to gtsam libraries to solve this? Otherwise, I guess this may lead to random crashes if "optimizing for native" is enabled.

[dellaert]

Thanks for flagging! Two concerns I have:

• is -faligned_new cross-platform/compiler?
• hesitant to apply a blanket flag solution before fully understanding the problem.
  I never fully understood alignment, but willing to be educated :-)

[jlblancoc]

Hi!
It's a complex point... But basically it all has to do with special SIMD instructions used in optimized Eigen (e.g. SSE3, SSE4, AVX2) which require & assume that matrix data blocks start at addresses that are a multiple of N (with N=16, 32, ... bytes, depending on the SIMD family).

C++11 introduced alignas(N): See https://en.cppreference.com/w/cpp/language/alignas

That's great for variables in the stack (local variables), but what happens with this?

struct alignas(32) MyEigenVector { ... } ;

struct Foo
{
  MyEigenVector v;
};

auto f = std::make_shared<Foo>();

the last line calls the good old new(), which ignores the alignas constraints, leading to runtime crashes if (by chance) the returned address does not fulfill with the required alignment.

Now, that is finally fixed in C++17:

• http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0035r2.html

And it seems to have been well-received in Eigen: http://eigen.tuxfamily.org/bz/show_bug.cgi?id=1409

But... if the users (us) do not enable C++17, the new new(N,align) functions are not called.
That is what gcc is complaining about in the warning copied above. It seems to be a new warning, I never saw it before... but that's a good thing we are informed about the potential runtime crash!

I think there exist these solutions:

• Either use the gcc-specific flag (and things will work on gcc only; perhaps clang has the equivalent flag);
• Or switch to C++17 (and no special flag would be required).

Anyway, I think this issue has a low priority... unless unexpected random crashes start to show up while running optimized code (!).

[chrisbeall]

What OS, compiler, and C++ standard are you using when you get this warning? I don't think I've ever seen this. (I'm mostly on Ubuntu 16.04, C++14, gcc 5.4 or clang 8.0)

Eigen already provided a way to handle alignment for SSE3, AVX2, etc. which works in C++11 & C++14. Per the instructions at
https://eigen.tuxfamily.org/dox/group__TopicUnalignedArrayAssert.html we've added the EIGEN_MAKE_ALIGNED_OPERATOR_NEW macro in lots of our classes, avoid make_shared, etc.

I think we are probably just missing EIGEN_MAKE_ALIGNED_OPERATOR_NEW somewhere in Expressions.h?

[chrisbeall]

Actually, I see Expressions.h uses make_shared. We should fix that. make_shared with structs having Eigen members will be ok starting with C++17, exactly as you explained. I think it's probably too soon to make C++17 the default for gtsam for a few more years :-)

[jlblancoc]

I think it's probably too soon to make C++17 the default for gtsam for a few more years :-)

I guess it depends on how much you can bear people complaining about it :-)

avoid make_shared, etc.

Hmm... there's actually a workaround if you like it:

template <typename T, class... Args>
 std::shared_ptr<T> make_aligned_shared(Args&&... args)
 {
     using T_nc = typename std::remove_const<T>::type;
     return std::allocate_shared<T>(
          Eigen::aligned_allocator<T_nc>(), std::forward<Args>(args)...);
 }

[jlblancoc]

What OS, compiler, and C++ standard are you using when you get this warning?

It took me a while to reproduce it too... it only happens when building the tests, not the regular libraries.
On Ubuntu 18.04, gcc 7.4.0, etc. Everything to defaults in cmake. Here is the complete set of flags, for reference:

/usr/bin/c++  -DBOOST_OPTIONAL_ALLOW_BINDING_TO_RVALUES 
-DBOOST_OPTIONAL_CONFIG_ALLOW_BINDING_TO_RVALUES 
...
-std=c++11 -Wall -march=native -O3 -DNDEBUG    -O3 -DNDEBUG   -o ...

VERBOSE=1 make testEssentialMatrixFactor 

[100%] Building CXX object gtsam/slam/tests/CMakeFiles/testEssentialMatrixFactor.dir/testEssentialMatrixFactor.cpp.o
cd /home/jlblanco/code/gtsam-jlblancoc/build/gtsam/slam/tests && /usr/bin/c++  -DBOOST_OPTIONAL_ALLOW_BINDING_TO_RVALUES -DBOOST_OPTIONAL_CONFIG_ALLOW_BINDING_TO_RVALUES -DTOPSRCDIR=\"/home/jlblanco/code/gtsam-jlblancoc\" -I/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/include -I/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/libmetis -I/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/metis/GKlib -I/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/SuiteSparse_config -I/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/CCOLAMD/Include -I/home/jlblanco/code/gtsam-jlblancoc -I/home/jlblanco/code/gtsam-jlblancoc/build -I/home/jlblanco/code/gtsam-jlblancoc/CppUnitLite -I/home/jlblanco/code/gtsam-jlblancoc/gtsam/3rdparty/Eigen  -std=c++11 -Wall -march=native -O3 -DNDEBUG    -O3 -DNDEBUG   -o CMakeFiles/testEssentialMatrixFactor.dir/testEssentialMatrixFactor.cpp.o -c /home/jlblanco/code/gtsam-jlblancoc/gtsam/slam/tests/testEssentialMatrixFactor.cpp
In file included from /home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression.h:302:0,
                 from /home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/ExpressionFactor.h:22,
                 from /home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/expressionTesting.h:22,
                 from /home/jlblanco/code/gtsam-jlblancoc/gtsam/slam/tests/testEssentialMatrixFactor.cpp:11:
/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h: In instantiation of ‘gtsam::Expression<T>::Expression(const T&) [with T = gtsam::Unit3]’:
/home/jlblanco/code/gtsam-jlblancoc/gtsam/slam/tests/testEssentialMatrixFactor.cpp:149:39:   required from here
/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h:32:11: warning: ‘new’ of type ‘gtsam::internal::ConstantExpression<gtsam::Unit3>’ with extended alignment 32 [-Waligned-new=]
     root_(new internal::ConstantExpression<T>(value)) {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h:32:11: note: uses ‘void* operator new(std::size_t)’, which does not have an alignment parameter
/home/jlblanco/code/gtsam-jlblancoc/gtsam/nonlinear/Expression-inl.h:32:11: note: use ‘-faligned-new’ to enable C++17 over-aligned new support

[jlblancoc]

I don't know why, but this is not showing up anymore, so I guess it could be closed.

[jlblancoc]

Just found another instance of this same issue, so reopening again:

In file included from /home/jlblanco/code/gtsam/gtsam/slam/tests/testRotateFactor.cpp:8:
/home/jlblanco/code/gtsam/gtsam/slam/RotateFactor.h: In member function ‘virtual gtsam::NonlinearFactor::shared_ptr gtsam::RotateDirectionsFactor::clone() const’:
/home/jlblanco/code/gtsam/gtsam/slam/RotateFactor.h:93:58: warning: ‘new’ of type ‘gtsam::RotateDirectionsFactor::This’ {aka ‘gtsam::RotateDirectionsFactor’} with extended alignment 32 [-Waligned-new=]
   93 |         gtsam::NonlinearFactor::shared_ptr(new This(*this))); }
      |                                                          ^
/home/jlblanco/code/gtsam/gtsam/slam/RotateFactor.h:93:58: note: uses ‘void* operator new(std::size_t)’, which does not have an alignment parameter
/home/jlblanco/code/gtsam/gtsam/slam/RotateFactor.h:93:58: note: use ‘-faligned-new’ to enable C++17 over-aligned new support

This happens with g++9 under amd64 and u18.04.

[jlblancoc]

Closed by #185