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

ThreadSanitizer for iceoryx #692

Open
1 of 4 tasks
elBoberido opened this issue Apr 8, 2021 · 7 comments · Fixed by #1824
Open
1 of 4 tasks

ThreadSanitizer for iceoryx #692

elBoberido opened this issue Apr 8, 2021 · 7 comments · Fixed by #1824
Assignees
Labels
enhancement New feature good first issue Good for newcomers
Milestone

Comments

@elBoberido
Copy link
Member

elBoberido commented Apr 8, 2021

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

  • 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 elBoberido added enhancement New feature good first issue Good for newcomers labels Apr 8, 2021
@elBoberido elBoberido self-assigned this Jan 5, 2022
@elBoberido
Copy link
Member Author

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
Copy link
Member Author

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

@ibrhmkuru ibrhmkuru mentioned this issue Jan 4, 2023
21 tasks
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 10, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 12, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 12, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 12, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 12, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 12, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 18, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 19, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 19, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Jan 19, 2023
@elBoberido elBoberido linked a pull request Jan 19, 2023 that will close this issue
21 tasks
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 16, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 16, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 17, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 17, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 17, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 17, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 17, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 17, 2023
ibrhmkuru added a commit to ApexAI/iceoryx that referenced this issue Feb 17, 2023
dkroenke added a commit that referenced this issue Feb 27, 2023
…b-to-pr-ci

iox-#692 Move tsan jobs into build-test.yml
@elBoberido elBoberido added this to the Medium prio milestone Sep 21, 2023
@elBoberido
Copy link
Member Author

@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
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link
Contributor

FerdinandSpitzschnueffler commented Nov 15, 2023

@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
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature good first issue Good for newcomers
Projects
Status: To do
Development

Successfully merging a pull request may close this issue.

2 participants