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

Add thread sanitizer test in CI #4799

Merged
merged 76 commits into from
Aug 10, 2024

Conversation

lucafedeli88
Copy link
Member

@lucafedeli88 lucafedeli88 commented Mar 22, 2024

This PR adds a thread sanitizer test in CI.

Specifically:

  • a new clang_sanitizers.yml workflow file is created
  • the pre-existing UB sanitizers test is moved from ubuntu.yml to clang_sanitizers.yml workflow
  • a thread sanitizer test is added. This test needs clang-17. Therefore, for this test, we use a Ubuntu 23.10 container and we install clang-17 with the instructions added to a new dependencies file (clang17.sh).
  • the test consists in compiling WarpX with the thread sanitizer and in running the laser_acceleration example in 1D,2D,3D, and RZ, as well as the embedded_circle example in 2D. In all cases the serialization of initial conditions is deactivated, although it does not seem to affect the thread sanitizer. Note that WarpX is compiled in double precision, since the embedded_circle example is not tested in single precision and seems to be numerically unstable unless double precision is used. Note also that WarpX is compiled without EB support for the 4 laser_acceleration tests because there seems to be bugs when WarpX with EB support is used to run laser_acceleration in 2D (this is currently under investigation).
  • A fix proposed by @WeiqunZhang in Skip empty particle tiles in operator++ to avoid race condition in constructor. AMReX-Codes/amrex#3951 is implemented to fix a race condition in ParticleBoundaryBuffer.cpp
  • Following @RemiLehe 's suggestion, address and pointer-compare sanitizer checks have been added to UB sanitizer test.
  • The PR changes where current_correction is set to true (the default behavior) to fix an issue found by the UB sanitizer.

The thread sanitizer test uses libarcher, which should theoretically avoid false positives with OpenMP (see https://github.com/llvm/llvm-project/tree/main/openmp/tools/archer)

Some additional comments:

  • export PMIX_MCA_gds=hash I am not sure why this setting is required to be honest (I am still trying to understand). However, if I run without this setting I get an error message like: The gds/shmem component attempted to attach to a shared-memory segment at a particular base address, but was given a different one, followed by a suggestion to disable the gds/shmem component. It seems to be a relatively common issue.
  • export TSAN_OPTIONS='ignore_noninstrumented_modules=1' should avoid false positives with libraries not compiled with the thread sanitizer. Archer documentation suggests using this setting.
  • export ARCHER_OPTIONS="verbose=1" makes WarpX print the message:
    Archer detected OpenMP application with TSan, supplying OpenMP synchronization semantics, which confirms that libarcher is in use.
  • export OMPI_ALLOW_RUN_AS_ROOT=1 and export OMPI_ALLOW_RUN_AS_ROOT_CONFIRM=1 are needed because we are running the test in a docker container.
  • export "ASAN_OPTIONS=detect_leaks=0" is required to disable leak checks: MPI implementations leak memory

@lucafedeli88 lucafedeli88 added component: tests Tests and CI cleaning Clean code, improve readability labels Mar 22, 2024
@lucafedeli88 lucafedeli88 changed the title [WIP] Add thread sanitizer [WIP] Add thread sanitizer in CI Mar 22, 2024
@lucafedeli88 lucafedeli88 changed the title [WIP] Add thread sanitizer in CI [WIP] Add thread sanitizer test in CI Mar 22, 2024
@lucafedeli88
Copy link
Member Author

Ping, @ax3l and @RemiLehe ;-) !

Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Great work, thank you! :)

We can run this over more of the fast tests once CTest is merged #5068

@ax3l ax3l merged commit 75e2fd9 into ECP-WarpX:development Aug 10, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: affects latest release Bug also exists in latest release version bug Something isn't working cleaning Clean code, improve readability component: tests Tests and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants