RFC: Run Rust tests using Valgrind and cargo-careful

[adamreichold]

Maybe an alternative or in addition to #2688 to employ Valgrind for detection of memory errors and separately cargo-careful to benefit from debug assertions in the standard library and additional consistency checks. As discussed in #2688, this also runs the test under Valgrind in release mode as many memory safety issues only manifest themselves in release builds.

I went for separate jobs instead of extending the build job matrix as that job is already rather complicated with multiple interleaving concerns. We might want more platform coverage for the careful job though. Similarly, we might want to include the Python tests in the Valgrind job, but that will most likely be more effort than setting an environment variable.

[adamreichold]

Hhhmmm, Valgrind has some issues with the DWARF5 debuginfo generated by LLVM 14 which prevents testing the macro crates using it for now. Not sure whether this is a deal breaker or not.

[davidhewitt]

I'm a bit split on which of these I prefer. I sort of wonder why not have both? Or do you think there's no need for both address sanitiser and valgrind?

Given the CI pipeline is getting heavier and heavier, I'm wondering whether we want to consider a lightweight push pipeline and then something like bors to run heavier checks just before merging? An alternative could be to have a weekly scheduled run for these kinds of santitizer checks (like pyo3-ffi-check), however it's definitely better to catch issues before merge if possible.

[messense]

+1 for bors, I've had some good experience with it recently: rust-cross/rust-musl-cross#60

[adamreichold]

I'm a bit split on which of these I prefer. I sort of wonder why not have both? Or do you think there's no need for both address sanitiser and valgrind?

Given the CI pipeline is getting heavier and heavier, I'm wondering whether we want to consider a lightweight push pipeline and then something like bors to run heavier checks just before merging? An alternative could be to have a weekly scheduled run for these kinds of santitizer checks (like pyo3-ffi-check), however it's definitely better to catch issues before merge if possible.

I do agree with using something like bors and splitting up the CI workflows, however I'd say this should be a separate endeavour after merging either or both of ASAN and Valgrind.

As for the two: I'd suggest doing only one of them as the overlap is pretty large and doing both wastes energy, both electrical as well as maintainer energy.

I think going for ASAN as in #2699 is the variant that implies less effort for us and is measurably faster to run (and that PR can be merged AFAIK after running the tests in release mode while keeping the debug assertions on). Since it includes -Zbuild-std it also includes some of the coverage that cargo-careful provides.

So using Valgrind adds coverage for non-instrumented code, e.g. the Python libraries which we do not rebuild with sanitizer support. cargo-careful adds "some nightly-only flags that tell rustc to insert extra run-time checks to guard some unsafe operations against UB" in addition to the debug assertions enabled by -Zbuild-std.

So the question is whether those two additions are worth it AFAIU. For the coverage of non-instrumented code, I have to admit to not having an example of where we miss a defect due to this. Also when using ASAN in C/C++ projects, one also does not "rebuild the world" with ASAN instrumentation enabled. (One does do so however when using MSAN, so the point is not completely moot AFAIK.)

As for the extra flags set by cargo-careful, those are listed in https://github.com/RalfJung/cargo-careful/blob/master/src/main.rs#L12 and we could just integrate them into the address-sanitizer Nox session in #2699. We should commit to keeping up-to-date with those flags (probably by periodically checking what cargo-careful does).

So all in all, while it does seem to be the more comprehensive option, I admit that the case for Valgrind and cargo-careful is relatively thin compared to adding those flags to #2699 which would also keep our CI more lightweight.

[adamreichold]

Not sure what is up with the trybuild tests now...

[adamreichold]

Not sure what is up with the trybuild tests now...

Pinning to Rust 1.61 implied that the compiler output changed significantly. The same could probably happen with the nightly toolchain, so I added env: TRYBUILD: overwrite to both jobs.

[davidhewitt]

Ah, that makes sense 👍

This looks good to me, will merge this and include it in the testing for the 0.17.3 release.