From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 67081385773E for ; Thu, 11 May 2023 20:20:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 67081385773E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683836432; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=TeeqgbycRzmh98fjecBsIYdD0dxN1JRiQgJkQzZQXDk=; b=K5ilzyMOCdqhs3Z214ln06BIvUiQz/8iC2+cdhLvsY5BUFx/MX7jJDWnW0ZEJW9dRKQ27M JBm2KQyKchypiGtdhsnfZ8+JG2w6UHL0bVt/Esr/BsW66POxZUuZg1KXRO0U5XHmSHN/m7 xbFmu5fiadP+XJA9oQAUOiZTufNomqM= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-528-8JpjubTRMY2a-HEBt-QRRQ-1; Thu, 11 May 2023 16:20:30 -0400 X-MC-Unique: 8JpjubTRMY2a-HEBt-QRRQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4B775299E75F; Thu, 11 May 2023 20:20:30 +0000 (UTC) Received: from localhost (unknown [10.42.28.178]) by smtp.corp.redhat.com (Postfix) with ESMTP id 131C92166B26; Thu, 11 May 2023 20:20:29 +0000 (UTC) From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Fix chrono::hh_mm_ss::subseconds() [PR109772] Date: Thu, 11 May 2023 21:20:29 +0100 Message-Id: <20230511202029.1058974-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE,URI_HEX autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Tested powerpc64le-linux. Pushed to trunk. This is a regression on gcc-13 too, but I'm undecided about the ABI change for the branch. Generally that would be a no-go, but the affected specializations are probably so rare that it would be OK. And we definitely want to fix the ambiguity on the branch anyway. -- >8 -- I borked the logic in r13-4526-g5329e1a8e1480d so that the selected partial specialization of hh_mm_ss::__subseconds might not be able to represent the correct number of subseconds. This can result in a truncated value being stored for the subseconds, e.g., 4755859375 gets truncated to 460892079 because the correct value doesn't fit in uint_least32_t. Instead of checking whether the maximum value of the incoming duration type can be represented, we would need to check whether that maximum value can be represented after being converted to the correct precision type: template static constexpr bool __fits = duration_cast(_Duration::max()).count() <= duration_values<_Tp>::max(); However, this can fail to compile, due to integer overflow in the constexpr multiplications. Instead, we could limit the check to the case where the incoming duration has the same period as the precision, where no conversion is needed and so no overflow can happen. But that seems of very limited value, as it would only benefit specializations like hh_mm_ss>, which can only represent a time-of-day between -00:00:00.0215 and +00:00:00.0215 measured in picoseconds! Additionally, the hh_mm_ss::__subseconds partial specializations do not have disjoint constraints, so that some hh_mm_ss specializations result in ambiguities tying to match a __subseconds partial specialization. The most practical fix is to just stop using the __fits variable template in the constraints of the partial specializations. This fixes the truncated values by not selecting an inappropriate partial specialization, and fixes the ambiguous match by ensuring the constraints are disjoint. Fixing this changes the layout of some specializations, so is an ABI change. It only affects specializations that have a small (less than 64-bit) representation type and either a very small period (e.g. like the picosecond specialization above) or a non-power-of-ten period like ratio<1, 1024>. For example both hh_mm_ss> and hh_mm_ss> are affected (increasing from 16 bytes to 24 on x86_64), but hh_mm_ss> and hh_mm_ss> are not affected. libstdc++-v3/ChangeLog: PR libstdc++/109772 * include/std/chrono (hh_mm_ss::__fits): Remove variable template. (hh_mm_ss::__subseconds): Remove __fits from constraints. * testsuite/std/time/hh_mm_ss/109772.cc: New test. * testsuite/std/time/hh_mm_ss/1.cc: Adjust expected size for hh_mm_ss>. --- libstdc++-v3/include/std/chrono | 12 ++----- libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc | 2 +- .../testsuite/std/time/hh_mm_ss/109772.cc | 31 +++++++++++++++++++ 3 files changed, 34 insertions(+), 11 deletions(-) create mode 100644 libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono index 7bfc9b79acf..660e8d2b746 100644 --- a/libstdc++-v3/include/std/chrono +++ b/libstdc++-v3/include/std/chrono @@ -2398,17 +2398,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return {}; } }; - // True if the maximum constructor argument can be represented in _Tp. - template - static constexpr bool __fits - = duration_values::max() - <= duration_values<_Tp>::max(); - template requires (!treat_as_floating_point_v<_Rep>) && ratio_less_v<_Period, ratio<1, 1>> - && (ratio_greater_equal_v<_Period, ratio<1, 250>> - || __fits) + && ratio_greater_equal_v<_Period, ratio<1, 250>> struct __subseconds> { unsigned char _M_r{}; @@ -2421,8 +2414,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template requires (!treat_as_floating_point_v<_Rep>) && ratio_less_v<_Period, ratio<1, 250>> - && (ratio_greater_equal_v<_Period, ratio<1, 4000000000>> - || __fits) + && ratio_greater_equal_v<_Period, ratio<1, 4000000000>> struct __subseconds> { uint_least32_t _M_r{}; diff --git a/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc b/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc index f8a3e115af3..85f991c5e03 100644 --- a/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc +++ b/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc @@ -109,8 +109,8 @@ size() static_assert(sizeof(hh_mm_ss>) == sizeof(S1)); struct S2 { long long h; char m, s; bool neg; int ss; }; static_assert(sizeof(hh_mm_ss>) == sizeof(S2)); - static_assert(sizeof(hh_mm_ss>) == sizeof(S2)); struct S3 { long long h; char m, s; bool neg; long long ss; }; + static_assert(sizeof(hh_mm_ss>) == sizeof(S3)); static_assert(sizeof(hh_mm_ss>) == sizeof(S3)); struct S4 { long long h; char m, s; bool neg; double ss; }; static_assert(sizeof(hh_mm_ss>) == sizeof(S4)); diff --git a/libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc b/libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc new file mode 100644 index 00000000000..36137f283d2 --- /dev/null +++ b/libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc @@ -0,0 +1,31 @@ +// { dg-options "-std=gnu++20" } +// { dg-do compile { target c++20 } } + +// PR libstdc++/109772 Memory layout optimization of chrono::hh_mm_ss is wrong + +#include +#include + +using std::chrono::hh_mm_ss; +using std::chrono::duration; +using std::ratio; + +constexpr bool +test_truncated() +{ + using int_1_1024 = duration>; + static_assert( hh_mm_ss::fractional_width == 10 ); + + hh_mm_ss t1(int_1_1024(487)); + VERIFY( t1.subseconds().count() == (10'000'000'000 * 487 / 1024) ); + + hh_mm_ss t2(int_1_1024(1023)); + VERIFY( t2.subseconds().count() == (10'000'000'000 * 1023 / 1024) ); + + return true; +} + +static_assert( test_truncated() ); + +// Check for ambiguous partial specializations of hh_mm_ss::__subseconds: +static_assert( hh_mm_ss>>::fractional_width == 6 ); -- 2.40.1