ThreadSanitizer for iceoryx

eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication

https://iceoryx.io

Apache License 2.0

1.57k stars 373 forks source link

ThreadSanitizer for iceoryx #692

Open elBoberido opened 3 years ago

elBoberido commented 3 years ago

Brief feature description

Integrate the ThreadSanitizer in iceoryx to detect data races.

Detailed information

It seems Mozilla had good experience finding data races with the ThreadSanitizer https://hacks.mozilla.org/2021/04/eliminating-data-races-in-firefox-a-technical-report/.

I suggest to also integrate it into iceoryx since we are heavily using multi-threaded code.

Tasks

[x] add ThreadSanitizer to iceoryx by extending the current sanitizers in IceoryxPlatform.cmake
[ ] fix the low hanging fruits
[ ] add the complex cases to a suppression list and ask maintainer which one should be fixed with a high prio
[ ] CI runs with the ThreadSanitizer to prevent adding new data races
- investigate the reasons for the massive increase in the test time
- run the tests in parallel jobs, one for hoofs_moduletiontests, posh_moduletests and posh_integrationtests

elBoberido commented 2 years ago

It seems we cannot run tsan and asan in together and need separate builds for each sanitizer. See also http://www.stablecoder.ca/2018/02/01/analyzer-build-types.html

@dkroenke what do you think, shall we add more CI builds or find another solution?

elBoberido commented 2 years ago

Running the hoofs_moduletests with tsan results in 22 warnings. Most of them in Timer_test and TriggerQueue_test. I did not yet investigate if those are real errors or false positives

cc @elfenpiff @MatthiasKillat @budrus

elBoberido commented 7 months ago

@FerdinandSpitzschnueffler you already did suppress some tsan warnings. May you share the suppression syntax you used to suppress the warnings in the code rather than the suppression file?

FerdinandSpitzschnueffler commented 7 months ago

@FerdinandSpitzschnueffler you already did suppress some tsan warnings. May you share the suppression syntax you used to suppress the warnings in the code rather than the suppression file?

@elBoberido For a particular function one can use __attribute__((no_sanitize("thread"))). Then, the instrumentation of that function by the thread sanitizer is disabled. Does this help?

elBoberido commented 7 months ago

@FerdinandSpitzschnueffler thanks. I guess that will result in warnings on windows so we might end up with

#if defined __has_attribute
#if __has_attribute (no_sanitize)
__attribute__ ((no_sanitize("thread")))
#endif
#endif

... maybe wrapped in a macro.

So this is then on function level and not file level or for a single expression?

FerdinandSpitzschnueffler commented 7 months ago

@elBoberido argh, yes on windows this will probably cause warnings. According to several documentations (e.g. https://releases.llvm.org/15.0.0/tools/clang/docs/AttributeReference.html#no-sanitize or https://clang.llvm.org/docs/ThreadSanitizer.html) the suppression would be on a function level. If I remember correctly (no guarantees), one needs an ignore list for file level suppression, but this can lead to false positives (since the suppression syntax I mentioned before does instrument the code but ignores the warnings, whereas the ignore list really disables the instrumentation of the code). I'm not aware of any single expression suppression. If the suppression is needed on file/expression level, I'd also have to dig deeper.

elBoberido commented 7 months ago

@FerdinandSpitzschnueffler thanks for for the detailed information. I think a suppression on function level is sufficient. If it is really needed one can move single expressions to functions.