Drake: Promote msvc warning C4316 to an error

Created on 2 Jun 2016  路  8Comments  路  Source: RobotLocomotion/drake

The msvc warning C4316 is _"'MyClass': object allocated on the heap may not be aligned 16"_.

PR #2171 has been going through a pass-merge-fail-revert-pass-merge-fail dance.

In part, it's because Eigen spills its alignment restrictions all over the place (#1854), so it's extraordinarily difficult to write correct code. I really can't express strongly enough how awful the alignment-bleeds-everywhere problem is. The fundamental primitives of computation (Vector2d, Isometry3d, Quaterniond, etc.) explode in your face if you look at them funny.

But in any case, we could still do better about paying attention that an explosion is imminent, e.g., during the pre-merge jenkins builds -- or even better, workstation builds.

In particular, on our win32 builds, any instance of warning C4316 within Drake itself is probably a true positive problem every single time, and should be promoted to an error. It appears that the externals (bullet, at least) hit this warning as well, so for now I suppose we should only enable it within drake-distro/drake and not the externals.

high bug

Most helpful comment

I'm bumping up the priority on this, because it's allowing pre-merge Jenkins to pass, but then failing on master later on.

/CC @liangfok

All 8 comments

/cc @sherm1 @david-german-tri @liangfok

Great idea to treat this as an error. The required compiler flag is /we4316.

I'm bumping up the priority on this, because it's allowing pre-merge Jenkins to pass, but then failing on master later on.

/CC @liangfok

Note that the pre-merge Win32 build for #2596 also contained the following warning:

10:18:51 C:\projects\workspace\experimental\a99ae8ad\build\include\gtest/internal/gtest-internal.h(484): warning C4316: 'drake::systems::plants::test::rigid_body_tree::`anonymous-namespace'::RBTCollisionTest_FindAndComputeContactPoints_Test': object allocated on the heap may not be aligned 16

It was subsequently fixed in #2683 .

/CC @amcastro-tri

Add set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /we4316") to the obvious spot in drake/CMakeLists.txt. I'd volunteer, but the others all have pretty comments explaining why they're there. Maybe if someone wants to suggest text for such?

How about: "Elevates C4316 to be a compile-time error so that 16-bit Eigen alignment issues can be detected prior to merge into master."

I would say "This is a warning that is emitted when Eigen alignment is disobeyed; it is almost always a true positive problem, so we promote it to error severity by default."

The CMake and comment are the easy parts. The only hard part is getting this passing in a win32 build environment, which was more than I wanted to take on myself.

Windows is no longer supported.

Was this page helpful?
0 / 5 - 0 ratings