Skip to content

Commit

Permalink
Coverity Scan (AMReX-Codes#3361)
Browse files Browse the repository at this point in the history 
## Summary

Fix issues found by Coverity Scan, including infinite loop bugs in the
kernels of MLALaplacian.

The scan will be done nightly on a local workstation.

Add coverity scan and license badges to README.md. Remove the cmake
README.md because it's broken.

## Checklist

The proposed changes:
- [x] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate
  • Loading branch information
@WeiqunZhang
WeiqunZhang authored Jun 12, 2023
1 parent 65ba959 commit 0375e78
Show file tree
Hide file tree
Showing 53 changed files with 341 additions and 195 deletions.
7 changes: 6 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@
<a href="https://doi.org/10.5281/zenodo.2555438">
<img src="https://zenodo.org/badge/DOI/10.5281/zenodo.2555438.svg" alt="DOI">
</a>
<img src="https://github.com/AMReX-codes/amrex/workflows/cmake/badge.svg?branch=development" alt="CI: CMake on Development">
<a href="https://scan.coverity.com/projects/amrex-codes-amrex">
<img alt="Coverity Scan Build Status" src="https://scan.coverity.com/projects/28563/badge.svg">
</a>
<a href="https://opensource.org/licenses/BSD-3-Clause">
<img alt="License" src="https://img.shields.io/badge/License-BSD_3--Clause-blue.svg">
</a>
</p>


Expand Down
6 changes: 3 additions & 3 deletions Src/Amr/AMReX_Amr.H
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,8 +407,8 @@ protected:
std::string regrid_grids_file; //!< Grids file that will bypass regridding.
std::string initial_grids_file; //!< Grids file that will bypass regridding only at initialization.
Vector<std::unique_ptr<AmrLevel> > amr_level; //!< Vector of levels
Real cumtime; //!< Physical time variable.
Real start_time; //!< Physical time this simulation started.
Real cumtime = std::numeric_limits<Real>::lowest(); //!< Physical time variable.
Real start_time = std::numeric_limits<Real>::lowest(); //!< Physical time this simulation started.
Vector<Real> dt_level; //!< Timestep at this level.
Vector<int> level_steps; //!< Number of time steps at this level.
Vector<int> level_count;
Expand All @@ -434,7 +434,7 @@ protected:
std::string plot_file_root; //!< Root name of plotfile.
std::string small_plot_file_root; //!< Root name of small plotfile.

int which_level_being_advanced; //!< Only >=0 if we are in Amr::timeStep(level,...)
int which_level_being_advanced = -1; //!< Only >=0 if we are in Amr::timeStep(level,...)

int record_grid_info;
int record_run_info;
Expand Down
33 changes: 25 additions & 8 deletions Src/Amr/AMReX_Amr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,8 @@ Amr::InitAmr ()
//
// Otherwise we expect a vector of max_level values
//
pp.queryarr("regrid_int",regrid_int,0,max_level);
auto status = pp.queryarr("regrid_int",regrid_int,0,max_level);
amrex::ignore_unused(status);
}
}

Expand All @@ -416,6 +417,7 @@ Amr::InitAmr ()
int in_finest,ngrid;

is >> in_finest;
AMREX_ASSERT(in_finest >= 0 && in_finest < std::numeric_limits<int>::max());
STRIP;
initial_ba.resize(in_finest);

