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

Run tests with address sanitizer on CI #2688

Closed
wants to merge 2 commits into from
Closed

Conversation

messense
Copy link
Member

No description provided.

@adamreichold
Copy link
Member

For rust-numpy, I eventually switched over to Valgrind's memcheck instead. It is much slower, but it also has better coverage as it does not rely on compiler-generated instrumentation, i.e. it will also cover the CPython code we interact with in addition to our own code. (I suspect that there are bugs which are only apparent when both sides are considered, e.g. when the allocation happens on the CPython side does not get the necessary ASAN metadata attached.)

@messense
Copy link
Member Author

Valgrind does not finish in cargo test debug mode, while it finished with --release, it's very slow and did not catch the memory issue found in #2687.

@adamreichold
Copy link
Member

adamreichold commented Oct 16, 2022

while it finished with --release, it's very slow and did not catch the memory issue found in #2687.

We should definitely run the sanitizer tests in release mode as a lot of issues in FFI interactions only come to light with optimizations applied as otherwise stack space is not reused as aggressively for example.

As for why it did not uncover #2687, I think that is due no actual invalid memory accesses happening. Or put differently, ASAN did not catch that either. I think this is due to building the standard library with debug assertions enabled which asserts non-null pointers for from_raw_parts, i.e. this is the cargo-careful, not the ASAN part.

I think it would be useful to do both: Always have a pass with -Zbuild-std in all CI matrix elements and a separate job that uses ASAN or Valgrind.

(A benefit of Valgrind for rust-numpy was that it was able to check the doctests in addition to the unit tests. The doctests failed to compile using the usual ASAN-via-RUSTFLAGS approach.)

@messense
Copy link
Member Author

Make sense, thanks for the detailed explanation @adamreichold

@adamreichold
Copy link
Member

For rust-numpy, I decided for separate CI jobs using Valgrind and cargo-careful which appears to be the best compromise between coverage and simplicity to me at the moment: PyO3/rust-numpy#355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants