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

Race condition in steady_clock::now for _LIBCPP_WIN32API #40668

Closed
ivafanas mannequin opened this issue Mar 31, 2019 · 5 comments
Closed

Race condition in steady_clock::now for _LIBCPP_WIN32API #40668

ivafanas mannequin opened this issue Mar 31, 2019 · 5 comments
Labels
bugzilla libc++

Comments

@ivafanas
Copy link
Mannequin

@ivafanas ivafanas mannequin commented Mar 31, 2019

Bugzilla Link 41323
Resolution FIXED
Resolved on Apr 02, 2019 07:00
Version unspecified
OS All
CC @cpplearner,@ivafanas,@mclow

Extended Description

Method implementation is:

steady_clock::time_point
steady_clock::now() _NOEXCEPT
{
  static LARGE_INTEGER freq;
  static BOOL initialized = FALSE;
  if (!initialized)
    initialized = QueryPerformanceFrequency(&freq); // always succceeds

  LARGE_INTEGER counter;
  QueryPerformanceCounter(&counter);
  return time_point(duration(counter.QuadPart * nano::den / freq.QuadPart));
}

And seems like there is a race condition on freq and initialized variables if steady_clock::now has never been called before new threads creation.

Possible fixes are:

  1. use thread local storage for freq and initialized, but there might be some problems on Windows XP:
    https://reverseengineering.stackexchange.com/questions/14171/thread-local-storage-access-on-windows-xp

  2. use std::call_once guard, but it is slightly slower than thread local due to atomic access.

@mclow
Copy link
Contributor

@mclow mclow commented Apr 1, 2019

This should fix the problem, but I don't have direct access to a Windows box to check.
Can you let me know?
Also, how did you discover this?

diff --git a/libcxx/src/chrono.cpp b/libcxx/src/chrono.cpp
index ea0a638acc5..d342ee32d6c 100644
--- a/libcxx/src/chrono.cpp
+++ b/libcxx/src/chrono.cpp
@@ -177,16 +177,21 @@ steady_clock::now() _NOEXCEPT

#elif defined(_LIBCPP_WIN32API)

+static LARGE_INTEGER
+init_FREQ()
+{

  • LARGE_INTEGER val;
  • (void) QueryPerformanceFrequency(&val);
  • return val;
    +}

steady_clock::time_point
steady_clock::now() _NOEXCEPT
{

  • static LARGE_INTEGER freq;
  • static BOOL initialized = FALSE;
  • if (!initialized)
  • initialized = QueryPerformanceFrequency(&freq); // always succceeds
  • static const LARGE_INTEGER freq = init_FREQ();

    LARGE_INTEGER counter;

  • QueryPerformanceCounter(&counter);
  • (void) QueryPerformanceCounter(&counter);
    return time_point(duration(counter.QuadPart * nano::den / freq.QuadPart));
    }

@ivafanas
Copy link
Mannequin Author

@ivafanas ivafanas mannequin commented Apr 1, 2019

Yes, this fix is ok.
And it is cleaner than my proposals.

Can you let me know? Also, how did you discover this?

Well, I do not have windows box too. I'm just reviewing libcpp code in order to understand how it is organized.

@mclow
Copy link
Contributor

@mclow mclow commented Apr 1, 2019

Fixed in r357413.

@cpplearner
Copy link
Mannequin

@cpplearner cpplearner mannequin commented Apr 2, 2019

Fixed in r357413.

r357413 changes a invocation of "QueryPerformanceCounter" to instead call "QueryPerformanceFrequency" (note Counter vs. Frequency). Seems like a mistake?

@mclow
Copy link
Contributor

@mclow mclow commented Apr 2, 2019

Fixed in r357413.

r357413 changes a invocation of "QueryPerformanceCounter" to instead call
"QueryPerformanceFrequency" (note Counter vs. Frequency). Seems like a
mistake?

Oops. r357474

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla libc++
Projects
None yet
Development

No branches or pull requests

1 participant