greg7mdp / parallel-hashmap

A family of header-only, very fast and memory-friendly hashmap and btree containers.
https://greg7mdp.github.io/parallel-hashmap/
Apache License 2.0
2.53k stars 239 forks source link

Clang's "thread-safety-analysis" warnings #196

Closed phallot closed 1 year ago

phallot commented 1 year ago

Hello,

When building 1.3.11 (and below) with Clang-15's "thread-safety-analysis" option, there are some warnings reported.
At a quick glance, they look like false positives to me, but it might be worth a look? Also maybe there is a way to satisfy/silence them?

Thanks.

In file included from external/parallel-hashmap/parallel_hashmap/phmap.h:121:
external/parallel-hashmap/parallel_hashmap/phmap_base.h:5046:48: error: mutex 'this' is still held at the end of function [-Werror,-Wthread-safety-analysis]
        void lock()            { this->Lock(); }
                                               ^
external/parallel-hashmap/parallel_hashmap/phmap_base.h:5046:40: note: mutex acquired here
        void lock()            { this->Lock(); }
                                       ^
external/parallel-hashmap/parallel_hashmap/phmap_base.h:5047:40: error: releasing mutex 'this' that was not held [-Werror,-Wthread-safety-analysis]
        void unlock()          { this->Unlock(); }
                                       ^
external/parallel-hashmap/parallel_hashmap/phmap_base.h:5049:54: error: mutex 'this' is still held at the end of function [-Werror,-Wthread-safety-analysis]
        void lock_shared()     { this->ReaderLock(); }
                                                     ^
external/parallel-hashmap/parallel_hashmap/phmap_base.h:5049:40: note: mutex acquired here
        void lock_shared()     { this->ReaderLock(); }
                                       ^
external/parallel-hashmap/parallel_hashmap/phmap_base.h:5050:40: error: releasing mutex 'this' that was not held [-Werror,-Wthread-safety-analysis]
        void unlock_shared()   { this->ReaderUnlock(); }
                                       ^
4 errors generated.
greg7mdp commented 1 year ago

Weird, I tried with clang-16 and clang-17 which I have installed on my system (ubuntu) and I don't get any errors. Just installed clang-15 and no error or warning either when building the examples. What are you building that triggers these warnings?

greg7mdp commented 1 year ago

Also, there are fewer than 5046 lines in phmap_base.h. Did you add something?

phallot commented 1 year ago

I am using v1.3.11 which has more lines. https://github.com/greg7mdp/parallel-hashmap/blob/v1.3.11/parallel_hashmap/phmap.h

I'll try to create a repro.

greg7mdp commented 1 year ago

Oh yes I see, sorry. Are you using these mutexes directly in your code, or is it always from phmap?

phallot commented 1 year ago

We are using phmap::node_hash_map and phmap::HashState().combine, so not directly, no.

greg7mdp commented 1 year ago

I'm a little bit surprised, phmap::node_hash_map doesn't do any locking, maybe you mean phmap::parallel_node_hash_map?

In any case, thanks for reporting this issue. if you can create a small example reproducing the issue, I'll be happy to look into it.

phallot commented 1 year ago

I am not entirely sure how "thread-safety-analysis" works to be honest, but if it's like other warnings, the whole .h file is checked, not just the code used.

Thanks for the quick answers, I'll try to create a small example.

greg7mdp commented 1 year ago

Just wondering, are you on a mac? Because I can't get the warnings on ubuntu.

phallot commented 1 year ago

This is in a debian VM running on an ARM mac.

greg7mdp commented 1 year ago

Oh I see these warnings come from the Abseil mutex code which is included only when the Abseil headers are found. Clearly the warnings are false positives.

greg7mdp commented 1 year ago

No need for a repro. I'll address this when I have some time. Thanks again!

greg7mdp commented 1 year ago

Issue is fixed - thank you for reporting it @phallot