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

Add data race detector plugin #130

Merged
merged 2 commits into from
Aug 9, 2024
Merged

Conversation

AlphaDaze
Copy link
Contributor

@AlphaDaze AlphaDaze commented Jul 31, 2024

This PR adds a data race detector plugin. This plugin detects possible data races within the underlying program along with information that can be utilised for debugging.

The README provides further context for the plugin and its usage.

Compile with make datarace_ft and run with mambo_datarace_ft. The plugin includes two algorithms which can be compiled separately: fasttrack and djit. Both are happens-before algorithms with fasttrack being an improvement over djit with both memory and runtime.

All comments and suggestions are welcome!

Sample output

--- FORK: Thread 1029188 created by 0
--- LOCK: Thread 1029188 locked 0x7fff22128b10
--- UNLOCK: Thread 1029188 unlocked 0x7fff22128b10
--- FORK: Thread 1029189 created by 1029188
--- LOCK: Thread 1029188 locked 0x7fff22128b10
--- UNLOCK: Thread 1029188 unlocked 0x7fff22128b10
--- FORK: Thread 1029223 created by 1029188
--- JOIN: Thread 1029189 joining 1029188
!!! READ RACE: Possible read race - 0x41104c by thread 1029223
>>>> at 0x40074c: /home/user/Documents/git/tests/hg04_race (0x400744) (th10)
!!! WRITE RACE: Possible write race - 0x41104c by thread 1029223
>>>> at 0x40075c: /home/user/Documents/git/tests/hg04_race (0x400744) (th10)
--- JOIN: Thread 1029223 joining 1029188
--- LOCK: Thread 1029188 locked 0x7fff22128ab0
--- UNLOCK: Thread 1029188 unlocked 0x7fff22128ab0
We're done; exiting with status: 0

Copy link
Collaborator

@IgWod IgWod left a comment

Choose a reason for hiding this comment

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

Hi @AlphaDaze,

Thank you for your contribution. It looks good, however there are few things I'd like to be addressed before merging it. Please see attached comments.

Also, a major point, please remove any commented out code or add explanation why it's commend out and how it can be used in the future. We don't need any dead code in the repo :).

Let me know if something is unclear.

Many thanks,
Igor

dbm.h Outdated Show resolved Hide resolved
plugins/datarace/README.md Outdated Show resolved Hide resolved
plugins/datarace/README.md Outdated Show resolved Hide resolved
plugins/datarace/README.md Outdated Show resolved Hide resolved
plugins/datarace/README.md Outdated Show resolved Hide resolved
plugins/datarace/datarace.c Outdated Show resolved Hide resolved
plugins/datarace/datarace.c Outdated Show resolved Hide resolved
plugins/datarace/datarace.c Outdated Show resolved Hide resolved
plugins/datarace/datarace.h Outdated Show resolved Hide resolved
plugins/datarace/datarace.h Outdated Show resolved Hide resolved
@AlphaDaze AlphaDaze requested a review from IgWod August 4, 2024 15:35
Copy link
Collaborator

@IgWod IgWod left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments. I found two more small issues, but I'm happy otherwise. Please either remove the commented out code I highlighted or add TODO/explanation why it's commented out.

The other things is commit messages. Could you please clean it up? Ideally we would like to have two commits. The first commit for changes to the core and then the second one adding the plugin. Shouldn't be too difficult to achieve with git rebase -i and some amending.

Once that's done I'm happy with the PR, but I'll ask @jkressel to have a look as well, and once he approves it we can merge it.

plugins/datarace/datarace.c Outdated Show resolved Hide resolved
plugins/datarace/datarace.c Outdated Show resolved Hide resolved
@IgWod IgWod requested a review from jkressel August 6, 2024 14:50
@AlphaDaze
Copy link
Contributor Author

Sounds good! Thanks for taking a look at it. I have rebased and squashed the relevant commits together as requested.

If there is any other changes (however small), please do let me know.

@josh-nook
Copy link
Contributor

josh-nook commented Aug 9, 2024

Looks good to me (I'm a new intern). The markdown reads well

@IgWod
Copy link
Collaborator

IgWod commented Aug 9, 2024

Looks good to me! I'll ask @jkressel to have a look now.

Copy link
Collaborator

@jkressel jkressel left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'm happy to merge.

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.

4 participants