Reimplement `GetWindowsLocalTimeZone` without `icu.h` #329

yukawa · 2025-08-28T14:51:47Z

This commit reimplements my previous commits (2ebbe0d)(f8e494f), which removed the dependency on WinRT on Windows by using system "icu.dll" (#315) when <icu.h> is available in the build environment.

Here are major improvements:

The relevant code is extracted into separate source files src/time_zone_name_win.{cc,h}. This is also a preparation for implementing TimeZoneIf with Windows time APIs, where one more ICU API needs to be dynamically imported.
- cctz::TimeZoneIf implementation that is compatible with Windows time APIs #328
The dependency on <icu.h> is removed by locally defining required constants and API prototypes. This should make it possible to build CCTZ with the MinGW toolchain, where <icu.h> is not yet available.
LoadLibraryExW(L"icu.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) is replaced with LoadLibraryW with the full path to icu.dll in the System32 directory, because LOAD_LIBRARY_SEARCH_SYSTEM32 can be ignored when icu.dll is already loaded from a non-System32 directory. Let's stick to the system icu.dll in favor of predictability.

Other than the above improvements, the overall logic remains unchanged.

derekmauro

Thanks for contributing this. I only have some minor comments.

src/icu_win.cc

src/icu_win.h

src/icu_win.cc

src/time_zone_name_win.h

src/time_zone_name_win.cc

devbww · 2025-09-16T05:19:15Z

src/time_zone_name_win.cc

+  // This function is intended to be lock free to avoid potential deadlocks
+  // with loader-lock taken inside LoadLibraryW. As LoadLibraryW and


This is intended to be lock-free so this is a kind of feature. Added a comment at the beginning of the function.

I don't know why "lock-free" is a goal here. What deadlocks are possible if we only call LoadLibraryW() at most once?

That is, for my money, GetWindowsLocalTimeZone() should just say something like ...

std::string local_time_zone; static const auto getTimeZoneIDForWindowsID = LoadIcuGetTimeZoneIDForWindowsID(); if (getTimeZoneIDForWindowsID != nullptr) { // Set local_time_zone. ... } return local_time_zone;

No need for atomic anything.

I don't know why "lock-free" is a goal here. What deadlocks are possible if we only call LoadLibraryW() at most once?

Win32 is an interesting environment, where things like LoadLibraryW(), GetProcAddress(), and DllMain() share the same lock.

https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-best-practices#deadlocks-caused-by-lock-order-inversion

As explained in #316 (comment), I guess deadlocks are still possible in the following scenario.

Suppose there are two running thread X and thread Y.

Thread X enters DllMain, where the operating system implicitly acquires Loader lock.

Thread Y acquires the lock to initialize static const auto getTimeZoneIDForWindowsID variable. It then get blocked at LoadLibraryW because the thread X still holds the Loader Lock.

Thread X also tries to read static const auto getTimeZoneIDForWindowsID then gets blocked because it is not yet fully initialized.

The current approach with std::atomic_* doesn't have the above dead lock issue, as it is just an optimistic optimization that does not try to enforce the exactly-once initialization.

Let me know if I misunderstood your question.

@devbww Let me know if there is any remaining concerns on this.

I guess my question boils down to situation 3. Why/how would a thread inside DllMain be calling CCTZ's GetWindowsLocalTimeZone()?

Why/how would a thread inside DllMain be calling CCTZ's GetWindowsLocalTimeZone()?

In my observation, people do not directly call cctz::local_time_zone() but end up calling it through higher level libraries that internally use CCTZ. A most notable example would be abseil and protobuf.

For instance, just calling absl::InitializeLog() ends up internally calling GetWindowsLocalTimeZone().

https://github.com/abseil/abseil-cpp/blob/ff4395895672c03fb544be00e3d66ebada1fcc47/absl/log/initialize.cc#L35

void InitializeLog() { InitializeLogImpl(absl::LocalTimeZone()); }

Other commonly-used methods that internally trigger GetWindowsLocalTimeZone() would be:

absl::AbslStringify(absl::Time)

absl::FormatTime(absl::Time)

In the real world DllMain implementations, it becomes extremely difficult for us to see if methods like absl::InitializeLog() and absl::FormatTime(absl::Time) are indirectly called or not. Here is an excerpt from Chromium code base.

https://github.com/chromium/chromium/blob/c56a646a9557ea1d54bc4ab7b9be2f1320a47e2d/chrome/credential_provider/gaiacp/gaia_credential_provider_module.cc#L148-L204

BOOL CGaiaCredentialProviderModule::DllMain(HINSTANCE /*hinstance*/, DWORD reason, LPVOID reserved) { switch (reason) { case DLL_PROCESS_ATTACH: { exit_manager_ = std::make_unique<base::AtExitManager>(); _set_invalid_parameter_handler(InvalidParameterHandler); // Initialize base. Command line will be set from GetCommandLineW(). base::CommandLine::Init(0, nullptr); // Initialize logging. logging::LoggingSettings settings; settings.logging_dest = logging::LOG_NONE; std::wstring log_file_path = GetGlobalFlagOrDefault(kRegLogFilePath, std::wstring{}); if (not log_file_path.empty()) { settings.logging_dest = logging::LOG_TO_FILE; bool append_log = GetGlobalFlagOrDefault(kRegLogFileAppend, 0); settings.delete_old = append_log ? logging::APPEND_TO_OLD_LOG_FILE : logging::DELETE_OLD_LOG_FILE; settings.log_file_path = log_file_path; } logging::InitLogging(settings); logging::SetLogItems(true, // Enable process id. true, // Enable thread id. true, // Enable timestamp. false); // Enable tickcount. logging::SetEventSource("GCPW", GCPW_CATEGORY, MSG_LOG_MESSAGE); if (GetGlobalFlagOrDefault(kRegEnableVerboseLogging, 0)) logging::SetMinLogLevel(logging::LOGGING_VERBOSE); break; } case DLL_PROCESS_DETACH: LOGFN(VERBOSE) << "DllMain(DLL_PROCESS_DETACH)"; // When this DLL is loaded for testing, don't reset the command line // since it causes tests to crash. if (!is_testing_) base::CommandLine::Reset(); _set_invalid_parameter_handler(nullptr); exit_manager_.reset(); crash_reporter::DestroyCrashpadClient(); break; default: break; } return ATL::CAtlDllModuleT<CGaiaCredentialProviderModule>::DllMain(reason, reserved); }

I actually don't know whether methods like absl::FormatTime(absl::Time) and absl::FormatTime(absl::Time) are called or not in the above code, but the fact it's hard to see is already a good reason to make it safe in the first place, I believe.

OK, I'm going to defer to your expertise here.

Still, it seems to me that it is a fundamental flaw having interfaces that take a lock, while having others that can call out to arbitrary code while holding the same lock. If that didn't happen everyone would be better off, and we wouldn't have to perform these dances.

src/time_zone_name_win.cc

derekmauro

Trying to get the presubmit to run on this.

CMakeLists.txt

devbww · 2025-11-09T19:37:00Z

src/time_zone_name_win.cc

+// UChar is defined as char16_t in ICU by default, but it is also safe to assume
+// wchar_t and char16_t are equivalent on Windows.
+using UChar = wchar_t;


So why not just define it as char16_t and remove the need to comment?

The main motivation is to avoid reinterpret_cast around Win32 APIs, which receive wchar_t* instead of char16_t*. Rephrased the comment to clarify it.

src/time_zone_name_win.cc

devbww · 2025-11-09T20:10:36Z

src/time_zone_name_win.cc

+  }
+
+  std::wstring result;
+  // TODO: Remove std::max() when we require C++17 or higher.


Is this referring to &result[0] being undefined when result.empty()? If so, then I believe anything after C++11 is sufficient. Either way it is worth clarifying.

OK, you are right. Removed TODO comment for simplicity.

devbww · 2025-11-09T20:12:10Z

src/time_zone_name_win.cc

+  while (true) {
+    UErrorCode status = U_ZERO_ERROR;
+    result.resize(len);
+    // TODO: Switch to std::string::data() when we require C++17 or higher.


Is this referring to data() returning a pointer to non-const? It is worth clarifying.

Removed TODO as &result[0] remains to be sufficient.

devbww · 2025-11-09T20:22:35Z

src/time_zone_name_win.cc

+    if (U_SUCCESS(status)) {
+      return Utf16ToUtf8(result.data(), len);
+    }
+    if (status != U_BUFFER_OVERFLOW_ERROR) {


I would probably also fail if len != static_cast<std::int32_t>(len), as that would cause us to pass the wrong length on the next loop.

Indeed, that test should come before the getTimeZoneIDForWindowsID() call in case result.capacity() is initially beyond std::int32_t limits. That's highly unlikely, I know, but why not get it exactly right anyway.

Alternatively, the initial len could be capped at the maximum std::int32_t.

Good point. Initialized len with std::int32_t as the maximum.

I believe that using &result[0] (here and elsewhere) while claiming to support C++11 still requires us to bound the initial len from below at 1.

devbww · 2025-11-09T20:27:24Z

src/time_zone_name_win.cc

+  }
+  const int chars_len = static_cast<int>(size);
+  std::string result;
+  // TODO: Remove std::max() when we require C++17 or higher.


See the other comment on excluding the empty initial capacity.

Done. Initialized len with std::int32_t as the maximum.

devbww · 2025-11-09T20:27:27Z

src/time_zone_name_win.cc

+    result.resize(len);
+    // TODO: Switch to std::string::data() when we require C++17 or higher.
+    len = ::WideCharToMultiByte(CP_UTF8, WC_ERR_INVALID_CHARS, ptr, chars_len,
+                                &result[0], static_cast<int>(result.size()),


See the other comment on checking for int overflow.

Done. Initialized len with std::int32_t as the maximum.

devbww · 2025-11-09T20:31:08Z

src/time_zone_name_win.cc

+  do {
+    result.resize(len);
+    // TODO: Switch to std::string::data() when we require C++17 or higher.
+    len = ::GetSystemDirectoryW(&result[0], static_cast<UINT>(result.size()));


See the other comment on checking for cast truncation.

Done. Initialized len with std::uint32_t as the maximum.

devbww · 2025-11-09T20:42:03Z

src/time_zone_name_win.cc

+  // This function is intended to be lock free to avoid potential deadlocks
+  // with loader-lock taken inside LoadLibraryW. As LoadLibraryW and


I guess my question boils down to situation 3. Why/how would a thread inside DllMain be calling CCTZ's GetWindowsLocalTimeZone()?

devbww · 2025-11-13T00:26:21Z

src/time_zone_name_win.cc

+    if (U_SUCCESS(status)) {
+      return Utf16ToUtf8(result.data(), len);
+    }
+    if (status != U_BUFFER_OVERFLOW_ERROR) {


I believe that using &result[0] (here and elsewhere) while claiming to support C++11 still requires us to bound the initial len from below at 1.

devbww · 2025-11-13T00:40:10Z

src/time_zone_name_win.cc

+  // This function is intended to be lock free to avoid potential deadlocks
+  // with loader-lock taken inside LoadLibraryW. As LoadLibraryW and


OK, I'm going to defer to your expertise here.

Still, it seems to me that it is a fundamental flaw having interfaces that take a lock, while having others that can call out to arbitrary code while holding the same lock. If that didn't happen everyone would be better off, and we wouldn't have to perform these dances.

src/time_zone_name_win.cc

This commit reimplements my previous commits [1][2], which removed the dependency on WinRT on Windows by using system "icu.dll" (google#315) when <icu.h> is available in the build environment. Here are major improvements: * The relevant code is extracted into separate source files "src/time_zone_name_win.{cc,h}". This is also a preparation for implementing TimeZoneIf with Windows time APIs (google#328), where one more ICU API needs to be dynamically imported. * The dependency on <icu.h> is removed by locally defining required constants and API prototypes. This should make it possible to build CCTZ with the MinGW toolchain, where <icu.h> is not yet available. * LoadLibraryExW(L"icu.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) is replaced with LoadLibraryW with the full path to "icu.dll" in the System32 directory, because LOAD_LIBRARY_SEARCH_SYSTEM32 can be ignored when "icu.dll" is already loaded from a non-System32 directory. Let's stick to the system "icu.dll" in favor of predictability. Other than the above improvements, the overall logic remains unchanged. [1]: 2ebbe0d [2]: f8e494f

devbww

OK, thanks. I'll leave it to @derekmauro to run the checks and give final approval.

derekmauro · 2025-11-18T14:26:21Z

Thanks @yukawa for the contribution and @devbww for the review.

yukawa marked this pull request as ready for review August 28, 2025 14:52

derekmauro requested changes Sep 4, 2025

View reviewed changes

src/icu_win.cc Outdated Show resolved Hide resolved

src/icu_win.cc Outdated Show resolved Hide resolved

yukawa force-pushed the introduce_icu_win branch from 4af4c81 to ffa61d1 Compare September 5, 2025 05:18

devbww mentioned this pull request Sep 5, 2025

Implement TimeZoneIf with Windows Registry #327

Draft

devbww reviewed Sep 5, 2025

View reviewed changes

yukawa force-pushed the introduce_icu_win branch 2 times, most recently from a193211 to d47ce9c Compare September 7, 2025 15:42

devbww reviewed Sep 16, 2025

View reviewed changes

yukawa force-pushed the introduce_icu_win branch 2 times, most recently from e9571db to 1e8d3bb Compare September 17, 2025 10:00

derekmauro self-requested a review November 7, 2025 17:25

derekmauro approved these changes Nov 7, 2025

View reviewed changes

derekmauro force-pushed the introduce_icu_win branch from 1e8d3bb to af9947a Compare November 7, 2025 17:34

derekmauro requested changes Nov 7, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

yukawa force-pushed the introduce_icu_win branch from af9947a to c8bc1fb Compare November 8, 2025 06:02

devbww reviewed Nov 9, 2025

View reviewed changes

devbww reviewed Nov 13, 2025

View reviewed changes

yukawa force-pushed the introduce_icu_win branch from c900357 to 401182f Compare November 14, 2025 02:49

devbww approved these changes Nov 18, 2025

View reviewed changes

derekmauro approved these changes Nov 18, 2025

View reviewed changes

derekmauro merged commit 27ca173 into google:master Nov 18, 2025
7 checks passed

yukawa deleted the introduce_icu_win branch November 19, 2025 01:36

		// This function is intended to be lock free to avoid potential deadlocks
		// with loader-lock taken inside LoadLibraryW. As LoadLibraryW and

Reimplement GetWindowsLocalTimeZone without icu.h #329

Reimplement GetWindowsLocalTimeZone without icu.h #329

Conversation

yukawa commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekmauro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yukawa Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

derekmauro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Reimplement `GetWindowsLocalTimeZone` without `icu.h` #329

Reimplement `GetWindowsLocalTimeZone` without `icu.h` #329

yukawa commented Aug 28, 2025 •

edited

Loading

yukawa Sep 16, 2025 •

edited

Loading