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

V3LockGuard: Add constructor for adopting already locked mutex. #4476

Merged
merged 8 commits into from
Sep 13, 2023

Conversation

kamilrakoczy
Copy link
Contributor

Pre-PR to: #4228

Add constructor for adopting already locked mutex.

Copy link
Member

@wsnyder wsnyder left a comment

Choose a reason for hiding this comment

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

I don't see where this is used, I'm ok with the change, but would prefer not to add dead code that hurts internal coverage. Can you fold this into whatever needs it - or better add a self test case that uses it. Thanks

@kamilrakoczy
Copy link
Contributor Author

I don't see where this is used, I'm ok with the change, but would prefer not to add dead code that hurts internal coverage. Can you fold this into whatever needs it - or better add a self test case that uses it. Thanks

Sure, I've added usage of it, so now this PR requires: #4477

I think I can't really add selfTest for adopt_lock_t as it would require check if mutex is locked, but std::mutex doesn't have such feature. We could try to use try_lock, but it have undefined behavior when mutex is locked by the same thread that is using try_lock: https://en.cppreference.com/w/cpp/thread/mutex/try_lock. We could also add extra field to V3Mutex class that stores thread id of thread (if mutex is locked) but I think it is not worth just for the test.

@kamilrakoczy
Copy link
Contributor Author

This PR should be ready for review and merge.

Mariusz Glebocki and others added 6 commits September 13, 2023 14:17
Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
src/V3ThreadPool.cpp Outdated Show resolved Hide resolved
src/V3ThreadSafety.h Outdated Show resolved Hide resolved
src/V3ThreadPool.cpp Show resolved Hide resolved
kamilrakoczy and others added 2 commits September 13, 2023 15:53
Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
@wsnyder wsnyder merged commit 9fe459c into verilator:master Sep 13, 2023
1 check passed
@mglb mglb deleted the kr/adopt_lock branch September 21, 2023 10:08
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