Fix mpiio with stdlibc++ range checking #3234

mkuron · 2019-10-05T06:32:43Z

Fixes #3230. Reported by @junghans.

When mpiio was used but no bonds were present, we would still try to copy zero bonds from a zero-length vector. This triggered an assertion when stdlibc++ range checking was enabled.

Please tag for cherry-picking into 4.1.1.

codecov · 2019-10-05T08:01:37Z

Codecov Report

Merging #3234 into python will decrease coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           python   #3234   +/-   ##
======================================
- Coverage      85%     85%   -1%     
======================================
  Files         528     528           
  Lines       25805   25805           
======================================
- Hits        22150   22149    -1     
- Misses       3655    3656    +1

Impacted Files	Coverage Δ
src/core/io/mpiio/mpiio.cpp	`84% <100%> (ø)`	⬆️
src/core/particle_data.cpp	`95% <0%> (-1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dc1e06...dd68957. Read the comment docs.

src/core/io/mpiio/mpiio.cpp

hirschsn · 2019-10-07T06:50:30Z

I tend to think that the C++ implementation used in #3230 is broken in this case. The standard states for copy_n:
"Effects: For each non-negative integer i < n, performs *(result + i) = *(first + i)."

If an implementation for copy_n asserts: __n < this->size(), the implementation forgot the corner case: __n == 0. Or am I missing something here?

Nonetheless, this PR looks good because it is an easy workaround for the problem.

src/core/io/mpiio/mpiio.cpp

mkuron · 2019-10-07T07:43:16Z

@hirschsn, the assertion is not inside std::copy_n, but in std::vector::operator[]. And that's standard-compliant because subscripting beyond bounds is undefined.

hirschsn · 2019-10-07T08:20:35Z

@mkuron I see. That makes sense. :)

Wouldn't bond.begin() + off where off == 0 in this case fix the problem already?

NVM, I see that's what you've done.

mkuron · 2019-10-07T16:48:00Z

Build is failing because of #3235, which is fixed by espressomd/docker#139.

jngrad · 2019-10-11T18:33:55Z

bors r=fweik

@junghans

3234: Fix mpiio with stdlibc++ range checking r=fweik a=mkuron Fixes #3230. Reported by @junghans. When mpiio was used but no bonds were present, we would still try to copy zero bonds from a zero-length vector. This triggered an assertion when stdlibc++ range checking was enabled. Please tag for cherry-picking into 4.1.1. 3236: ESS2019 installation guide updates r=KaiSzuttor a=mkuron Lessons learned today: - We require MPI 3 because we depend on const-correctness in a few places. That means that OpenMPI 1.6.5 and lower are not supported anymore. - Installing the ROCm driver breaks access to /dev/kfd, causing hwloc initialization during `mpiexec` to hang. Rebooting helps. - Add matplotlib, ipython and jupyter to the Mac install guide. - Homebrew now defaults to Python 3, requires manually enabling cython, and it's unclear whether the hdf5 package still supports MPI (Homebrew/homebrew-core#26974) - Anaconda (~/anaconda[23]) and python.org packages (/Library/Python and /usr/local/bin) are also sources of conflict Please tag for the 4.1.1 release 3238: maintainer: Escape module python in wrapper script r=jngrad a=fweik Fixes #3237. Description of changes: - Added quotes around the module path in python wrapper script. Co-authored-by: Michael Kuron <mkuron@users.noreply.github.com> Co-authored-by: Michael Kuron <mkuron@icp.uni-stuttgart.de> Co-authored-by: Kai Szuttor <kai@icp.uni-stuttgart.de> Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de>

bors · 2019-10-11T18:48:59Z

Canceled (will resume)

jngrad · 2019-10-11T18:50:12Z

bors r-
(failed CI)

bors · 2019-10-11T18:50:14Z

Canceled

jngrad · 2019-10-11T18:51:37Z

bors r=fweik

@junghans

3234: Fix mpiio with stdlibc++ range checking r=fweik a=mkuron Fixes #3230. Reported by @junghans. When mpiio was used but no bonds were present, we would still try to copy zero bonds from a zero-length vector. This triggered an assertion when stdlibc++ range checking was enabled. Please tag for cherry-picking into 4.1.1. 3236: ESS2019 installation guide updates r=KaiSzuttor a=mkuron Lessons learned today: - We require MPI 3 because we depend on const-correctness in a few places. That means that OpenMPI 1.6.5 and lower are not supported anymore. - Installing the ROCm driver breaks access to /dev/kfd, causing hwloc initialization during `mpiexec` to hang. Rebooting helps. - Add matplotlib, ipython and jupyter to the Mac install guide. - Homebrew now defaults to Python 3, requires manually enabling cython, and it's unclear whether the hdf5 package still supports MPI (Homebrew/homebrew-core#26974) - Anaconda (~/anaconda[23]) and python.org packages (/Library/Python and /usr/local/bin) are also sources of conflict Please tag for the 4.1.1 release 3238: maintainer: Escape module python in wrapper script r=jngrad a=fweik Fixes #3237. Description of changes: - Added quotes around the module path in python wrapper script. Co-authored-by: Michael Kuron <mkuron@users.noreply.github.com> Co-authored-by: Michael Kuron <mkuron@icp.uni-stuttgart.de> Co-authored-by: Kai Szuttor <kai@icp.uni-stuttgart.de> Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de>

bors · 2019-10-11T19:50:36Z

Build succeeded

ICP GitLab CI

mkuron added 2 commits October 5, 2019 08:28

Fix mpiio with stdlibc++ range checking

3cab308

Formatting

a500ee3

jngrad added this to the Espresso 4.1.1 milestone Oct 5, 2019

jngrad approved these changes Oct 5, 2019

View reviewed changes

jngrad added the BugFix label Oct 5, 2019

fweik suggested changes Oct 5, 2019

View reviewed changes

src/core/io/mpiio/mpiio.cpp Outdated Show resolved Hide resolved

hirschsn reviewed Oct 7, 2019

View reviewed changes

src/core/io/mpiio/mpiio.cpp Outdated Show resolved Hide resolved

use iterator arithmetic inside mpiio_common_read

dd68957

fweik approved these changes Oct 7, 2019

View reviewed changes

bors bot merged commit dd68957 into espressomd:python Oct 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mpiio with stdlibc++ range checking #3234

Fix mpiio with stdlibc++ range checking #3234

mkuron commented Oct 5, 2019

codecov bot commented Oct 5, 2019 •

edited

Loading

hirschsn commented Oct 7, 2019

mkuron commented Oct 7, 2019

hirschsn commented Oct 7, 2019 •

edited

Loading

mkuron commented Oct 7, 2019

jngrad commented Oct 11, 2019

bors bot commented Oct 11, 2019

jngrad commented Oct 11, 2019

bors bot commented Oct 11, 2019

jngrad commented Oct 11, 2019

bors bot commented Oct 11, 2019

Fix mpiio with stdlibc++ range checking #3234

Fix mpiio with stdlibc++ range checking #3234

Conversation

mkuron commented Oct 5, 2019

codecov bot commented Oct 5, 2019 • edited Loading

Codecov Report

hirschsn commented Oct 7, 2019

mkuron commented Oct 7, 2019

hirschsn commented Oct 7, 2019 • edited Loading

mkuron commented Oct 7, 2019

jngrad commented Oct 11, 2019

bors bot commented Oct 11, 2019

Canceled (will resume)

jngrad commented Oct 11, 2019

bors bot commented Oct 11, 2019

Canceled

jngrad commented Oct 11, 2019

bors bot commented Oct 11, 2019

Build succeeded

codecov bot commented Oct 5, 2019 •

edited

Loading

hirschsn commented Oct 7, 2019 •

edited

Loading