Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

False positive from clang-analyzer-deadcode.DeadStores with reference and thread #50641

Open Quuxplusone opened 3 years ago

Quuxplusone commented 3 years ago
Bugzilla Link PR51674
Status NEW
Importance P enhancement
Reported by Sam Kearney (samuelmkearney@gmail.com)
Reported on 2021-08-30 07:50:34 -0700
Last modified on 2021-09-03 09:27:46 -0700
Version unspecified
Hardware PC Windows NT
CC dcoughlin@apple.com, llvm-bugs@lists.llvm.org, nok.raven@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

To reproduce: Install LLVM 14.0, add this minimal example as main.cpp:


#include <chrono>
#include <thread>
#include <iostream>

using namespace std::chrono_literals;

int main()
{
  bool keep_running_thread = true;

  std::thread thrd(
      [](const bool& keep_running) {
        while (keep_running)
        {
          std::cout << "Still running!\n";
          std::this_thread::sleep_for(200ms);
        }
      },
      std::ref(keep_running_thread));

  std::this_thread::sleep_for(1s);
  keep_running_thread = false;
  thrd.join();
  return 0;
}

Run clang-tidy --checks=clang-analyzer-* main.cpp. It reports:

main.cpp:22:3: warning: Value stored to 'keep_running_thread' is never read [clang-analyzer-deadcode.DeadStores] keep_running_thread = false; ^ ~ main.cpp:22:3: note: Value stored to 'keep_running_thread' is never read keep_running_thread = false; ^ ~

I would expect no warning here since a reference to keep_running_thread is passed to the thread.

Note: You can work around this by using a capturing lambda that captures keep_running_thread by reference; but I would expect this to work using the method shown as well.

Quuxplusone commented 3 years ago

The warning message is actually not wrong: the code contains a data race, the compiler is allowed to replace the read operation of keep_running with a constant and remove the read of keep_running_thread.

Quuxplusone commented 3 years ago

Oh, interesting! I'm happy to have this bug closed then. If you're able to point me to the documentation or portion of the standard that explains this, that would be very helpful for me, just for my own learning experience. Thanks!

Quuxplusone commented 3 years ago
(In reply to Sam Kearney from comment #2)
> Oh, interesting! I'm happy to have this bug closed then.

Leave it open. The warning message could be improved.

(In reply to Sam Kearney from comment #2)
> If you're able to
> point me to the documentation or portion of the standard that explains this,
> that would be very helpful for me, just for my own learning experience.

[intro.races] p21 (https://eel.is/c++draft/intro.races#21)