google / sanitizers

AddressSanitizer, ThreadSanitizer, MemorySanitizer
Other
11.4k stars 1.03k forks source link

Thread Sanitizer false positive on lock-order-inversion (potential deadlock) #1419

Open brenoguim opened 3 years ago

brenoguim commented 3 years ago

I'm starting to use the Thread Sanitizer and debugged a few issues that turned out to be false. It's somewhat related to #814 but not quite the same:

#include <thread>                                                                                                       
#include <mutex>                                                                                                        
#include <atomic>                                                                                                       

int main()                                                                                                              
{                                                                                                                       
    std::atomic<bool> stop = false;                                                                                     
    std::mutex m1, m2, m3;                                                                                              

    std::thread a([&] {                                                                                                 
        while (!stop) {                                                                                                 
            std::lock_guard l1(m1);                                                                                     
            std::lock_guard l2(m2);                                                                                     
        }                                                                                                               
    });                                                                                                                 

    std::thread b([&] {                                                                                                 
        while (!stop) {                                                                                                 
            {                                                                                                           
                std::lock_guard l2(m2);                                                                                 
                std::lock_guard l3(m3);                                                                                 
            }                                                                                                           
            {                                                                                                           
                std::lock_guard l3(m3);                                                                                 
                std::lock_guard l1(m1);                                                                                 
            }                                                                                                           
        }                                                                                                               
    });                                                                                                                 

    using namespace std::chrono_literals;                                                                                  
    std::this_thread::sleep_for(3s);                                                                                       

    stop = true;                                                                                                           
    a.join();                                                                                                           
    b.join();                                                                                                           
}

TSan reports a ordering inversion because m1->m2 may deadlock with m2->m3 + m3->m1 from the second thread. However this can't really happen because the two later connections cannot happen concurrently, since they come from the same thread of execution.

I suspect the same reasoning for #814 may apply here such that it doesn't look like good code. But while I agree it may not be the greatest code, it would be much more valuable to fix the more concerning violation first - but it's hard to find them among these "false" ones.

dvyukov commented 3 years ago

Looks similar to #488