Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Sanitizer build with Ubuntu 20.04 #3636

Closed
wants to merge 18 commits into from

Conversation

RudolfWeeber
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Apr 6, 2020

Codecov Report

Merging #3636 into python will increase coverage by 0%.
The diff coverage is 84%.

Impacted file tree graph

@@           Coverage Diff            @@
##           python   #3636     +/-   ##
========================================
  Coverage      88%     88%             
========================================
  Files         524     523      -1     
  Lines       23471   22446   -1025     
========================================
- Hits        20658   19845    -813     
+ Misses       2813    2601    -212     
Impacted Files Coverage Δ
src/core/TabulatedPotential.hpp 100% <ø> (ø)
src/core/bond_error.hpp 0% <0%> (ø)
src/core/bonded_interactions/angle_cosine.hpp 100% <ø> (ø)
src/core/bonded_interactions/angle_cossquare.hpp 100% <ø> (ø)
src/core/bonded_interactions/angle_harmonic.hpp 100% <ø> (ø)
...re/bonded_interactions/bonded_interaction_data.cpp 91% <ø> (+<1%) ⬆️
src/core/electrostatics_magnetostatics/mmm1d.cpp 94% <ø> (-1%) ⬇️
src/core/grid_based_algorithms/halo.cpp 63% <ø> (+<1%) ⬆️
src/core/grid_based_algorithms/lb.cpp 96% <ø> (-1%) ⬇️
src/core/immersed_boundary/ImmersedBoundaries.hpp 100% <ø> (ø)
... and 198 more

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 8f105e3...0ac6628. Read the comment docs.

@KaiSzuttor
Copy link
Member

you can try locally by:
docker run -it docker.pkg.github.com/espressomd/docker/ubuntu-20.04:a1192b35590a1a474c55fe1e9a1e6c25758454ea bash

@jngrad
Copy link
Member

jngrad commented Apr 6, 2020

2382 /builds/espressomd/espresso/build/src/python/espressomd/collision_detection.cpp:10435:5: error: 'tp_print' is deprecated [-Werror,-Wdeprecated-declarations]
2383     0,
2384     ^
2385 /usr/include/python3.8/cpython/object.h:260:5: note: 'tp_print' has been explicitly marked deprecated here
2386     Py_DEPRECATED(3.8) int (*tp_print)(PyObject *, FILE *, int);
2387     ^
2388 /usr/include/python3.8/pyport.h:515:54: note: expanded from macro 'Py_DEPRECATED'
2389 #define Py_DEPRECATED(VERSION_UNUSED) __attribute__((__deprecated__))

