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

odb: protecting dbModule::makeUniqueDbModule()::name_id_map with a lock #6245

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

Conversation

LucasYuki
Copy link
Contributor

Protects the static variable name_id_map from the dbModule::makeUniqueDbModule method with a lock (issue #5981)

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@@ -559,11 +560,15 @@ dbModule* dbModule::makeUniqueDbModule(const char* cell_name,
std::string module_name = orig_cell_name + '_' + std::string(inst_name);
do {
std::string full_name = module_name;

name_id_map_mutex.lock();
Copy link
Member

Choose a reason for hiding this comment

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

Its better to use std::unique_lock than manual lock and unlock calls (eg exception safety).

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@QuantamHD
Copy link
Collaborator

QuantamHD commented Nov 26, 2024

This does not seem like an acceptable solution. If you have multiple odb's won't calls to makeUniqueDbModule interfere with each other?

@maliberty

@maliberty
Copy link
Member

maliberty commented Nov 26, 2024

Interfere in the sense that the numbering will depend on the order of the calls. It won't be a thread safety issue but it could cause some non-determinism in the names.

@maliberty
Copy link
Member

I think the easiest thing to do is move name_id_map to be a non-persistent member of dbBlock. No lock is then needed and the extra memory will disappear on close.

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.

3 participants