odb: protecting LefParser and DefParser namespaces #6475
Conversation
Signed-off-by: Lucas Yuki Imamura <[email protected]>
Signed-off-by: Lucas Yuki Imamura <[email protected]>
Signed-off-by: Lucas Yuki Imamura <[email protected]>
Signed-off-by: Lucas Yuki Imamura <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/odb/src/defin/defin.cpp
Outdated
| std::mutex defin::_def_mutex; | ||
|
|
||
| defin::defin(dbDatabase* db, utl::Logger* logger, MODE mode) | ||
| { | ||
| std::lock_guard<std::mutex> lock(_def_mutex); | ||
| _reader = new definReader(db, logger, mode); | ||
| } | ||
|
|
||
| defin::~defin() | ||
| { | ||
| std::lock_guard<std::mutex> lock(_def_mutex); |
There was a problem hiding this comment.
I'm not clear the locking is doing anything here. It kind of looks like this is trying to protect two defin's from running at the same time on the same database, but what we're really concerned about is two different defin's running on two different databases.
What are these locks here to do?
There was a problem hiding this comment.
I think he has just locked every method. The key ones are ::create*/replace* methods which will block simultaneous parsing.
@LucasYuki I think some of these do nothing more than set a manipulate local variables and not the db and therefore do not need locks (eg ctor, dtor, skip). Please analyze it a bit more.
Signed-off-by: Lucas Yuki Imamura <[email protected]>
Signed-off-by: Lucas Yuki Imamura <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@QuantamHD are your concerns resolved? |
|
I am, but could we add a comment to the mutexes that explains why they exist. Specifically mentioning that the defin and lefin parsers rely on static vars. |
|
@LucasYuki please add the comment to this PR. |
Signed-off-by: Lucas Yuki Imamura <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
Protects the global and static variables in the LefParser and DefParser namespaces (issue #5981)
Changes: