comparison of identical zoned_time objects fails if created in different dll contexts

[michael-brade]

Pull request #3662 was supposed to fix this issue but it didn't supply the right test case. Here it is. The reason seems to be that .get_time_zone() returns a _TimeZonePtr, so adresses of the pointers are compared instead of the actual data. If the time(zone) was created in a different DLL, then the addresses are not the same even if the time is exactly the same.

Command-line test case

CMakeLists.txt

project(chrono)

set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

add_library(chrono-dll SHARED chrono-dll.cpp)

add_executable(chrono-test chrono-test.cpp)
target_link_libraries(chrono-test PUBLIC chrono-dll)

chrono-dll.cpp

#include <chrono>

__declspec(dllexport)
std::chrono::zoned_time<std::chrono::system_clock::duration> 
get_zoned_time(std::chrono::system_clock::time_point time) {
    return std::chrono::zoned_time<std::chrono::system_clock::duration> {std::chrono::current_zone(), time};
}

chrono-test.cpp

#include <chrono>
#include <iostream>

using namespace std;
using namespace std::chrono;

__declspec(dllimport)
zoned_time<system_clock::duration> get_zoned_time(system_clock::time_point time);

int main() {
    using timePoint = zoned_time<system_clock::duration>;

    auto now = system_clock::now();
    timePoint x {current_zone(), now};
    timePoint y = get_zoned_time(now);

    cout.setf(cout.boolalpha);
    cout << (x == y) << '\n';
}

Result:

C:\git>cmake -B build
-- Building for: Visual Studio 17 2022
-- Selecting Windows SDK version 10.0.20348.0 to target Windows 10.0.19042.
-- The C compiler identification is MSVC 19.35.32216.1
-- The CXX compiler identification is MSVC 19.35.32216.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.35.32215/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2022/Professional/VC/Tools/MSVC/14.35.32215/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: C:/git/build

C:\git>cmake --build build
MSBuild version 17.5.1+f6fdcf537 for .NET Framework

  Checking Build System
  Building Custom Rule C:/git/CMakeLists.txt
  chrono-dll.cpp
     Bibliothek "C:/git/build/Debug/chrono-dll.lib" und Objekt "C:/git/build/Debug/chrono-dll.exp" werden erstellt.
  chrono-dll.vcxproj -> C:\git\build\Debug\chrono-dll.dll
  Building Custom Rule C:/git/CMakeLists.txt
  chrono-test.cpp
  chrono-test.vcxproj -> C:\git\build\Debug\chrono-test.exe
  Building Custom Rule C:/git/CMakeLists.txt

C:\git>.\build\Debug\chrono-test.exe
false

Expected behavior

I expect to be able to compare zoned times even on different DLLs.

STL version

Microsoft Visual Studio Professional 2022 (64-Bit) - Current
Version 17.5.3

[StephanTLavavej]

Thanks for the self-contained test case! We talked about this at the weekly maintainer meeting; @dmitrykobets-msft noted that the C++ Standard doesn't recognize the existence of DLLs so this is technically not a conformance issue and we could simply decide that this is unsupported. @strega-nil-ms noted that zoned_time is templated on TimeZonePtr and that has relatively weak requirements. Because time_zone itself stores only the name, I believe we could make this scenario work, while imposing relatively minimal runtime cost on non-DLL scenarios, and without impacting the compile-time correctness of custom TimeZonePtrs, through the following approach:

1. Compare the addresses first, as mandated by the Standard. (This is the "fast path".)
2. If the addresses are non-equal, then if constexpr the TimeZonePtr is exactly const time_zone*, then compare the pointed-to objects. (This handles the user DLL scenario. It does impose a runtime cost on non-DLL scenarios when the comparison resolves to false, but the string comparison should be fairly cheap and hopefully there aren't millions being performed every second.)

Regarding test coverage, I would be satisfied with manual testing, without adding automated testing; we do have the ability to build multiple user binaries in our test harness but it's really obnoxious and I think it wouldn't be worth the effort here, as long as the relevant product code is commented as to why it's doing something beyond the Standard.

[YexuanXiao]

When the tzdb is updated, should zoned_times constructed from different versions of the tzdb be allowed to compare as equal?

[YexuanXiao]

The time_zone::operator== compares by name rather than by address. Since users cannot create time_zones, this means that time_zone from different versions of the tzdb can still be considered equal. I believe it makes sense to extend this property to zoned_time. Perhaps an LWG issue is needed here.

#include <chrono>
#include <cassert>

int main()
{
    using namespace std::chrono;
    auto&& tzdb = get_tzdb();
    auto&& new_tzdb = reload_tzdb();
    auto&& lzone = tzdb.current_zone();
    auto&& new_lzone = new_tzdb.current_zone();

    if (tzdb.version == new_tzdb.version)
        return 1;

    auto now = system_clock::now();

    zoned_time lzt{ lzone, now };
    zoned_time nlzt{ new_lzone, now };

    assert(*lzone == *new_lzone);
    // lzone == new_lzone          ?
    // lzt == nlzt                 ?
}

[frederick-vs-ja]

The time_zone::operator== compares by name rather than by address. Since users cannot create time_zones, this means that time_zone from different versions of the tzdb can still be considered equal.

I guess this is pointless at least for MSVC STL. MSVC STL's time_zone only stores a string which contains its name, and different time_zone objects of the same name don't behave differently.

[lfcarreon1]

IMO, the return value for operator== for std::chrono::zoned_time which is stated in the standard as:

x.zone == y.zone && x.tp == y.tp

should instead be:

(x.zone_) == (y.zone) && x.tp == y.tp_

because it doesn't make sense to compare pointers. It should be comparing the data pointed to.