Closed Bug 1671691 Opened 4 years ago Closed 3 years ago

TSan: Properly Instrument Rust's stdlib

Categories

(Core :: Sanitizers, task)

task

Tracking

()

RESOLVED FIXED
85 Branch
Tracking Status
firefox85 --- fixed

People

(Reporter: Gankra, Assigned: Gankra)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Anything that uses a Rust Mutex or Arc currently looks like a datarace to TSan. Apparently there's some flags we should try building rust with that might Just Work?

It's possible we just need to follow these steps: https://github.com/japaric/rust-san

most notably, compiling our crates with RUSTFLAGS="-Z sanitizer=thread" (unstable nightly option, which is fine for testing)

Also possible we need to use cargo's -Z build-std feature to recompile std?

Assignee: nobody → a.beingessner

Tabling this for the next week, in case https://github.com/rust-lang/rust/issues/78533 bears fruit and greatly simplifies this.

Simulacrum is suddenly less optimistic about what this involves, so back to getting -Z build-std working (currently running into issues resolving the libbacktrace dep rust uses).

Ok so, it seems mostly just that the rust-src component doesn't include dependencies (like backtrace). Normally this would be fine because you can just grab them from crates.io... except we have shadowed crates.io with our vendor. So if the stdlib has any deps that aren't in our vendor, build fails.

Considering how to ensure we have deps.

Note: there are vendor artifacts for the whole rustc build -- https://static.rust-lang.org/dist/2020-03-25/rustc-nightly-src.tar.xz

However these are ~100mb because they include llvm, as well as many other things that aren't needed for the stdlib. Currently there's no simple way to grab these deps. We could hand-include the vendored crates, but that will probably be a nasty maintenance burden.

Update: I am currently investigating whether it would be reasonable to make cargo vendor -Zbuild-std add std's deps to a vendor: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/cargo.20vendor.20-Zbuild-std

NB: most relevant upstream discussion is in https://github.com/rust-lang/wg-cargo-std-aware/issues/23

I have a hacked up build running, and am now filing bugs / adding suppression removals.

This is behind a --vendor-std flag, which also disables the update
vendor does to allow std vendoring to be run separately from a vendor
bump.

Also handles setting -Zsanitizer=thread.

Some WIP debug gunk in there too.

Depends on D95873

Attached patches are WIP / proof-of-concept. Pushing them up so I can have them available as context for discussion of the next step with peers.

Ok so the attached patches are enough to create a green tsan try build that instruments the rust stdlib. These patches are specifically limited because they require a local invocation of ./mach vendor rust --vendor-std --build-peers-said-large-imports-were-ok, with the exact nightly CI uses installed.

The added vendored libraries are currently about 10MB.

The main challenge here is the interaction between -Zbuild-std and our vendored environment. The rust-src component does not include all the dependencies to build std (or more specifically, libtest, which includes std), and relies on pulling them from crates.io. Because we're a vendored environment, we intercept these crates.io lookups and they fail.

So: we need to somehow get the std dependencies into our vendored environment. There are a few options:

Option A - Upstream fixes

This is "the right way" which I expect we will move to longterm. Specifically, the proposed solution in this cargo issue, which would add a minimal libtest vendor to the rust-src component, and teach -Zbuild-std to use it.

The main knock against this is that it may take a while. I'd have to make changes to cargo -Zbuild-std, cargo vendor, and the code that builds the rust-src artifact, have all the nightlies update enough times to have everything see everything else, and then update the rust nightly for firefox's tsan CI.

Option B - Make mach vendor rust require the tsan nightly

A bit janky, but we could just make developers who want to vendor rust keep the tsan nightly ci toolchain installed, so that they can run my --vendor-std mode. This would also bloat the tree with 10MB of new vendored code, for one build mode that only CI really uses.

Option C - Make CI run vendor rust

--vendor-std currently doesn't update gkrust, so it can be run independently to just pull in the std deps. This means, in theory, CI could just run it to pull down the deps just-in-time. The sloppiest version of this would require every tsan try run to do this. A more refined version would somehow cache the result.

Option D - Add the entire rustc vendor artifact to CI

There are non-rustup artifacts which include the entire vendor for building rustc. This is about 100MB (includes llvm), but it could be easily cached with the other artifacts we download in CI. That said, I'm not totally sure how to merge its vendor with ours. But it might be possible?


I've been investigating C as the least messy MVP. I'm not sure if we want to commit the resources required to complete A (and if we're willing to wait that long). I assume B isn't politically viable. I'm uncertain D will work.

Any thoughts and feedback would be greatly appreciated.

It seems to me D is the easiest and best option in general, after A. Ideally, you'd get the source-code artifact from repack_rust.py.

I was convinced to give A a try, and, oops it was way easier than I thought.

https://github.com/rust-lang/rust/pull/78790 is approved and in the bors queue to land (might bounce for infra access-rights reasons).

https://github.com/rust-lang/cargo/pull/8834 is partially approved, waiting on a second opinion.

The two can land out of sync, and -Zbuild-std will just start working as soon as there's a nightly with both patches included.

Attachment #9185748 - Attachment is obsolete: true
Attachment #9185743 - Attachment is obsolete: true
Attachment #9185741 - Attachment is obsolete: true
Attachment #9185742 - Attachment is obsolete: true
  • Bumps the tsan toolchain to rust-nightly-2020-11-14 that has my patches to make -Zbuild-std work in vendored environments:

  • Passes -Zbuild-std to cargo when MOZ_TSAN is defined (mk_add_options --enable-thread-sanitizer)

  • Removes generic Rust supressions and adds much more specific ones

    • One presumed upstream false positive from tsan not understanding the code
    • One actual upstream bug tsan found (yay!)
    • One new real issue uncovered
    • One issue that probably already existed intermittently but I happened to hit
Pushed by abeingessner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6331313cdb48
Enable Rust stdlib instrumentation. r=decoder
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: