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

needs CI test for memory leaks and race conditions #154

Closed
spacejam opened this issue Aug 23, 2017 · 5 comments · Fixed by #591
Closed

needs CI test for memory leaks and race conditions #154

spacejam opened this issue Aug 23, 2017 · 5 comments · Fixed by #591
Assignees
Labels

Comments

@spacejam
Copy link
Contributor

#142 could have been caught before release by having travis run tests with lsan. here's how I've solved this for my lock-free tree:

https://github.com/spacejam/rsdb/blob/5aec53905ef58bad8650697b4550d6d47836832f/.travis.yml#L27

https://github.com/spacejam/rsdb/blob/master/hack/leak_detector

I have the rust version pinned (there are currently issues with some sanitizers post nightly-07.06) and I run it with sudo so the sanitizer can run.

Let me know if you'd like me to adapt a similar approach for this project!

@jeehoonkang
Copy link
Contributor

I definitely want this! We should ask other maintainers, but I am 100% for for CI testing.

Can it be run in Travis CI for free? I am just worried that we need sudo permission here.

@ghost
Copy link

ghost commented Aug 25, 2017

This is great! Do you get any false positives with sanitizers? When I last tried them, many were firing within Rust's runtime, usually coming up when using std::sync::mpsc or when spawning threads.

Can it be run in Travis CI for free? I am just worried that we need sudo permission here.

Yeah, Travis is free for open source projects. Regarding the environment, you can choose between running your script in a sudo-less container or within a sudo-enabled VM. The machine has 2 physical cores and (I believe) dozens of logical cores.

@spacejam
Copy link
Contributor Author

Yeah, tsan will false positive on memory fences, and I think it's false positiving on lazy_static since it seems not to follow implicit happens-before relationships, just explicit uses of atomic operations on particular memory locations, which can be frustrating. On the other hand, I think it's a super powerful tool for finding bugs, and there could be a lot of value in making sure the main components of a library avoid these known false positives, so that users of that library can take advantage of the power of the tool in most cases. FWIW I've had no issues with false-positives in coco so far, despite the one memory fence in Thread::set_pinned.

@spacejam
Copy link
Contributor Author

FYI I'm starting to peek at the complexity of adding LLVM's __attribute__((no_sanitize("thread"))) attribute to rust, since the utility of tsan is so great, and the false positives can easily make the tool unusable!

@spacejam
Copy link
Contributor Author

spacejam commented Sep 1, 2017

setting this environment variable allows suppression of specific known-safe code:
export TSAN_OPTIONS="suppressions=blacklist.txt"
where the contents of blacklist.txt follow the syntax as described https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions

So, this should provide us with the best of both worlds I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants