From 752d3930ce367984af008bff1ed41eef546cacbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Sch=C3=B6nberger?= Date: Sat, 22 Jan 2022 12:33:58 +0100 Subject: [PATCH 1/5] Add Address Sanitizer options and fix reported issues --- CMakeLists.txt | 11 ++++++++++ doc/install.rst | 16 ++++++++++++++ lib/Graclus/multilevelLib/mlkkm.c | 6 +++--- lib/PBA/pba.cpp | 16 +++++++------- lib/PBA/pba.h | 2 +- src/base/cost_functions_test.cc | 30 ++++++++++++++------------- src/base/line.cc | 13 +++++++++--- src/base/reconstruction.cc | 2 +- src/estimators/similarity_transform.h | 4 ++-- 9 files changed, 68 insertions(+), 32 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7b94902a3d..e6bfd1bb88 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -61,6 +61,7 @@ option(CUDA_ENABLED "Whether to enable CUDA, if available" ON) option(GUI_ENABLED "Whether to enable the graphical UI" ON) option(OPENGL_ENABLED "Whether to enable OpenGL, if available" ON) option(TESTS_ENABLED "Whether to build test binaries" OFF) +option(ASAN_ENABLED "Whether to enable AddressSanitizer flags" OFF) option(PROFILING_ENABLED "Whether to enable google-perftools linker flags" OFF) option(CGAL_ENABLED "Whether to enable the CGAL library" ON) option(BOOST_STATIC "Whether to enable static boost library linker flags" ON) @@ -198,6 +199,16 @@ else() message(STATUS "Disabling interprocedural optimization") endif() +if(ASAN_ENABLED) + message(STATUS "Enabling ASan flags") + if(CMAKE_CXX_COMPILER_ID MATCHES Clang) + add_compile_options(-fsanitize=address -fno-omit-frame-pointer -fsanitize-address-use-after-scope) + add_link_options(-fsanitize=address) + else() + message(FATAL_ERROR "Unsupported compiler for ASan mode") + endif() +endif() + if(CUDA_FOUND) if(CUDA_ENABLED) add_definitions("-DCUDA_ENABLED") diff --git a/doc/install.rst b/doc/install.rst index f2d3d19e8e..259663929d 100755 --- a/doc/install.rst +++ b/doc/install.rst @@ -316,6 +316,22 @@ with the source code ``hello_world.cc``:: } +---------------- +AddressSanitizer +---------------- + +If you want to build COLMAP with address sanitizer flags enabled, you need to +use a recent compiler with ASan support. For example, you can manually install +a recent clang version on your Ubuntu machine and invoke CMake as follows:: + + CC=/usr/bin/clang CXX=/usr/bin/clang++ cmake .. \ + -DASAN_ENABLED=ON \ + -DTESTS_ENABLED=ON \ + -DCMAKE_BUILD_TYPE=RelWithDebInfo + +Note that it is generally useful to combine ASan with debug symbols to get +meaningful traces for reported issues. + ------------- Documentation ------------- diff --git a/lib/Graclus/multilevelLib/mlkkm.c b/lib/Graclus/multilevelLib/mlkkm.c index 9452f7ee7d..2f1d7165c8 100644 --- a/lib/Graclus/multilevelLib/mlkkm.c +++ b/lib/Graclus/multilevelLib/mlkkm.c @@ -182,12 +182,12 @@ int MLKKMPartitioning(CtrlType *ctrl, GraphType *graph, int nparts, int chain_le GraphType *cgraph; int wgtflag=3, numflag=0, options[10], edgecut; float ncut; - idxtype *cptr, *cind; + // idxtype *cptr, *cind; int numcomponents; char *mlwkkm_fname = "coarse.graph"; - cptr = idxmalloc(graph->nvtxs, "MLKKMPartitioning: cptr"); - cind = idxmalloc(graph->nvtxs, "MLKKMPartitioning: cind"); + // cptr = idxmalloc(graph->nvtxs, "MLKKMPartitioning: cptr"); + // cind = idxmalloc(graph->nvtxs, "MLKKMPartitioning: cind"); //printf("Computing the number of connected components.\n"); /*numcomponents = FindComponents(ctrl, graph, cptr, cind); diff --git a/lib/PBA/pba.cpp b/lib/PBA/pba.cpp index be9b10129d..77d62b0702 100755 --- a/lib/PBA/pba.cpp +++ b/lib/PBA/pba.cpp @@ -116,14 +116,14 @@ void ParallelBA::SetFocalMask(const int* fmask, float weight) { if (_optimizer && weight > 0) _optimizer->SetFocalMask(fmask, weight); } -void* ParallelBA::operator new(size_t size) { - void* p = malloc(size); - if (p == 0) { - const std::bad_alloc ba; - throw ba; - } - return p; -} +// void* ParallelBA::operator new(size_t size) { +// void* p = malloc(size); +// if (p == 0) { +// const std::bad_alloc ba; +// throw ba; +// } +// return p; +// } ParallelBA* NewParallelBA(ParallelBA::DeviceT device) { return new ParallelBA(device); diff --git a/lib/PBA/pba.h b/lib/PBA/pba.h index fe6542f33c..3ebf5813f9 100755 --- a/lib/PBA/pba.h +++ b/lib/PBA/pba.h @@ -132,7 +132,7 @@ class ParallelBA { public: PBA_EXPORT ParallelBA(DeviceT device = PBA_CUDA_DEVICE_DEFAULT, const int num_threads = -1); - PBA_EXPORT void* operator new(size_t size); + // PBA_EXPORT void* operator new(size_t size); PBA_EXPORT virtual ~ParallelBA(); public: diff --git a/src/base/cost_functions_test.cc b/src/base/cost_functions_test.cc index 7ad01f06e7..d19e6d3598 100755 --- a/src/base/cost_functions_test.cc +++ b/src/base/cost_functions_test.cc @@ -39,9 +39,9 @@ using namespace colmap; BOOST_AUTO_TEST_CASE(TestBundleAdjustmentCostFunction) { - ceres::CostFunction* cost_function = + std::unique_ptr cost_function( BundleAdjustmentCostFunction::Create( - Eigen::Vector2d::Zero()); + Eigen::Vector2d::Zero())); double qvec[4] = {1, 0, 0, 0}; double tvec[3] = {0, 0, 0}; double point3D[3] = {0, 0, 1}; @@ -69,10 +69,11 @@ BOOST_AUTO_TEST_CASE(TestBundleAdjustmentCostFunction) { } BOOST_AUTO_TEST_CASE(TestBundleAdjustmentConstantPoseCostFunction) { - ceres::CostFunction* cost_function = BundleAdjustmentConstantPoseCostFunction< - SimplePinholeCameraModel>::Create(ComposeIdentityQuaternion(), - Eigen::Vector3d::Zero(), - Eigen::Vector2d::Zero()); + std::unique_ptr cost_function( + BundleAdjustmentConstantPoseCostFunction< + SimplePinholeCameraModel>::Create(ComposeIdentityQuaternion(), + Eigen::Vector3d::Zero(), + Eigen::Vector2d::Zero())); double point3D[3] = {0, 0, 1}; double camera_params[3] = {1, 0, 0}; double residuals[2]; @@ -98,9 +99,9 @@ BOOST_AUTO_TEST_CASE(TestBundleAdjustmentConstantPoseCostFunction) { } BOOST_AUTO_TEST_CASE(TestRigBundleAdjustmentCostFunction) { - ceres::CostFunction* cost_function = + std::unique_ptr cost_function( RigBundleAdjustmentCostFunction::Create( - Eigen::Vector2d::Zero()); + Eigen::Vector2d::Zero())); double rig_qvec[4] = {1, 0, 0, 0}; double rig_tvec[3] = {0, 0, -1}; double rel_qvec[4] = {1, 0, 0, 0}; @@ -131,8 +132,9 @@ BOOST_AUTO_TEST_CASE(TestRigBundleAdjustmentCostFunction) { } BOOST_AUTO_TEST_CASE(TestRelativePoseCostFunction) { - ceres::CostFunction* cost_function = RelativePoseCostFunction::Create( - Eigen::Vector2d(0, 0), Eigen::Vector2d(0, 0)); + std::unique_ptr cost_function( + RelativePoseCostFunction::Create(Eigen::Vector2d(0, 0), + Eigen::Vector2d(0, 0))); double qvec[4] = {1, 0, 0, 0}; double tvec[3] = {0, 1, 0}; double residuals[1]; @@ -140,13 +142,13 @@ BOOST_AUTO_TEST_CASE(TestRelativePoseCostFunction) { BOOST_CHECK(cost_function->Evaluate(parameters, residuals, nullptr)); BOOST_CHECK_EQUAL(residuals[0], 0); - cost_function = RelativePoseCostFunction::Create(Eigen::Vector2d(0, 0), - Eigen::Vector2d(1, 0)); + cost_function.reset(RelativePoseCostFunction::Create(Eigen::Vector2d(0, 0), + Eigen::Vector2d(1, 0))); BOOST_CHECK(cost_function->Evaluate(parameters, residuals, nullptr)); BOOST_CHECK_EQUAL(residuals[0], 0.5); - cost_function = RelativePoseCostFunction::Create(Eigen::Vector2d(0, 0), - Eigen::Vector2d(1, 1)); + cost_function.reset(RelativePoseCostFunction::Create(Eigen::Vector2d(0, 0), + Eigen::Vector2d(1, 1))); BOOST_CHECK(cost_function->Evaluate(parameters, residuals, nullptr)); BOOST_CHECK_EQUAL(residuals[0], 0.5); } diff --git a/src/base/line.cc b/src/base/line.cc index 1aeb522ebe..accbc4fc6c 100644 --- a/src/base/line.cc +++ b/src/base/line.cc @@ -38,6 +38,13 @@ extern "C" { } namespace colmap { +namespace { + +struct RawDeleter { + void operator()(double* p) { free(p); } +}; + +} // namespace std::vector DetectLineSegments(const Bitmap& bitmap, const double min_length) { @@ -55,9 +62,9 @@ std::vector DetectLineSegments(const Bitmap& bitmap, bitmap_data.end()); int num_segments; - std::unique_ptr segments_data(lsd(&num_segments, - bitmap_data_double.data(), - bitmap.Width(), bitmap.Height())); + std::unique_ptr segments_data( + lsd(&num_segments, bitmap_data_double.data(), bitmap.Width(), + bitmap.Height())); std::vector segments; segments.reserve(num_segments); diff --git a/src/base/reconstruction.cc b/src/base/reconstruction.cc index 60ce45d0f3..37baa9f180 100755 --- a/src/base/reconstruction.cc +++ b/src/base/reconstruction.cc @@ -1487,8 +1487,8 @@ size_t Reconstruction::FilterPoints3DWithLargeReprojectionError( class Point3D& point3D = Point3D(point3D_id); if (point3D.Track().Length() < 2) { - DeletePoint3D(point3D_id); num_filtered += point3D.Track().Length(); + DeletePoint3D(point3D_id); continue; } diff --git a/src/estimators/similarity_transform.h b/src/estimators/similarity_transform.h index 69ebfe6909..c940d42541 100755 --- a/src/estimators/similarity_transform.h +++ b/src/estimators/similarity_transform.h @@ -109,8 +109,8 @@ SimilarityTransformEstimator::Estimate( dst_mat.col(i) = dst[i]; } - const auto model = Eigen::umeyama(src_mat, dst_mat, kEstimateScale) - .topLeftCorner(kDim, kDim + 1); + const M_t model = Eigen::umeyama(src_mat, dst_mat, kEstimateScale) + .topLeftCorner(kDim, kDim + 1); if (model.array().isNaN().any()) { return std::vector{}; From 22a30e0fee07f7ceeb63f9fa71998e9164742f92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Sch=C3=B6nberger?= Date: Sat, 22 Jan 2022 12:40:18 +0100 Subject: [PATCH 2/5] Add CI build --- .azure-pipelines/build-ubuntu.yaml | 6 ++++-- .azure-pipelines/build.yaml | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.azure-pipelines/build-ubuntu.yaml b/.azure-pipelines/build-ubuntu.yaml index 8985c847e1..7e44fb8866 100644 --- a/.azure-pipelines/build-ubuntu.yaml +++ b/.azure-pipelines/build-ubuntu.yaml @@ -2,9 +2,10 @@ parameters: displayName: 'Ubuntu 18.04' ubuntuVersion: '18.04' cudaEnabled: false + asanEnabled: false jobs: -- job: ubuntu_build_${{ replace(parameters.ubuntuVersion, '.', '') }}_cuda_${{ parameters.cudaEnabled }} +- job: ubuntu_build_${{ replace(parameters.ubuntuVersion, '.', '') }}_cuda_${{ parameters.cudaEnabled }}_asa_${{ parameters.asanEnabled }} displayName: '${{ parameters.displayName }}' pool: vmImage: 'ubuntu-${{ parameters.ubuntuVersion }}' @@ -48,7 +49,8 @@ jobs: cmake .. \ -GNinja \ -DTESTS_ENABLED=ON \ - -DCUDA_ARCHS=6.0 + -DCUDA_ARCHS=6.0 \ + -DASAN_ENABLED=${{ parameters.asanEnabled }} ninja displayName: 'Configure and build' diff --git a/.azure-pipelines/build.yaml b/.azure-pipelines/build.yaml index 41eeec9370..4bae406c12 100644 --- a/.azure-pipelines/build.yaml +++ b/.azure-pipelines/build.yaml @@ -16,6 +16,11 @@ jobs: displayName: 'Ubuntu 20.04 (CUDA)' ubuntuVersion: 20.04 cudaEnabled: true + - template: build-ubuntu.yaml + parameters: + displayName: 'Ubuntu 20.04 (ASan)' + ubuntuVersion: 20.04 + asanEnabled: true - template: build-mac.yaml parameters: displayName: 'Mac 10.15' From 24d523fe483d18ae539b23692ea3f2f066a97874 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Sch=C3=B6nberger?= Date: Sat, 22 Jan 2022 12:43:13 +0100 Subject: [PATCH 3/5] Fix --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e6bfd1bb88..5b9077798b 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -201,7 +201,7 @@ endif() if(ASAN_ENABLED) message(STATUS "Enabling ASan flags") - if(CMAKE_CXX_COMPILER_ID MATCHES Clang) + if(IS_CLANG OR IS_GNU) add_compile_options(-fsanitize=address -fno-omit-frame-pointer -fsanitize-address-use-after-scope) add_link_options(-fsanitize=address) else() From efb3a48a61fde8ae7cd7b23b170caa00d03a3baa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Sch=C3=B6nberger?= Date: Sat, 22 Jan 2022 13:41:05 +0100 Subject: [PATCH 4/5] Asan with clang --- .azure-pipelines/build-ubuntu.yaml | 7 +++++++ src/ui/colormaps.cc | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.azure-pipelines/build-ubuntu.yaml b/.azure-pipelines/build-ubuntu.yaml index 7e44fb8866..200a149356 100644 --- a/.azure-pipelines/build-ubuntu.yaml +++ b/.azure-pipelines/build-ubuntu.yaml @@ -42,6 +42,13 @@ jobs: echo '##vso[task.setvariable variable=CXX]/usr/bin/cuda-g++' displayName: 'Install CUDA' + - ${{ if eq(parameters.asanEnabled, true) }}: + - script: | + sudo apt-get install -y clang-10 + echo '##vso[task.setvariable variable=CC]/usr/bin/clang' + echo '##vso[task.setvariable variable=CXX]/usr/bin/clang++' + displayName: 'Install CUDA' + - script: | cmake --version mkdir build diff --git a/src/ui/colormaps.cc b/src/ui/colormaps.cc index 869f5e8256..ea793f0837 100755 --- a/src/ui/colormaps.cc +++ b/src/ui/colormaps.cc @@ -150,7 +150,7 @@ void PointColormapGroundResolution::Prepare( const Eigen::Vector3f xyz = point3D.second.XYZ().cast(); - for (const auto track_el : point3D.second.Track().Elements()) { + for (const auto& track_el : point3D.second.Track().Elements()) { const auto& image = images[track_el.image_id]; const float focal_length = focal_lengths[image.CameraId()]; const float focal_length2 = focal_length * focal_length; From 80949d0a35b0d28d822f52026e6569a0485bde1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Sch=C3=B6nberger?= Date: Sat, 22 Jan 2022 14:07:04 +0100 Subject: [PATCH 5/5] Small cosmetic fixes to recent ASan support --- .azure-pipelines/build-ubuntu.yaml | 4 ++-- CMakeLists.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.azure-pipelines/build-ubuntu.yaml b/.azure-pipelines/build-ubuntu.yaml index 200a149356..5df22af9a2 100644 --- a/.azure-pipelines/build-ubuntu.yaml +++ b/.azure-pipelines/build-ubuntu.yaml @@ -5,7 +5,7 @@ parameters: asanEnabled: false jobs: -- job: ubuntu_build_${{ replace(parameters.ubuntuVersion, '.', '') }}_cuda_${{ parameters.cudaEnabled }}_asa_${{ parameters.asanEnabled }} +- job: ubuntu_build_${{ replace(parameters.ubuntuVersion, '.', '') }}_cuda_${{ parameters.cudaEnabled }}_asan_${{ parameters.asanEnabled }} displayName: '${{ parameters.displayName }}' pool: vmImage: 'ubuntu-${{ parameters.ubuntuVersion }}' @@ -47,7 +47,7 @@ jobs: sudo apt-get install -y clang-10 echo '##vso[task.setvariable variable=CC]/usr/bin/clang' echo '##vso[task.setvariable variable=CXX]/usr/bin/clang++' - displayName: 'Install CUDA' + displayName: 'Install Clang' - script: | cmake --version diff --git a/CMakeLists.txt b/CMakeLists.txt index 5b9077798b..3e04d90bb7 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -200,7 +200,7 @@ else() endif() if(ASAN_ENABLED) - message(STATUS "Enabling ASan flags") + message(STATUS "Enabling ASan support") if(IS_CLANG OR IS_GNU) add_compile_options(-fsanitize=address -fno-omit-frame-pointer -fsanitize-address-use-after-scope) add_link_options(-fsanitize=address)