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

New locker mechanism and small data race fixes #1166

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mkornaukhov03
Copy link
Contributor

@mkornaukhov03 mkornaukhov03 commented Nov 26, 2024

I have tried to run transpiler with TSAN on simple program and found lots of data races. This PR fixes some of them, but not all.

  • Introduce new locker that is implemented as 3-state futex-based mutex
  • Add some atomic variables instead of non-atomic
  • Clone some nodes instead of copying references

@mkornaukhov03 mkornaukhov03 force-pushed the mkornaukhov03/tsan-compiler branch from 4d08728 to 7478704 Compare December 24, 2024 08:19
@mkornaukhov03 mkornaukhov03 force-pushed the mkornaukhov03/tsan-compiler branch from cd7d4ed to 725f1a0 Compare December 26, 2024 13:38
@mkornaukhov03 mkornaukhov03 force-pushed the mkornaukhov03/tsan-compiler branch from 1dfc11e to 650367c Compare December 27, 2024 13:16
@mkornaukhov03 mkornaukhov03 marked this pull request as ready for review December 27, 2024 13:29
@mkornaukhov03 mkornaukhov03 changed the title tsan in compiler New locker mechanism and small data race fixes Dec 27, 2024
@mkornaukhov03 mkornaukhov03 self-assigned this Jan 14, 2025
}

copyable_atomic_integral& operator=(copyable_atomic_integral other) {
*this = other.load();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like risky. Does it potentially lead to recursive invocation of assign operator?

I suppose, will be better to rewrite

if (this != &other) { 
   this->store(other.load());
}
return *this;

Correct me, if I'm wrong.

std::atomic<T>(other.load()) {
}

copyable_atomic_integral& operator=(copyable_atomic_integral other) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, will be better to pass by reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is intentional. It handles self assignment problem properly: currently in 56 line will be used constructor number 2. In case of your suggestions there will be problem with self assignment because of another order of function (constructor in this case) overloading.

@@ -177,11 +178,11 @@ class CompilerCore {
}

void set_functions_txt_parsed() {
is_functions_txt_parsed = true;
is_functions_txt_parsed.store(true, std::memory_order_seq_cst);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use certain memory order option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used for synchronization, where are some while(!get_functions_txt_parsed()) loops. I'm not sure that it isn't synchronized with other atomic variables.

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

Successfully merging this pull request may close these issues.

2 participants