From 06c0d798af68736a62d1930b6fd661398bb1154e Mon Sep 17 00:00:00 2001 From: Kevin Boyd Date: Wed, 29 Jul 2020 11:42:59 +0000 Subject: [PATCH] Fix undefined behavior flagged by UBSAN Running under the UBSAN sanitizer pointed out a bunch of undefined behavior - most of them were harmless issues in rarely taken branches, but should still be avoided. --- CMakeLists.txt | 5 ++- admin/gitlab-ci/gromacs.gitlab-ci.yml | 35 +++++++++++++++ admin/ubsan-suppressions.txt | 9 ++++ cmake/gmxBuildTypeUBSAN.cmake | 43 +++++++++++++++++++ src/config.h.cmakein | 1 + .../applied_forces/tests/electricfield.cpp | 9 ++-- src/gromacs/domdec/domdec_topology.cpp | 10 ++--- src/gromacs/fileio/xdrd.cpp | 19 ++++---- src/gromacs/gmxpreprocess/gpp_nextnb.cpp | 8 ++-- src/gromacs/gmxpreprocess/pdb2gmx.cpp | 31 +++++++------ src/gromacs/mdrun/md.cpp | 4 +- src/gromacs/mdrun/minimize.cpp | 6 ++- src/gromacs/mdrun/rerun.cpp | 5 ++- src/gromacs/mdrun/runner.cpp | 10 ++--- src/gromacs/mdrunutility/threadaffinity.cpp | 6 +-- src/gromacs/nbnxm/pairlist.cpp | 12 +++--- .../onlinehelp/tests/helpwritercontext.cpp | 7 ++- src/gromacs/selection/indexutil.cpp | 10 ++++- src/gromacs/selection/parsetree.cpp | 5 ++- src/gromacs/selection/sm_same.cpp | 7 ++- src/gromacs/utility/arrayref.h | 6 +-- src/gromacs/utility/loggerbuilder.h | 4 +- 22 files changed, 180 insertions(+), 72 deletions(-) create mode 100644 admin/ubsan-suppressions.txt create mode 100644 cmake/gmxBuildTypeUBSAN.cmake diff --git a/CMakeLists.txt b/CMakeLists.txt index 3e902192dd..cbc6a16be4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -90,13 +90,14 @@ include(gmxBuildTypeProfile) include(gmxBuildTypeTSAN) include(gmxBuildTypeASAN) include(gmxBuildTypeMSAN) +include(gmxBuildTypeUBSAN) include(gmxBuildTypeReleaseWithAssert) if(NOT CMAKE_BUILD_TYPE) - set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel Reference RelWithAssert Profile TSAN ASAN MSAN." FORCE) + set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel Reference RelWithAssert Profile TSAN ASAN MSAN UBSAN." FORCE) # Set the possible values of build type for cmake-gui set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "Debug" "Release" - "MinSizeRel" "RelWithDebInfo" "Reference" "RelWithAssert" "Profile" "TSAN" "ASAN" "MSAN") + "MinSizeRel" "RelWithDebInfo" "Reference" "RelWithAssert" "Profile" "TSAN" "ASAN" "MSAN" "UBSAN") endif() if(CMAKE_CONFIGURATION_TYPES) # Add appropriate GROMACS-specific build types for the Visual diff --git a/admin/gitlab-ci/gromacs.gitlab-ci.yml b/admin/gitlab-ci/gromacs.gitlab-ci.yml index a74b8d1aa4..1c688dfa81 100644 --- a/admin/gitlab-ci/gromacs.gitlab-ci.yml +++ b/admin/gitlab-ci/gromacs.gitlab-ci.yml @@ -290,6 +290,16 @@ gromacs:clang-ASAN:configure: COMPILER_MAJOR_VERSION: 8 CMAKE_BUILD_TYPE_OPTIONS: "-DCMAKE_BUILD_TYPE=ASAN" +gromacs:clang-UBSAN:configure: + extends: + - .gromacs:base:configure + - .use-clang:base + - .rules:merge-requests + image: gromacs/cmake-3.13.0-llvm-8-tsan:master + variables: + COMPILER_MAJOR_VERSION: 8 + CMAKE_BUILD_TYPE_OPTIONS: "-DCMAKE_BUILD_TYPE=UBSAN" + gromacs:gcc-8-cuda-10.1:release:configure: extends: - .gromacs:base:release:configure @@ -435,6 +445,19 @@ gromacs:clang-ASAN:build: needs: - job: gromacs:clang-ASAN:configure +gromacs:clang-UBSAN:build: + extends: + - .variables:default + - .gromacs:base:build + - .use-clang:base + - .use-ccache + - .rules:merge-requests + image: gromacs/cmake-3.13.0-llvm-8-tsan:master + tags: + - k8s-scilifelab + needs: + - job: gromacs:clang-UBSAN:configure + gromacs:clang-static-analyzer:build: extends: - .variables:default @@ -539,6 +562,7 @@ gromacs:clang-8-cuda-10.1:release:build: CTEST_RUN_MODE: "ExperimentalTest" script: - cd $BUILD_DIR + - export UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1:suppressions=$CI_PROJECT_DIR/admin/ubsan-suppressions.txt # Needed to run MPI enabled code in the docker images, until we set up different users - export OMPI_ALLOW_RUN_AS_ROOT=1 - export OMPI_ALLOW_RUN_AS_ROOT_CONFIRM=1 @@ -649,6 +673,17 @@ gromacs:clang-ASAN:test: needs: - job: gromacs:clang-ASAN:build +gromacs:clang-UBSAN:test: + extends: + - .gromacs:base:test + - .use-clang:base + - .rules:merge-requests + image: gromacs/cmake-3.13.0-llvm-8-tsan:master + tags: + - k8s-scilifelab + needs: + - job: gromacs:clang-UBSAN:build + gromacs:clang-9-mpi:test: extends: - .gromacs:base:test diff --git a/admin/ubsan-suppressions.txt b/admin/ubsan-suppressions.txt new file mode 100644 index 0000000000..055ba2207e --- /dev/null +++ b/admin/ubsan-suppressions.txt @@ -0,0 +1,9 @@ +# Add UBSAN suppressions here, with a leading comment about how and +# why it is appropriate, and any circumstances about when it should be +# removed. + +# This is only a problem when using the built-in XDR implementation of +# xdr_vector. It supplies an extra argument needed for xdr_string +# which is unused by other xdr_* primitives like xdr_float. The issue +# is harmless and only active when not using a system XDR library. +function:xdr_vector diff --git a/cmake/gmxBuildTypeUBSAN.cmake b/cmake/gmxBuildTypeUBSAN.cmake new file mode 100644 index 0000000000..3e26087e99 --- /dev/null +++ b/cmake/gmxBuildTypeUBSAN.cmake @@ -0,0 +1,43 @@ +# +# This file is part of the GROMACS molecular simulation package. +# +# Copyright (c) 2020, by the GROMACS development team, led by +# Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, +# and including many others, as listed in the AUTHORS file in the +# top-level source directory and at http://www.gromacs.org. +# +# GROMACS is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public License +# as published by the Free Software Foundation; either version 2.1 +# of the License, or (at your option) any later version. +# +# GROMACS is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with GROMACS; if not, see +# http://www.gnu.org/licenses, or write to the Free Software Foundation, +# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# +# If you want to redistribute modifications to GROMACS, please +# consider that scientific software is very special. Version +# control is crucial - bugs must be traceable. We will be happy to +# consider code for inclusion in the official distribution, but +# derived work must not be called official GROMACS. Details are found +# in the README & COPYING files - if they are missing, get the +# official version at http://www.gromacs.org. +# +# To help us fund GROMACS development, we humbly ask that you cite +# the research papers on the package. Check out http://www.gromacs.org. + +# Custom build type "UBSAN", to be used to run the +# undefined behaviour memory checker. + +set(_flags "-O1 -g -fsanitize=undefined -fno-omit-frame-pointer") +foreach(_language C CXX) + string(REPLACE "X" "+" _human_readable_language ${_language}) + set(CMAKE_${_language}_FLAGS_UBSAN ${_flags} CACHE STRING "${_human_readable_language} flags for undefined behavior sanitizer" FORCE) + mark_as_advanced(CMAKE_${_language}_FLAGS_UBSAN) +endforeach() diff --git a/src/config.h.cmakein b/src/config.h.cmakein index bd831a529b..524ff47a53 100644 --- a/src/config.h.cmakein +++ b/src/config.h.cmakein @@ -248,6 +248,7 @@ #define CMAKE_BUILD_TYPE_TSAN 8 #define CMAKE_BUILD_TYPE_ASAN 9 #define CMAKE_BUILD_TYPE_MSAN 10 +#define CMAKE_BUILD_TYPE_UBSAN 11 #cmakedefine CMAKE_BUILD_TYPE CMAKE_BUILD_TYPE_@CMAKE_BUILD_TYPE_UPPER@ /* Define relative path to OpenCL kernels */ diff --git a/src/gromacs/applied_forces/tests/electricfield.cpp b/src/gromacs/applied_forces/tests/electricfield.cpp index cf7ed342ee..8e19955ae3 100644 --- a/src/gromacs/applied_forces/tests/electricfield.cpp +++ b/src/gromacs/applied_forces/tests/electricfield.cpp @@ -48,6 +48,7 @@ #include "gromacs/gmxlib/network.h" #include "gromacs/math/paddedvector.h" #include "gromacs/mdlib/forcerec.h" +#include "gromacs/mdtypes/commrec.h" #include "gromacs/mdtypes/enerdata.h" #include "gromacs/mdtypes/forceoutput.h" #include "gromacs/mdtypes/iforceprovider.h" @@ -107,11 +108,11 @@ public: // Prepare a ForceProviderInput t_mdatoms md; std::vector chargeA{ 1 }; - md.homenr = ssize(chargeA); - md.chargeA = chargeA.data(); - CommrecHandle cr = init_commrec(MPI_COMM_WORLD); + md.homenr = ssize(chargeA); + md.chargeA = chargeA.data(); + t_commrec cr; matrix boxDummy = { { 0, 0, 0 }, { 0, 0, 0 }, { 0, 0, 0 } }; - ForceProviderInput forceProviderInput({}, md, 0.0, boxDummy, *cr); + ForceProviderInput forceProviderInput({}, md, 0.0, boxDummy, cr); // Prepare a ForceProviderOutput PaddedVector f = { { 0, 0, 0 } }; diff --git a/src/gromacs/domdec/domdec_topology.cpp b/src/gromacs/domdec/domdec_topology.cpp index 0332a60ec4..fe4f8c82ee 100644 --- a/src/gromacs/domdec/domdec_topology.cpp +++ b/src/gromacs/domdec/domdec_topology.cpp @@ -121,11 +121,11 @@ struct thread_work_t */ thread_work_t(const gmx_ffparams_t& ffparams) : idef(ffparams) {} - InteractionDefinitions idef; /**< Partial local topology */ - std::unique_ptr vsitePbc; /**< vsite PBC structure */ - int nbonded; /**< The number of bondeds in this struct */ - ListOfLists excl; /**< List of exclusions */ - int excl_count; /**< The total exclusion count for \p excl */ + InteractionDefinitions idef; /**< Partial local topology */ + std::unique_ptr vsitePbc = nullptr; /**< vsite PBC structure */ + int nbonded = 0; /**< The number of bondeds in this struct */ + ListOfLists excl; /**< List of exclusions */ + int excl_count = 0; /**< The total exclusion count for \p excl */ }; /*! \brief Struct for the reverse topology: links bonded interactions to atomsx */ diff --git a/src/gromacs/fileio/xdrd.cpp b/src/gromacs/fileio/xdrd.cpp index 45175a5d6a..d954678b20 100644 --- a/src/gromacs/fileio/xdrd.cpp +++ b/src/gromacs/fileio/xdrd.cpp @@ -3,7 +3,7 @@ * * Copyright (c) 1991-2000, University of Groningen, The Netherlands. * Copyright (c) 2001-2004, The GROMACS development team. - * Copyright (c) 2013,2014,2015,2018,2019, by the GROMACS development team, led by + * Copyright (c) 2013,2014,2015,2018,2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -114,20 +114,17 @@ int xdr_int64(XDR* xdrs, int64_t* i) // requires 64-bit systems. static_assert(2 * sizeof(int) >= 8, "XDR handling assumes that an int64_t can be stored in two ints"); - int imaj, imin; - int ret; - static const int64_t two_p32_m1 = 0xFFFFFFFF; - int64_t imaj64, imin64; + static const uint64_t two_p32_m1 = 0xFFFFFFFF; - imaj64 = ((*i) >> 32) & two_p32_m1; - imin64 = (*i) & two_p32_m1; - imaj = static_cast(imaj64); - imin = static_cast(imin64); - ret = xdr_int(xdrs, &imaj); + uint64_t imaj64 = ((*i) >> 32) & two_p32_m1; + uint64_t imin64 = (*i) & two_p32_m1; + int imaj = static_cast(imaj64); + int imin = static_cast(imin64); + int ret = xdr_int(xdrs, &imaj); ret |= xdr_int(xdrs, &imin); - *i = ((static_cast(imaj) << 32) | (static_cast(imin) & two_p32_m1)); + *i = ((static_cast(imaj) << 32) | (static_cast(imin) & two_p32_m1)); return ret; } diff --git a/src/gromacs/gmxpreprocess/gpp_nextnb.cpp b/src/gromacs/gmxpreprocess/gpp_nextnb.cpp index 5f6801a4dc..e4b360bd1f 100644 --- a/src/gromacs/gmxpreprocess/gpp_nextnb.cpp +++ b/src/gromacs/gmxpreprocess/gpp_nextnb.cpp @@ -3,7 +3,7 @@ * * Copyright (c) 1991-2000, University of Groningen, The Netherlands. * Copyright (c) 2001-2004, The GROMACS development team. - * Copyright (c) 2011,2014,2015,2018,2019, by the GROMACS development team, led by + * Copyright (c) 2011,2014,2015,2018,2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -399,8 +399,10 @@ static void sort_and_purge_nnb(t_nextnb* nnb) for (n = 0; (n <= nnb->nrex); n++) { /* Sort atoms in this list */ - qsort(nnb->a[i][n], nnb->nrexcl[i][n], sizeof(int), compare_int); - + if (nnb->nrexcl[i][n] > 0) + { + qsort(nnb->a[i][n], nnb->nrexcl[i][n], sizeof(int), compare_int); + } cnt = 0; prev = -1; for (j = 0; j < nnb->nrexcl[i][n]; j++) diff --git a/src/gromacs/gmxpreprocess/pdb2gmx.cpp b/src/gromacs/gmxpreprocess/pdb2gmx.cpp index 8a9e08746c..6c76adfd97 100644 --- a/src/gromacs/gmxpreprocess/pdb2gmx.cpp +++ b/src/gromacs/gmxpreprocess/pdb2gmx.cpp @@ -1345,6 +1345,10 @@ public: itp_file_(nullptr), mHmult_(0) { + gmx::LoggerBuilder builder; + builder.addTargetStream(gmx::MDLogger::LogLevel::Info, &gmx::TextOutputFile::standardOutput()); + builder.addTargetStream(gmx::MDLogger::LogLevel::Warning, &gmx::TextOutputFile::standardError()); + loggerOwner_ = std::make_unique(builder.build()); } // From ICommandLineOptionsModule @@ -1405,15 +1409,15 @@ private: WaterType waterType_; MergeType mergeType_; - FILE* itp_file_; - char forcefield_[STRLEN]; - char ffdir_[STRLEN]; - char* ffname_; - char* watermodel_; - std::vector incls_; - std::vector mols_; - real mHmult_; - std::unique_ptr loggerPointer_; + FILE* itp_file_; + char forcefield_[STRLEN]; + char ffdir_[STRLEN]; + char* ffname_; + char* watermodel_; + std::vector incls_; + std::vector mols_; + real mHmult_; + std::unique_ptr loggerOwner_; }; void pdb2gmx::initOptions(IOptionsContainer* options, ICommandLineOptionsModuleSettings* settings) @@ -1693,7 +1697,7 @@ void pdb2gmx::optionsFinished() /* Force field selection, interactive or direct */ choose_ff(strcmp(ff_.c_str(), "select") == 0 ? nullptr : ff_.c_str(), forcefield_, - sizeof(forcefield_), ffdir_, sizeof(ffdir_), *loggerPointer_); + sizeof(forcefield_), ffdir_, sizeof(ffdir_), loggerOwner_->logger()); if (strlen(forcefield_) > 0) { @@ -1726,12 +1730,7 @@ int pdb2gmx::run() int this_chainstart; int prev_chainstart; - gmx::LoggerBuilder builder; - builder.addTargetStream(gmx::MDLogger::LogLevel::Info, &gmx::TextOutputFile::standardOutput()); - builder.addTargetStream(gmx::MDLogger::LogLevel::Warning, &gmx::TextOutputFile::standardError()); - gmx::LoggerOwner logOwner(builder.build()); - loggerPointer_ = std::make_unique(logOwner.logger()); - const gmx::MDLogger& logger = *loggerPointer_; + const gmx::MDLogger& logger = loggerOwner_->logger(); GMX_LOG(logger.info) .asParagraph() diff --git a/src/gromacs/mdrun/md.cpp b/src/gromacs/mdrun/md.cpp index bae77f14de..3f563e2016 100644 --- a/src/gromacs/mdrun/md.cpp +++ b/src/gromacs/mdrun/md.cpp @@ -253,7 +253,9 @@ void gmx::LegacySimulator::do_md() "Either specify the -ei option to mdrun, or do not use this checkpoint file."); } - initialize_lambdas(fplog, *ir, MASTER(cr), &state_global->fep_state, state_global->lambda, lam0); + int* fep_state = MASTER(cr) ? &state_global->fep_state : nullptr; + gmx::ArrayRef lambda = MASTER(cr) ? state_global->lambda : gmx::ArrayRef(); + initialize_lambdas(fplog, *ir, MASTER(cr), fep_state, lambda, lam0); Update upd(*ir, deform); const bool doSimulatedAnnealing = initSimulatedAnnealing(ir, &upd); const bool useReplicaExchange = (replExParams.exchangeInterval > 0); diff --git a/src/gromacs/mdrun/minimize.cpp b/src/gromacs/mdrun/minimize.cpp index c67ff09cc2..c96ef39acd 100644 --- a/src/gromacs/mdrun/minimize.cpp +++ b/src/gromacs/mdrun/minimize.cpp @@ -389,7 +389,9 @@ static void init_em(FILE* fplog, { state_global->ngtc = 0; } - initialize_lambdas(fplog, *ir, MASTER(cr), &(state_global->fep_state), state_global->lambda, nullptr); + int* fep_state = MASTER(cr) ? &state_global->fep_state : nullptr; + gmx::ArrayRef lambda = MASTER(cr) ? state_global->lambda : gmx::ArrayRef(); + initialize_lambdas(fplog, *ir, MASTER(cr), fep_state, lambda, nullptr); if (ir->eI == eiNM) { @@ -2543,7 +2545,7 @@ void LegacySimulator::do_steep() } /* Send IMD energies and positions, if bIMD is TRUE. */ - if (imdSession->run(count, TRUE, state_global->box, + if (imdSession->run(count, TRUE, MASTER(cr) ? state_global->box : nullptr, MASTER(cr) ? state_global->x.rvec_array() : nullptr, 0) && MASTER(cr)) { diff --git a/src/gromacs/mdrun/rerun.cpp b/src/gromacs/mdrun/rerun.cpp index 2d12115cf5..c241a6b499 100644 --- a/src/gromacs/mdrun/rerun.cpp +++ b/src/gromacs/mdrun/rerun.cpp @@ -277,8 +277,9 @@ void gmx::LegacySimulator::do_rerun() auto nonConstGlobalTopology = const_cast(top_global); nonConstGlobalTopology->intermolecularExclusionGroup = genQmmmIndices(*top_global); } - - initialize_lambdas(fplog, *ir, MASTER(cr), &state_global->fep_state, state_global->lambda, lam0); + int* fep_state = MASTER(cr) ? &state_global->fep_state : nullptr; + gmx::ArrayRef lambda = MASTER(cr) ? state_global->lambda : gmx::ArrayRef(); + initialize_lambdas(fplog, *ir, MASTER(cr), fep_state, lambda, lam0); const bool simulationsShareState = false; gmx_mdoutf* outf = init_mdoutf(fplog, nfile, fnm, mdrunOptions, cr, outputProvider, mdModulesNotifier, ir, top_global, oenv, wcycle, diff --git a/src/gromacs/mdrun/runner.cpp b/src/gromacs/mdrun/runner.cpp index b90bc9aea5..a32dc97a70 100644 --- a/src/gromacs/mdrun/runner.cpp +++ b/src/gromacs/mdrun/runner.cpp @@ -1051,9 +1051,9 @@ int Mdrunner::mdrunner() snew(oriresdata, 1); init_orires(fplog, &mtop, inputrec, cr, ms, globalState.get(), oriresdata); - auto deform = prepareBoxDeformation(globalState->box, MASTER(cr) ? DDRole::Master : DDRole::Agent, - PAR(cr) ? NumRanks::Multiple : NumRanks::Single, - cr->mpi_comm_mygroup, *inputrec); + auto deform = prepareBoxDeformation( + globalState != nullptr ? globalState->box : box, MASTER(cr) ? DDRole::Master : DDRole::Agent, + PAR(cr) ? NumRanks::Multiple : NumRanks::Single, cr->mpi_comm_mygroup, *inputrec); ObservablesHistory observablesHistory = {}; @@ -1098,7 +1098,7 @@ int Mdrunner::mdrunner() /* override nsteps with value set on the commandline */ override_nsteps_cmdline(mdlog, mdrunOptions.numStepsCommandline, inputrec); - if (SIMMASTER(cr)) + if (isSimulationMasterRank) { copy_mat(globalState->box, box); } @@ -1776,7 +1776,7 @@ int Mdrunner::mdrunner() /* we need to join all threads. The sub-threads join when they exit this function, but the master thread needs to be told to wait for that. */ - if (PAR(cr) && MASTER(cr)) + if (MASTER(cr)) { tMPI_Finalize(); } diff --git a/src/gromacs/mdrunutility/threadaffinity.cpp b/src/gromacs/mdrunutility/threadaffinity.cpp index a2d08a0f59..8cf377acbf 100644 --- a/src/gromacs/mdrunutility/threadaffinity.cpp +++ b/src/gromacs/mdrunutility/threadaffinity.cpp @@ -541,10 +541,8 @@ static bool detectDefaultAffinityMask(const int nthreads_hw_avail) MPI_Initialized(&mpiIsInitialized); if (mpiIsInitialized) { - bool detectedDefaultAffinityMaskOnAllRanks; - MPI_Allreduce(&detectedDefaultAffinityMask, &detectedDefaultAffinityMaskOnAllRanks, 1, - MPI_C_BOOL, MPI_LAND, MPI_COMM_WORLD); - detectedDefaultAffinityMask = detectedDefaultAffinityMaskOnAllRanks; + bool maskToReduce = detectedDefaultAffinityMask; + MPI_Allreduce(&maskToReduce, &detectedDefaultAffinityMask, 1, MPI_C_BOOL, MPI_LAND, MPI_COMM_WORLD); } #endif diff --git a/src/gromacs/nbnxm/pairlist.cpp b/src/gromacs/nbnxm/pairlist.cpp index 253a4cd97a..ad09daed08 100644 --- a/src/gromacs/nbnxm/pairlist.cpp +++ b/src/gromacs/nbnxm/pairlist.cpp @@ -3894,7 +3894,7 @@ static Range getIZoneRange(const Nbnxm::GridSet::DomainSetup& domainSetup, } /* Returns the j-zone range for pairlist construction for the give locality and i-zone */ -static Range getJZoneRange(const gmx_domdec_zones_t& ddZones, +static Range getJZoneRange(const gmx_domdec_zones_t* ddZones, const InteractionLocality locality, const int iZone) { @@ -3906,12 +3906,12 @@ static Range getJZoneRange(const gmx_domdec_zones_t& ddZones, else if (iZone == 0) { /* Non-local: we need to avoid the local (zone 0 vs 0) interactions */ - return { 1, *ddZones.iZones[iZone].jZoneRange.end() }; + return { 1, *ddZones->iZones[iZone].jZoneRange.end() }; } else { /* Non-local with non-local i-zone: use all j-zones */ - return ddZones.iZones[iZone].jZoneRange; + return ddZones->iZones[iZone].jZoneRange; } } @@ -3977,7 +3977,9 @@ void PairlistSet::constructPairlists(const Nbnxm::GridSet& gridSet, } } - const gmx_domdec_zones_t& ddZones = *gridSet.domainSetup().zones; + const gmx_domdec_zones_t* ddZones = gridSet.domainSetup().zones; + GMX_ASSERT(locality_ == InteractionLocality::Local || ddZones != nullptr, + "Nonlocal interaction locality with null ddZones."); const auto iZoneRange = getIZoneRange(gridSet.domainSetup(), locality_); @@ -4003,7 +4005,7 @@ void PairlistSet::constructPairlists(const Nbnxm::GridSet& gridSet, /* With GPU: generate progressively smaller lists for * load balancing for local only or non-local with 2 zones. */ - progBal = (locality_ == InteractionLocality::Local || ddZones.n <= 2); + progBal = (locality_ == InteractionLocality::Local || ddZones->n <= 2); #pragma omp parallel for num_threads(numLists) schedule(static) for (int th = 0; th < numLists; th++) diff --git a/src/gromacs/onlinehelp/tests/helpwritercontext.cpp b/src/gromacs/onlinehelp/tests/helpwritercontext.cpp index eba2f1abf6..bab07ea99c 100644 --- a/src/gromacs/onlinehelp/tests/helpwritercontext.cpp +++ b/src/gromacs/onlinehelp/tests/helpwritercontext.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2015,2017,2019, by the GROMACS development team, led by + * Copyright (c) 2015,2017,2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -50,6 +50,7 @@ #include "gromacs/utility/arrayref.h" #include "gromacs/utility/gmxassert.h" #include "gromacs/utility/stringutil.h" +#include "gromacs/utility/textwriter.h" #include "testutils/stringtest.h" @@ -65,7 +66,9 @@ class HelpWriterContextTest : public gmx::test::StringTestBase public: void testFormatting(const std::string& text, gmx::HelpOutputFormat format, const char* id) { - gmx::HelpWriterContext context(nullptr, format); + FILE* fp = nullptr; + gmx::TextWriter writerStub(fp); + gmx::HelpWriterContext context(&writerStub, format); std::string result = context.substituteMarkupAndWrapToString(settings_, text); if (id == nullptr) { diff --git a/src/gromacs/selection/indexutil.cpp b/src/gromacs/selection/indexutil.cpp index 55a38ee961..5a6a4507bc 100644 --- a/src/gromacs/selection/indexutil.cpp +++ b/src/gromacs/selection/indexutil.cpp @@ -849,7 +849,10 @@ void gmx_ana_index_make_block(t_blocka* t, const gmx_mtop_t* top, gmx_ana_index_ srenew(t->a, g->isize); t->nalloc_a = g->isize; } - std::memcpy(t->a, g->index, g->isize * sizeof(*(t->a))); + if (t->nra > 0) + { + std::memcpy(t->a, g->index, g->isize * sizeof(*(t->a))); + } } /* Allocate memory for the block index. We don't know in advance @@ -1295,7 +1298,10 @@ void gmx_ana_indexmap_copy(gmx_ana_indexmap_t* dest, gmx_ana_indexmap_t* src, bo dest->b.nra = src->b.nra; std::memcpy(dest->orgid, src->orgid, dest->b.nr * sizeof(*dest->orgid)); std::memcpy(dest->b.index, src->b.index, (dest->b.nr + 1) * sizeof(*dest->b.index)); - std::memcpy(dest->b.a, src->b.a, dest->b.nra * sizeof(*dest->b.a)); + if (dest->b.nra > 0) + { + std::memcpy(dest->b.a, src->b.a, dest->b.nra * sizeof(*dest->b.a)); + } } dest->mapb.nr = src->mapb.nr; dest->mapb.nra = src->mapb.nra; diff --git a/src/gromacs/selection/parsetree.cpp b/src/gromacs/selection/parsetree.cpp index 1294a13801..d7e536358e 100644 --- a/src/gromacs/selection/parsetree.cpp +++ b/src/gromacs/selection/parsetree.cpp @@ -497,7 +497,10 @@ void _gmx_selelem_init_method_params(const gmx::SelectionTreeElementPointer& sel nparams = sel->u.expr.method->nparams; orgparam = sel->u.expr.method->param; snew(param, nparams); - memcpy(param, orgparam, nparams * sizeof(gmx_ana_selparam_t)); + if (nparams > 0) + { + memcpy(param, orgparam, nparams * sizeof(gmx_ana_selparam_t)); + } for (i = 0; i < nparams; ++i) { param[i].flags &= ~SPAR_SET; diff --git a/src/gromacs/selection/sm_same.cpp b/src/gromacs/selection/sm_same.cpp index c3aa2123da..abcd1745cd 100644 --- a/src/gromacs/selection/sm_same.cpp +++ b/src/gromacs/selection/sm_same.cpp @@ -513,8 +513,11 @@ static void evaluate_same_str(const gmx::SelMethodEvalContext& /*context*/, while (j < g->isize) { /* Do a binary search of the strings. */ - void* ptr; - ptr = bsearch(&d->val.s[j], d->as_s_sorted, d->nas, sizeof(d->as_s_sorted[0]), &cmp_str); + void* ptr = nullptr; + if (d->nas > 0) + { + ptr = bsearch(&d->val.s[j], d->as_s_sorted, d->nas, sizeof(d->as_s_sorted[0]), &cmp_str); + } /* Check whether the value was found in the as list. */ if (ptr == nullptr) { diff --git a/src/gromacs/utility/arrayref.h b/src/gromacs/utility/arrayref.h index 419450c5c3..f2c852b970 100644 --- a/src/gromacs/utility/arrayref.h +++ b/src/gromacs/utility/arrayref.h @@ -78,7 +78,7 @@ struct ArrayRefIter : constexpr ArrayRefIter(ArrayRefIter> it) noexcept : it_(&*it) { } - + constexpr T* data() const noexcept { return it_; } constexpr T& operator*() const noexcept { return *it_; } constexpr ArrayRefIter& operator+=(std::ptrdiff_t i) noexcept { @@ -270,12 +270,12 @@ public: return begin_[n]; } //! Returns the first element. - reference front() const { return *begin_; } + reference front() const { return *(begin_); } //! Returns the first element. reference back() const { return *(end_ - 1); } //! Returns a raw pointer to the contents of the array. - pointer data() const { return &*begin_; } + pointer data() const { return begin_.data(); } /*! \brief * Swaps referenced memory with the other object. diff --git a/src/gromacs/utility/loggerbuilder.h b/src/gromacs/utility/loggerbuilder.h index 1cc1e8a7ad..5948a01e6c 100644 --- a/src/gromacs/utility/loggerbuilder.h +++ b/src/gromacs/utility/loggerbuilder.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2016,2018,2019, by the GROMACS development team, led by + * Copyright (c) 2016,2018,2019,2020, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -111,7 +111,7 @@ private: * * This class is responsible of managing all memory allocated by LoggerBuilder * that is needed for operation of the actual logger. Also the actual logger - * instance is owned by this class. This allows keeing the actual logger simple + * instance is owned by this class. This allows keeping the actual logger simple * and streamlined. * * This class supports move construction and assignment, which allows -- 2.22.0