Expand All @@ -428,6 +430,7 @@ Amr::InitAmr ()
BoxList bl;
is >> ngrid;
STRIP;
AMREX_ASSERT(ngrid >= 0 && ngrid < std::numeric_limits<int>::max());
for (int i = 0; i < ngrid; i++)
{
Box bx;
Expand Down Expand Up @@ -458,11 +461,13 @@ Amr::InitAmr ()

is >> in_finest;
STRIP;
AMREX_ASSERT(in_finest >= 0 && in_finest < std::numeric_limits<int>::max());
regrid_ba.resize(in_finest);
for (int lev = 1; lev <= in_finest; lev++)
{
BoxList bl;
is >> ngrid;
AMREX_ASSERT(ngrid >= 0 && ngrid < std::numeric_limits<int>::max());
STRIP;
for (int i = 0; i < ngrid; i++)
{
Expand Down Expand Up @@ -1007,7 +1012,9 @@ Amr::writePlotFileDoit (std::string const& pltfile, bool regular)
ParallelDescriptor::Barrier("Amr::writePlotFile::end");
if(ParallelDescriptor::IOProcessor()) {
HeaderFile.close();
std::rename(pltfileTemp.c_str(), pltfile.c_str());
if (std::rename(pltfileTemp.c_str(), pltfile.c_str())) {
amrex::Abort("Amr::writePlotFileDoit: std::rename failed");
}
}
ParallelDescriptor::Barrier("Renaming temporary plotfile.");
//
Expand Down Expand Up @@ -1460,6 +1467,9 @@ Amr::restart (const std::string& filename)
is >> mx_lev;
is >> finest_level;

AMREX_ASSERT(mx_lev >= 0 && mx_lev < 1000 &&
finest_level >= 0 && finest_level < 1000);

Vector<Box> inputs_domain(max_level+1);
for (int lev = 0; lev <= max_level; ++lev)
{
Expand Down Expand Up @@ -1837,7 +1847,9 @@ Amr::checkPoint ()
ParallelDescriptor::Barrier("Amr::checkPoint::end");
if(ParallelDescriptor::IOProcessor()) {
HeaderFile.close();
std::rename(ckfileTemp.c_str(), ckfile.c_str());
if (std::rename(ckfileTemp.c_str(), ckfile.c_str())) {
amrex::Abort("Amr::checkPoint: std::rename failed");
}
}
ParallelDescriptor::Barrier("Renaming temporary checkPoint file.");
}
Expand Down Expand Up @@ -2215,34 +2227,39 @@ Amr::coarseTimeStep (Real stop_time)
FILE *fp;
if ((fp=fopen("dump_and_continue","r")) != nullptr) // NOLINT(bugprone-assignment-in-if-condition)
{
remove("dump_and_continue");
auto status = remove("dump_and_continue");
amrex::ignore_unused(status);
to_checkpoint = 1;
fclose(fp);
}
else if ((fp=fopen("stop_run","r")) != nullptr) // NOLINT(bugprone-assignment-in-if-condition)
{
remove("stop_run");
auto status = remove("stop_run");
amrex::ignore_unused(status);
to_stop = 1;
fclose(fp);
}
else if ((fp=fopen("dump_and_stop","r")) != nullptr) // NOLINT(bugprone-assignment-in-if-condition)
{
remove("dump_and_stop");
auto status = remove("dump_and_stop");
amrex::ignore_unused(status);
to_checkpoint = 1;
to_stop = 1;
fclose(fp);
}

if ((fp=fopen("plot_and_continue","r")) != nullptr) // NOLINT(bugprone-assignment-in-if-condition)
{
remove("plot_and_continue");
auto status = remove("plot_and_continue");
amrex::ignore_unused(status);
to_plot = 1;
fclose(fp);
}

if ((fp=fopen("small_plot_and_continue","r")) != nullptr) // NOLINT(bugprone-assignment-in-if-condition)
{
remove("small_plot_and_continue");
auto status = remove("small_plot_and_continue");
amrex::ignore_unused(status);
to_small_plot = 1;
fclose(fp);
}
Expand Down
14 changes: 7 additions & 7 deletions Src/Amr/AMReX_AmrLevel.H
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,14 +588,14 @@ private:
MultiFab& m_leveldata;
MultiFabCopyDescriptor m_mfcd;
Vector< Vector<MultiFabId> > m_mfid; // [level][oldnew]
Interpolater* m_map;
Interpolater* m_map = nullptr;
std::map<int,Box> m_ba;
Real m_time;
int m_growsize;
int m_index;
int m_scomp;
int m_ncomp;
bool m_FixUpCorners;
Real m_time = std::numeric_limits<Real>::lowest();
int m_growsize = std::numeric_limits<int>::lowest();
int m_index = -1;
int m_scomp = -1;
int m_ncomp = -1;
bool m_FixUpCorners = false;

std::map< int,Vector< Vector<Box> > > m_fbox; // [grid][level][validregion]
std::map< int,Vector< Vector<Box> > > m_cbox; // [grid][level][fillablesubbox]
Expand Down
9 changes: 5 additions & 4 deletions Src/Amr/AMReX_AmrLevel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,8 @@ AmrLevel::restart (Amr& papa,
fine_ratio = IntVect::TheUnitVector(); fine_ratio.scale(-1);
crse_ratio = IntVect::TheUnitVector(); crse_ratio.scale(-1);

AMREX_ASSERT(level >= 0 && level <= parent->maxLevel());

if (level > 0)
{
crse_ratio = parent->refRatio(level-1);
Expand Down Expand Up @@ -2074,10 +2076,9 @@ void AmrLevel::constructAreaNotToTag ()

// We disallow tagging in the remaining part of the domain.
BoxArray tagba = amrex::boxComplement(parent->Geom(level).Domain(),m_AreaToTag);
m_AreaNotToTag = tagba;
m_AreaNotToTag = std::move(tagba);

BoxArray bxa(parent->Geom(level).Domain());
BL_ASSERT(bxa.contains(m_AreaNotToTag));
BL_ASSERT(parent->Geom(level).Domain().contains(m_AreaNotToTag.minimalBox()));
}

if (parent->useFixedUpToLevel()<level)
Expand All @@ -2087,7 +2088,7 @@ void AmrLevel::constructAreaNotToTag ()
tagarea.grow(-parent->blockingFactor(level));
m_AreaToTag = tagarea;
BoxArray tagba = amrex::boxComplement(parent->Geom(level).Domain(),m_AreaToTag);
m_AreaNotToTag = tagba;
m_AreaNotToTag = std::move(tagba);
}
}

Expand Down
8 changes: 8 additions & 0 deletions Src/Amr/AMReX_StateData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ StateData::restartDoit (std::istream& is, const std::string& chkfile)

int nsets;
is >> nsets;
AMREX_ASSERT(nsets >= 0 && nsets <= 2);

new_data = std::make_unique<MultiFab>(grids,dmap,desc->nComp(),desc->nExtra(),
MFInfo().SetTag("StateData").SetArena(arena),
Expand Down Expand Up @@ -879,6 +880,7 @@ StateDataPhysBCFunct::operator() (MultiFab& mf, int dest_comp, int num_comp, Int

bool has_bndryfunc_fab = statedata->desc->hasBndryFuncFab();
bool run_on_gpu = statedata->desc->RunOnGPU() && Gpu::inLaunchRegion();
amrex::ignore_unused(run_on_gpu);

#ifdef AMREX_USE_OMP
#pragma omp parallel if (!run_on_gpu)
Expand All @@ -889,7 +891,9 @@ StateDataPhysBCFunct::operator() (MultiFab& mf, int dest_comp, int num_comp, Int
for (MFIter mfi(mf); mfi.isValid(); ++mfi)
{
FArrayBox& dest = mf[mfi];
#ifdef AMREX_USE_GPU
Array4<Real> const& desta = dest.array();
#endif
const Box& bx = dest.box();

bool has_phys_bc = false;
Expand Down Expand Up @@ -939,6 +943,7 @@ StateDataPhysBCFunct::operator() (MultiFab& mf, int dest_comp, int num_comp, Int

if (lo_slab.ok())
{
#ifdef AMREX_USE_GPU
if (run_on_gpu)
{
tmp.resize(lo_slab,num_comp);
Expand Down Expand Up @@ -990,6 +995,7 @@ StateDataPhysBCFunct::operator() (MultiFab& mf, int dest_comp, int num_comp, Int
});
}
else
#endif
{
tmp.resize(lo_slab,num_comp);
const Box db = amrex::shift(lo_slab, dir, -geom.period(dir));
Expand All @@ -1005,6 +1011,7 @@ StateDataPhysBCFunct::operator() (MultiFab& mf, int dest_comp, int num_comp, Int

if (hi_slab.ok())
{
#ifdef AMREX_USE_GPU
if (run_on_gpu)
{
tmp.resize(hi_slab,num_comp);
Expand Down Expand Up @@ -1056,6 +1063,7 @@ StateDataPhysBCFunct::operator() (MultiFab& mf, int dest_comp, int num_comp, Int
});
}
else
#endif
{
tmp.resize(hi_slab,num_comp);
const Box db = amrex::shift(hi_slab, dir, geom.period(dir));
Expand Down
1 change: 1 addition & 0 deletions Src/AmrCore/AMReX_AmrMesh.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ AmrMesh::InitAmrMesh (int max_level_in, const Vector<int>& n_cell_in,
} else {
max_level = max_level_in;
}
AMREX_ASSERT(max_level >= 0 && max_level < 1000);

int nlev = max_level + 1;

Expand Down
7 changes: 6 additions & 1 deletion Src/AmrCore/AMReX_FluxRegister.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ FluxRegister::OverwriteFlux (Array<MultiFab*,AMREX_SPACEDIM> const& crse_fluxes,
}

bool run_on_gpu = Gpu::inLaunchRegion();
amrex::ignore_unused(run_on_gpu);

// cell-centered mask:
constexpr int crse_cell = 0;
Expand Down Expand Up @@ -772,6 +773,7 @@ FluxRegister::OverwriteFlux (Array<MultiFab*,AMREX_SPACEDIM> const& crse_fluxes,
{
const std::vector<IntVect>& pshifts = cperiod.shiftIntVect();
Vector<Array4BoxTag<int> > tags;
amrex::ignore_unused(tags);

#ifdef AMREX_USE_OMP
#pragma omp parallel if (!run_on_gpu)
Expand Down Expand Up @@ -801,9 +803,12 @@ FluxRegister::OverwriteFlux (Array<MultiFab*,AMREX_SPACEDIM> const& crse_fluxes,
for (const auto& is : isects)
{
Box const& b = is.second-iv;
#ifdef AMREX_USE_GPU
if (run_on_gpu) {
tags.push_back({fab,b});
} else {
} else
#endif
{
cc_mask[mfi].setVal<RunOn::Host>(fine_cell, b, 0, 1);
}
}
Expand Down
30 changes: 23 additions & 7 deletions Src/AmrCore/AMReX_Interpolater.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ CellConservativeLinear::interp (const FArrayBox& crse,
AMREX_ASSERT(fine.box().contains(fine_region));

bool run_on_gpu = (runon == RunOn::Gpu && Gpu::inLaunchRegion());
amrex::ignore_unused(run_on_gpu);

Box const& cdomain = crse_geom.Domain();
amrex::ignore_unused(fine_geom);
Expand All @@ -455,14 +456,19 @@ CellConservativeLinear::interp (const FArrayBox& crse,
const Box& crse_region = CoarseBox(fine_region,ratio);
const Box& cslope_bx = amrex::grow(crse_region,-1);

FArrayBox ccfab(cslope_bx, ncomp*AMREX_SPACEDIM);
Array4<Real> const& tmp = ccfab.array();
Array4<Real const> const& ctmp = ccfab.const_array();

#ifdef AMREX_USE_GPU
AsyncArray<BCRec> async_bcr(bcr.data(), (run_on_gpu) ? ncomp : 0);
BCRec const* bcrp = (run_on_gpu) ? async_bcr.data() : bcr.data();

FArrayBox ccfab(cslope_bx, ncomp*AMREX_SPACEDIM);
Elixir cceli;
if (run_on_gpu) cceli = ccfab.elixir();
Array4<Real> const& tmp = ccfab.array();
Array4<Real const> const& ctmp = ccfab.const_array();
#else
BCRec const* bcrp = bcr.data();
#endif

#if (AMREX_SPACEDIM == 1)
if (crse_geom.IsSPHERICAL()) {
Expand Down Expand Up @@ -593,23 +599,29 @@ CellQuadratic::interp (const FArrayBox& crse,

// Are we running on GPU?
bool run_on_gpu = (runon == RunOn::Gpu && Gpu::inLaunchRegion());
amrex::ignore_unused(run_on_gpu);

// Set up domain for coarse geometry
Box const& cdomain = crse_geom.Domain();

// Set up AsyncArray for boundary conditions
AsyncArray<BCRec> async_bcr(bcr.data(), (run_on_gpu) ? ncomp : 0);
BCRec const* bcrp = (run_on_gpu) ? async_bcr.data() : bcr.data();

// Set up temporary fab (with elixir, as needed) for coarse grid slopes
#if (AMREX_SPACEDIM == 2)
int nslp = 5; // x, y, x^2, y^2, xy, in that order.
#else /* AMREX_SPACEDIM == 3 */
int nslp = 9; // x, y, z, x^2, y^2, z^2, xy, xz, yz, in that order.
#endif /* AMREX_SPACEDIM == 2 */
FArrayBox sfab(cslope_bx, nslp*ncomp);

#ifdef AMREX_USE_GPU
// Set up AsyncArray for boundary conditions
AsyncArray<BCRec> async_bcr(bcr.data(), (run_on_gpu) ? ncomp : 0);
BCRec const* bcrp = (run_on_gpu) ? async_bcr.data() : bcr.data();

Elixir seli;
if (run_on_gpu) seli = sfab.elixir();
#else
BCRec const* bcrp = bcr.data();
#endif

// Extract pointers to fab data
Array4<Real> const& finearr = fine.array();
Expand Down Expand Up @@ -1013,8 +1025,10 @@ CellQuartic::interp (const FArrayBox& crse,
Box bz = amrex::coarsen(target_fine_region, IntVect(2,2,1));
bz.grow(IntVect(2,2,0));
FArrayBox tmpz(bz, ncomp);
#ifdef AMREX_USE_GPU
Elixir tmpz_eli;
if (run_on_gpu) tmpz_eli = tmpz.elixir();
#endif
Array4<Real> const& tmpzarr = tmpz.array();
AMREX_HOST_DEVICE_PARALLEL_FOR_4D_FLAG(runon, bz, ncomp, i, j, k, n,
{
Expand All @@ -1026,8 +1040,10 @@ CellQuartic::interp (const FArrayBox& crse,
Box by = amrex::coarsen(target_fine_region, IntVect(AMREX_D_DECL(2,1,1)));
by.grow(IntVect(AMREX_D_DECL(2,0,0)));
FArrayBox tmpy(by, ncomp);
#ifdef AMREX_USE_GPU
Elixir tmpy_eli;
if (run_on_gpu) tmpy_eli = tmpy.elixir();
#endif
Array4<Real> const& tmpyarr = tmpy.array();
#if (AMREX_SPACEDIM == 2)
Array4<Real const> srcarr = crsearr;
Expand Down
Loading

0 comments on commit 0375e78

Please sign in to comment.