To resolve that issue, it seems we need to either downgrade Cython 0.29.14 to 0.29.13, or add the compiler flag -Wno-deprecated-declaration (pandas-dev/pandas#30609 (comment))

@RudolfWeeber
Copy link
Contributor Author

RudolfWeeber commented Apr 6, 2020 via email

@RudolfWeeber
Copy link
Contributor Author

@fweik
Copy link
Contributor

fweik commented Apr 6, 2020

Yup, the offending line is /usr/include/boost/mpi/detail/binary_buffer_iprimitive.hpp:44, which calls front() on std::vector, which is undefined if the vector is empty.

@fweik
Copy link
Contributor

fweik commented Apr 6, 2020

It's also trivial to fix, which doesn't help because we don't control the code.

@RudolfWeeber
Copy link
Contributor Author

RudolfWeeber commented Apr 6, 2020 via email

@fweik
Copy link
Contributor

fweik commented Apr 6, 2020

I don't know. The sanitizer says this is a nullpointer dereference, so this might also segfault

@fweik
Copy link
Contributor

fweik commented Apr 6, 2020

Ah, wait a second. The buffer is passed into the class, maybe it can be fixed from the outside...

@fweik
Copy link
Contributor

fweik commented Apr 6, 2020

No, it can't. This was also introduced quite recently. We should really ship our own version of boost mpi.

.gitlab-ci.yml Outdated Show resolved Hide resolved
RudolfWeeber and others added 2 commits April 7, 2020 15:38
Co-Authored-By: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
.gitlab-ci.yml Outdated
Comment on lines 109 to 110
script:
- export myconfig=maxset with_cuda=false with_coverage=false with_static_analysis=true with_asan=true with_ubsan=true test_timeout=900 ASAN_OPTIONS="allocator_may_return_null=1" OMPI_CC=clang-9 OMPI_CXX=clang++-9 CC=clang-9 CXX=clang++-9 UBSAN_OPTIONS=suppressions=${CI_PROJECT_DIR}/maintainer/CI/ubsan.supp
Copy link
Member

@KaiSzuttor KaiSzuttor Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
script:
- export myconfig=maxset with_cuda=false with_coverage=false with_static_analysis=true with_asan=true with_ubsan=true test_timeout=900 ASAN_OPTIONS="allocator_may_return_null=1" OMPI_CC=clang-9 OMPI_CXX=clang++-9 CC=clang-9 CXX=clang++-9 UBSAN_OPTIONS=suppressions=${CI_PROJECT_DIR}/maintainer/CI/ubsan.supp
variables:
myconfig: 'maxset'
with_cuda: 'false'
with_coverage: 'false'
with_static_analysis: 'true'
with_asan: 'true'
with_ubsan: 'true'
test_timeout: '900'
ASAN_OPTIONS: 'allocator_may_return_null=1'
OMPI_CC: 'clang-9'
OMPI_CXX: 'clang++-9'
CC: 'clang-9'
CXX: 'clang++-9'
UBSAN_OPTIONS: 'suppressions=${CI_PROJECT_DIR}/maintainer/CI/ubsan.supp'
script:

Copy link
Member

@KaiSzuttor KaiSzuttor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use env vars

@jngrad
Copy link
Member

jngrad commented Apr 9, 2020

@RudolfWeeber This PR looks ready to me. What is still missing?
@KaiSzuttor We should delay your proposed env var changes to avoid conflicts with #3582, or only apply it to the Ubuntu 20.04 job.

@@ -22,6 +22,7 @@
#if defined(__CUDACC__)

#include <cuda.h>
#include "clang_thrust_workaround.cuh"
Copy link
Member

@jngrad jngrad Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file clang_thrust_workaround.cuh seems to be missing. Also, I just found out the issue was fixed in thrust 1.9.5 (I'm writing a report for them)

Suggested change
#include "clang_thrust_workaround.cuh"
#if THRUST_VERSION < 100905
#include "clang_thrust_workaround.cuh"
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my investigation: NVIDIA/thrust#1032 (comment); if CUDA 10.1 is shipped with thrust 1.9.5, we should install it on ICP machines and drop support for the combination Clang+CUDA10.0 is ESPResSo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RudolfWeeber sorry, I forgot that editing a message doesn't send an email. In my previous message, I meant to say that you probably forgot to commit file clang_thrust_workaround.cuh.

@fweik
Copy link
Contributor

fweik commented Apr 9, 2020

For the record: I think the workaround is a bad idea.

@jngrad
Copy link
Member

jngrad commented Apr 9, 2020

I'm also not sure if it can be done like this from the ESPResSo side. When I tinkered with that solution, I found the only way to get it to work was to apply it directly to the thrust code, but maybe I'm wrong.

@RudolfWeeber
Copy link
Contributor Author

It looks like the workaround is already present in the OSX container...

Let's make a decision about how to proceed in the ES meeting.

@jngrad
Copy link
Member

jngrad commented Apr 15, 2020

offline core team discussion: if Clang 8.0 can build with thrust 1.9.3 keep it, otherwise add thrust 1.9.5 as a submodule.

@RudolfWeeber
Copy link
Contributor Author

Core team discussion yielded that @jngrad will take over this PR.

  • Sanitizers work with Clang9.
  • At the ES meeting, it was decided to use Clang 8 and the latest GCC versions in the image and at the ICP rather than patching system headers to allow compilation with Cuda and more recent compilers
  • The thrust workaround mentioned above might have to be removed again when building with Clang-8. If it is still needed, we decided to ship a new enough thrust with Esprseso as git submodule for the time being.

I'll post Docker changes to the Ubuntu 20 container separately.

@kodiakhq kodiakhq bot closed this in #3684 Apr 25, 2020
kodiakhq bot added a commit that referenced this pull request Apr 25, 2020
Fixes #3627, fixes #3650, closes #3636

Description of changes:
- run Clang-Tidy, ASAN, UBSAN in the Ubuntu 20.04 image
- re-enable Clang-Tidy on CUDA code
- fix Clang-Tidy 9 warnings
- replace `boost::optional` by custom code in CUDA sources
- remove unused code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants