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

Enable sanitizers for extended CI host build #1114

Merged
merged 12 commits into from
Jun 21, 2024

Conversation

BenWibking
Copy link
Collaborator

@BenWibking BenWibking commented Jun 14, 2024

PR Summary

Enable sanitizers (e.g., AddressSanitizer, UndefinedBehaviorSanitizer, etc.) for the extended CI host build. It is not enabled for the CUDA build, since CUDA does not support sanitizers.

Memory leak detection is currently disabled (does not work with OpenMPI, several unit tests have memory leaks).

Depends on:

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@BenWibking BenWibking marked this pull request as ready for review June 14, 2024 21:52
@BenWibking
Copy link
Collaborator Author

Tests are expected to fail until the issues are fixed in other PRs: https://github.com/parthenon-hpc-lab/parthenon/actions/runs/9522754529/job/26252926684

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Will be nice to have this automatically. Hopefully will catch bugs.

Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

LGTM, I agree that this will help us catch some subtler issues before they can enter develop.

Comment on lines +47 to +60
- name: Set vars based on matrix
id: cmake-vars
run: |
if ${{ matrix.device == 'host' }}; then
echo "enable_asan=ON" >> $GITHUB_OUTPUT
else
echo "enable_asan=OFF" >> $GITHUB_OUTPUT
fi

- name: Configure
run: |
cmake -B build \
-DCMAKE_BUILD_TYPE=Release \
-DENABLE_ASAN=${{ steps.cmake-vars.outputs.enable_asan }} \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that a common pattern?

Might sth like

-DENABLE_ASAN=${${{ matrix.device == 'host' }}^^} \

work, too?

Or if that one line logic doesn't work using an env var:

env:
  ENABLE_ASAN: ${{ matrix.device == 'host' }}

with (the ^^ to uppercase the lowercase result in the env var):

-DENABLE_ASAN=${ENABLE_ASAN^^} \

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bash outputs 1 for true and 0 for false, so I don't think that works, unless CMake translates 1 to ON?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should
"True if the constant is 1, ON, YES, TRUE, Y, or a non-zero number (including floating point numbers)." from https://cmake.org/cmake/help/latest/command/if.html

But I think GitHub actions only returns (lowercase) true or false for expressions so that's why we need to convert it to uppercase.

@BenWibking
Copy link
Collaborator Author

Probably need to disable LeakSanitizer for MPI runs due to the memory leaks in OpenMPI we can't do anything about...

@BenWibking
Copy link
Collaborator Author

AddressSanitizer and UndefinedBehaviorSanitizer run without workings, but some tests have memory leaks that cause LeakDetector to fail.

@BenWibking
Copy link
Collaborator Author

Rather than try to chase down every memory leak in the unit tests, I've just disabled memory leak detection entirely.

@Yurlungur
Copy link
Collaborator

Rather than try to chase down every memory leak in the unit tests, I've just disabled memory leak detection entirely.

I think that's the right thing but we should leave the memory leak test in place in the cmake so we can trigger it locally. Because someone should start trying to track these down over time. :)

@BenWibking BenWibking disabled auto-merge June 21, 2024 00:23
@BenWibking BenWibking merged commit 954c036 into develop Jun 21, 2024
50 checks passed
@BenWibking BenWibking deleted the BenWibking/enable-asan-ci branch June 21, 2024 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants