Merge pull request artivis#115 from artivis/fix/mem_alignment

- Fix memory alignment issues - expand CI with cppcheck & valgrind
varyshare · Feb 20, 2020 · 7a9564b · 7a9564b
2 parents ac06ddb + 2295b49
commit 7a9564b
Show file tree

Hide file tree

Showing 14 changed files with 136 additions and 69 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -92,24 +92,24 @@ matrix:
           exit -1
         fi
 
-  # #
-  # # CppCheck
-  # #
-  # - os: linux
-  #   env:
-  #     - TEST="CppCheck"
-  #   addons:
-  #     apt:
-  #       sources:
-  #         - ubuntu-toolchain-r-test
-  #       packages:
-  #         - gcc-6
-  #         - g++-6
-  #         - cppcheck
-  #   script:
-  #     - cmake -DENABLE_CPPCHECK=ON -DCMAKE_CXX_COMPILER="g++-6" -DBUILD_EXAMPLES=ON -DBUILD_TESTING=ON ..
-  #     - make
-  #     - make check
+  #
+  # CppCheck
+  #
+  - os: linux
+    env:
+      - TEST="CppCheck"
+    addons:
+      apt:
+        sources:
+          - ubuntu-toolchain-r-test
+        packages:
+          - gcc-6
+          - g++-6
+          - cppcheck # install newer version?
+    script:
+      - cmake -DENABLE_CPPCHECK=ON -DCMAKE_CXX_COMPILER="g++-6" -DBUILD_EXAMPLES=ON -DBUILD_TESTING=ON ..
+      - make
+      - make run-cppcheck
 
   #
   # Codecov
@@ -131,24 +131,24 @@ matrix:
       - cd ..
       - bash <(curl -s https://codecov.io/bash)
 
-  # #
-  # # Valgrind
-  # #
-  # - os: linux
-  #   env:
-  #     - TEST="Valgrind"
-  #   addons:
-  #     apt:
-  #       sources:
-  #         - ubuntu-toolchain-r-test
-  #       packages:
-  #         - gcc-6
-  #         - g++-6
-  #         - valgrind
-  #   script:
-  #     - cmake -DCMAKE_CXX_COMPILER="g++-6" ..
-  #     - make
-  #     - ctest -T memcheck
+  #
+  # Valgrind
+  #
+  - os: linux
+    env:
+      - TEST="Valgrind"
+    addons:
+      apt:
+        sources:
+          - ubuntu-toolchain-r-test
+        packages:
+          - gcc-6
+          - g++-6
+          - valgrind
+    script:
+      - cmake -DCMAKE_CXX_COMPILER="g++-6" -DENABLE_VALGRIND=ON -DBUILD_TESTING=ON ..
+      - make
+      - ctest -T memcheck
 
   #
   # G++ 5
@@ -186,6 +186,24 @@ matrix:
       - make
       - make test
 
+  #
+  # G++ 7
+  #
+  - os: linux
+    env:
+      - TEST="G++ 7"
+    addons:
+      apt:
+        sources:
+          - ubuntu-toolchain-r-test
+        packages:
+          - gcc-7
+          - g++-7
+    script:
+      - cmake -DCMAKE_CXX_COMPILER="g++-7" -DBUILD_EXAMPLES=ON -DBUILD_TESTING=ON ..
+      - make
+      - make test
+
   #
   # Clang 3.8
   #

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -190,28 +190,58 @@ endif()
 # CppCheck
 # ------------------------------------------------------------------------------
 
-# if(ENABLE_CPPCHECK)
-#
-#   list(APPEND CPPCHECK_ARGS
-#     --enable=warning,style,performance,portability,unusedFunction
-#     --std=c++11
-#     --verbose
-#     --error-exitcode=1
-#     --language=c++
-#     -DMAIN=main
-#     -I ${CMAKE_SOURCE_DIR}/include
-#       ${CMAKE_SOURCE_DIR}/include/*.h
-#       #${CMAKE_SOURCE_DIR}/examples/*.cpp
-#       ${CMAKE_SOURCE_DIR}/test/*.cpp
-#   )
-#
-#
-#   add_custom_target(check
-#     COMMAND cppcheck ${CPPCHECK_ARGS}
-#     COMMENT "running cppcheck"
-#   )
-#
-# endif()
+if(ENABLE_CPPCHECK)
+
+  # Find CppCheck executable
+  find_program(CPPCHECK "cppcheck")
+  if(CPPCHECK)
+
+      # Set export commands on
+      # set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+
+      list(APPEND CPPCHECK_ARGS
+        --enable=all
+        --std=c++11
+        # --verbose
+        --quiet
+        # --check-config
+        --xml-version=2
+        --language=c++
+        # Comment the line below to run cppcheck-html
+        --error-exitcode=1
+        --inline-suppr
+        --suppress=*:*googletest-*
+        --suppress=missingIncludeSystem
+        --suppress=missingInclude
+        --suppress=unmatchedSuppression:*
+        --suppress=syntaxError:*tangent_base.h:370
+        # Uncomment the line below to run cppcheck-html
+        # --output-file=${CMAKE_BINARY_DIR}/cppcheck_results.xml
+        ${CMAKE_SOURCE_DIR}/include
+        ${CMAKE_SOURCE_DIR}/examples
+        ${CMAKE_SOURCE_DIR}/test
+      )
+
+      add_custom_target(run-cppcheck
+          COMMAND ${CPPCHECK} ${CPPCHECK_ARGS}
+          COMMENT "Generate cppcheck report for the project"
+      )
+
+      find_program(CPPCHECK_HTML "cppcheck-htmlreport")
+      if(CPPCHECK_HTML)
+          add_custom_target(cppcheck-html
+              COMMAND ${CPPCHECK_HTML}
+              --title=${CMAKE_PROJECT_NAME}
+              --file=${CMAKE_BINARY_DIR}/cppcheck_results.xml
+              --report-dir=${CMAKE_BINARY_DIR}/cppcheck_results
+              --source-dir=${CMAKE_SOURCE_DIR}
+              COMMENT "Convert cppcheck report to HTML output"
+          )
+          ADD_DEPENDENCIES(cppcheck-html run-cppcheck)
+      endif()
+  endif()
+
+endif()
 
 # ------------------------------------------------------------------------------
 # Valgrind
@@ -223,6 +253,7 @@ if(ENABLE_VALGRIND)
   set(MEMORYCHECK_COMMAND_OPTIONS "${MEMORYCHECK_COMMAND_OPTIONS} --leak-check=full")
   set(MEMORYCHECK_COMMAND_OPTIONS "${MEMORYCHECK_COMMAND_OPTIONS} --track-fds=yes")
   set(MEMORYCHECK_COMMAND_OPTIONS "${MEMORYCHECK_COMMAND_OPTIONS} --trace-children=yes")
+  set(MEMORYCHECK_COMMAND_OPTIONS "${MEMORYCHECK_COMMAND_OPTIONS} --show-reachable=yes")
   set(MEMORYCHECK_COMMAND_OPTIONS "${MEMORYCHECK_COMMAND_OPTIONS} --error-exitcode=1")
 endif()
 

diff --git a/include/manif/ceres/constraint.h b/include/manif/ceres/constraint.h
@@ -21,6 +21,8 @@ class CeresConstraintFunctor
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND_TYPE(Tangent)
+
   using Covariance = Eigen::Matrix<double, LieGroup::DoF, LieGroup::DoF>;
   using InformationMatrix = Covariance;
 

diff --git a/include/manif/ceres/objective.h b/include/manif/ceres/objective.h
@@ -16,6 +16,8 @@ class CeresObjectiveFunctor
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND_TYPE(LieGroup)
+
   template <typename... Args>
   CeresObjectiveFunctor(Args&&... args)
     : target_state_(std::forward<Args>(args)...)

diff --git a/include/manif/impl/se2/SE2.h b/include/manif/impl/se2/SE2.h
@@ -58,6 +58,8 @@ struct SE2 : SE2Base<SE2<_Scalar>>
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND
+
   MANIF_COMPLETE_GROUP_TYPEDEF
   using Translation = typename Base::Translation;
   MANIF_INHERIT_GROUP_API

diff --git a/include/manif/impl/se2/SE2Tangent.h b/include/manif/impl/se2/SE2Tangent.h
@@ -51,6 +51,8 @@ struct SE2Tangent : SE2TangentBase<SE2Tangent<_Scalar>>
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND
+
   MANIF_TANGENT_TYPEDEF
   MANIF_INHERIT_TANGENT_API
   MANIF_INHERIT_TANGENT_OPERATOR

diff --git a/include/manif/impl/se3/SE3.h b/include/manif/impl/se3/SE3.h
@@ -61,8 +61,7 @@ struct SE3 : SE3Base<SE3<_Scalar>>
 
 public:
 
-  // Needed this underlying vector is size 7
-  EIGEN_MAKE_ALIGNED_OPERATOR_NEW
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND
 
   MANIF_COMPLETE_GROUP_TYPEDEF
   using Translation = typename Base::Translation;

diff --git a/include/manif/impl/se3/SE3Tangent.h b/include/manif/impl/se3/SE3Tangent.h
@@ -51,6 +51,8 @@ struct SE3Tangent : SE3TangentBase<SE3Tangent<_Scalar>>
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND
+
   MANIF_TANGENT_TYPEDEF
   MANIF_INHERIT_TANGENT_API
   MANIF_INHERIT_TANGENT_OPERATOR

diff --git a/include/manif/impl/so3/SO3.h b/include/manif/impl/so3/SO3.h
@@ -59,6 +59,8 @@ struct SO3 : SO3Base<SO3<_Scalar>>
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND
+
   MANIF_COMPLETE_GROUP_TYPEDEF
   MANIF_INHERIT_GROUP_API
   using Base::transform;

diff --git a/include/manif/impl/so3/SO3Tangent.h b/include/manif/impl/so3/SO3Tangent.h
@@ -51,6 +51,8 @@ struct SO3Tangent : SO3TangentBase<SO3Tangent<_Scalar>>
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND
+
   MANIF_TANGENT_TYPEDEF
   MANIF_INHERIT_TANGENT_API
   MANIF_INHERIT_TANGENT_OPERATOR

diff --git a/test/ceres/ceres_test_utils.h b/test/ceres/ceres_test_utils.h
@@ -7,7 +7,7 @@
 #define MANIF_TEST_JACOBIANS_CERES(manifold)                                                    \
   using TEST_##manifold##_JACOBIANS_CERES_TESTER = JacobianCeresTester<manifold>;               \
   TEST_F(TEST_##manifold##_JACOBIANS_CERES_TESTER, TEST_##manifold##_CERES_OBJECTIVE_JACOBIANS) \
-  { /*evalObjectiveJacs();*/ }                                                                      \
+  { /*evalObjectiveJacs();*/ }                                                                  \
   TEST_F(TEST_##manifold##_JACOBIANS_CERES_TESTER, TEST_##manifold##_CERES_JACOBIANS)           \
   { evalJacs(); }
 
@@ -38,6 +38,8 @@ class JacobianCeresTester : public ::testing::Test
 
 public:
 
+  MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND_TYPE(LieGroup)
+
   JacobianCeresTester()  = default;
   ~JacobianCeresTester() = default;
 
@@ -144,6 +146,7 @@ class JacobianCeresTester : public ::testing::Test
 
     ObjectiveJacobian autodiffJ_y_r;
     jacobian = autodiffJ_y_r.data();
+    (void)jacobian;
 
     autodiff_obj->Evaluate(parameters, &residuals, jacobians);
 

diff --git a/test/common_tester.h b/test/common_tester.h
@@ -370,6 +370,7 @@ class CommonTester : public ::testing::Test
     EXPECT_TRUE(state.isApprox(state, tol_));
     EXPECT_FALSE(state.isApprox(state_other, tol_));
 
+    // cppcheck-suppress duplicateExpression
     EXPECT_TRUE(state == state);
     EXPECT_FALSE(state == state_other);
 
@@ -384,6 +385,7 @@ class CommonTester : public ::testing::Test
     EXPECT_TRUE(delta.isApprox(delta, tol_));
     EXPECT_FALSE(delta.isApprox(delta+delta, tol_));
 
+    // cppcheck-suppress duplicateExpression
     EXPECT_TRUE(delta == delta);
     EXPECT_FALSE(delta == (delta+delta));
   }

diff --git a/test/eigen_gtest.h b/test/eigen_gtest.h
@@ -85,11 +85,11 @@ isEigenMatrixDimSize(const detail::EigenIndex dim,
   const auto sizes = DimGetter()(ms...);
 
   bool result = true;
-  auto f = [&result, &dim](const detail::EigenIndex i)
-                          { result &= (dim == i);};
+  auto f = [&result, &dim](const detail::EigenIndex i){ result &= (dim == i);};
 
   detail::call_for_each(f, sizes);
 
+  // cppcheck-suppress knownConditionTrueFalse
   if (!result)
   {
     std::stringstream ss;
@@ -196,8 +196,8 @@ isEigenMatrixSameSize(const Eigen::MatrixBase<Derived>& m0,
 template <class _DerivedA, class _DerivedB>
 inline ::testing::AssertionResult isEigenMatrixNear(const Eigen::MatrixBase<_DerivedA>& matrix_a,
                                                     const Eigen::MatrixBase<_DerivedB>& matrix_b,
-                                                    const std::string matrix_a_name = "matrix_a",
-                                                    const std::string matrix_b_name = "matrix_b",
+                                                    const std::string& matrix_a_name = "matrix_a",
+                                                    const std::string& matrix_b_name = "matrix_b",
                                                     double tolerance = 1e-8)
 {
   const ::testing::AssertionResult size_check =

diff --git a/test/test_utils.h b/test/test_utils.h
@@ -59,8 +59,8 @@ template <class _DerivedA, class _DerivedB>
 inline ::testing::AssertionResult
 isManifNear(const LieGroupBase<_DerivedA>& manifold_a,
             const LieGroupBase<_DerivedB>& manifold_b,
-            const std::string manifold_a_name = "manifold_a",
-            const std::string manifold_b_name = "manifold_b",
+            const std::string& manifold_a_name = "manifold_a",
+            const std::string& manifold_b_name = "manifold_b",
             double tolerance = 1e-5)
 {
   auto result =
@@ -80,8 +80,8 @@ template <class _DerivedA, class _DerivedB>
 inline ::testing::AssertionResult
 isManifNear(const TangentBase<_DerivedA>& tangent_a,
             const TangentBase<_DerivedB>& tangent_b,
-            const std::string tangent_a_name = "tangent_a",
-            const std::string tangent_b_name = "tangent_b",
+            const std::string& tangent_a_name = "tangent_a",
+            const std::string& tangent_b_name = "tangent_b",
             double tolerance = 1e-5)
 {
   return isEigenMatrixNear(tangent_a.coeffs(), tangent_b.coeffs(),
-Original file line number
+Diff line change
@@ Expand Up / @@ -21,6 +21,8 @@ class CeresConstraintFunctor @@
     public:
+      MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND_TYPE(Tangent)
       using Covariance = Eigen::Matrix<double, LieGroup::DoF, LieGroup::DoF>;
       using InformationMatrix = Covariance;
@@ Expand Down @@