[libcxx][test] Skip sys_info zdump test when zdump was built with 32 bit time_t #103056

DavidSpickett · 2024-08-13T13:05:27Z

This test started passing on Armhf Ubuntu Jammy, but fails on Focal. This is due to extra output from zdump on Jammy, which makes it match the information libcxx gets from the time zone database.

Focal:

$ zdump --version
zdump (Ubuntu GLIBC 2.31-0ubuntu9.16) 2.31
tzdata/focal-updates,now 2024a-0ubuntu0.20.04.1 all [installed,automatic]
$ zdump -V -c1800,2100 Africa/Addis_Ababa
Africa/Addis_Ababa  Mon May  4 21:24:39 1936 UT = Mon May  4 23:59:59 1936 ADMT isdst=0 gmtoff=9320
Africa/Addis_Ababa  Mon May  4 21:24:40 1936 UT = Tue May  5 00:24:40 1936 EAT isdst=0 gmtoff=10800

Jammy:

$ zdump --version
zdump (Ubuntu GLIBC 2.35-0ubuntu3.8) 2.35
tzdata/jammy-updates,now 2024a-0ubuntu0.22.04.1 all [installed,automatic]
$ zdump -V -c1800,2100 Africa/Addis_Ababa
Africa/Addis_Ababa  Fri Dec 31 21:25:11 1869 UT = Fri Dec 31 23:59:59 1869 LMT isdst=0 gmtoff=9288
Africa/Addis_Ababa  Fri Dec 31 21:25:12 1869 UT = Sat Jan  1 00:00:32 1870 ADMT isdst=0 gmtoff=9320
Africa/Addis_Ababa  Mon May  4 21:24:39 1936 UT = Mon May  4 23:59:59 1936 ADMT isdst=0 gmtoff=9320
Africa/Addis_Ababa  Mon May  4 21:24:40 1936 UT = Tue May  5 00:24:40 1936 EAT isdst=0 gmtoff=10800

(lots more zones have differences)

This is due to zdump being built with time_t as 32 bit on armhf in glibc 2.31. The input years get truncated into the 32 bit range, putting some entries outside the possible range.

In glibc 2.34 this changed so that zdump has time_t as 64 bit on armhf, so it shows all the entries.

https://sourceware.org/git/?p=glibc.git;a=blob;f=NEWS;h=d488874aba371b2bfa1d99fb607eb653fdd19e17;hb=HEAD#l1331

* Add support for 64-bit time_t on configurations like x86 where time_t
  is traditionally 32-bit.  Although time_t still defaults to 32-bit on
  these configurations, this default may change in future versions.
  This is enabled with the _TIME_BITS preprocessor macro set to 64 and is
  only supported when LFS (_FILE_OFFSET_BITS=64) is also enabled.  It is
  only enabled for Linux and the full support requires a minimum kernel
  version of 5.1.

This commit adds a test feature to detect this change. The alternative is to disable it on any armhf Linux, which seems too broad to me.

zatrazz · 2024-08-13T13:34:56Z

Checking zdump output should be ok, it is a glibc provided binary and it is directly on test. LGTM.

libcxx/utils/libcxx/test/features.py

llvmbot · 2024-08-14T09:46:33Z

@llvm/pr-subscribers-libcxx

Author: David Spickett (DavidSpickett)

Changes

This test started passing on Armhf Ubuntu Jammy, but fails on Focal. This is due to extra output from zdump on Jammy, which makes it match the information libcxx gets from the time zone database.

Focal:

$ zdump --version
zdump (Ubuntu GLIBC 2.31-0ubuntu9.16) 2.31
tzdata/focal-updates,now 2024a-0ubuntu0.20.04.1 all [installed,automatic]
$ zdump -V -c1800,2100 Africa/Addis_Ababa
Africa/Addis_Ababa  Mon May  4 21:24:39 1936 UT = Mon May  4 23:59:59 1936 ADMT isdst=0 gmtoff=9320
Africa/Addis_Ababa  Mon May  4 21:24:40 1936 UT = Tue May  5 00:24:40 1936 EAT isdst=0 gmtoff=10800

Jammy:

$ zdump --version
zdump (Ubuntu GLIBC 2.35-0ubuntu3.8) 2.35
tzdata/jammy-updates,now 2024a-0ubuntu0.22.04.1 all [installed,automatic]
$ zdump -V -c1800,2100 Africa/Addis_Ababa
Africa/Addis_Ababa  Fri Dec 31 21:25:11 1869 UT = Fri Dec 31 23:59:59 1869 LMT isdst=0 gmtoff=9288
Africa/Addis_Ababa  Fri Dec 31 21:25:12 1869 UT = Sat Jan  1 00:00:32 1870 ADMT isdst=0 gmtoff=9320
Africa/Addis_Ababa  Mon May  4 21:24:39 1936 UT = Mon May  4 23:59:59 1936 ADMT isdst=0 gmtoff=9320
Africa/Addis_Ababa  Mon May  4 21:24:40 1936 UT = Tue May  5 00:24:40 1936 EAT isdst=0 gmtoff=10800

(lots more zones have differences)

This is due to zdump being built with time_t as 32 bit on armhf in glibc 2.31. The input years get truncated into the 32 bit range, putting some entries outside the possible range.

In glibc 2.34 this changed so that zdump has time_t as 64 bit on armhf, so it shows all the entries.

https://sourceware.org/git/?p=glibc.git;a=blob;f=NEWS;h=d488874aba371b2bfa1d99fb607eb653fdd19e17;hb=HEAD#l1331

* Add support for 64-bit time_t on configurations like x86 where time_t
  is traditionally 32-bit.  Although time_t still defaults to 32-bit on
  these configurations, this default may change in future versions.
  This is enabled with the _TIME_BITS preprocessor macro set to 64 and is
  only supported when LFS (_FILE_OFFSET_BITS=64) is also enabled.  It is
  only enabled for Linux and the full support requires a minimum kernel
  version of 5.1.

This commit adds a test feature to detect this change. The alternative is to disable it on any armhf Linux, which seems too broad to me.

Full diff: https://github.com/llvm/llvm-project/pull/103056.diff

2 Files Affected:

(modified) libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp (+1-4)
(modified) libcxx/utils/libcxx/test/features.py (+15)

diff --git a/libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp b/libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp
index 207f8e4df45413..6af236ff007cb9 100644
--- a/libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp
+++ b/libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp
@@ -7,15 +7,12 @@
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
-// UNSUPPORTED: no-filesystem, no-localization, no-tzdb, has-no-zdump
+// UNSUPPORTED: no-filesystem, no-localization, no-tzdb, has-no-zdump, zdump-time_t-32bit
 // REQUIRES: long_tests
 
 // XFAIL: libcpp-has-no-experimental-tzdb
 // XFAIL: availability-tzdb-missing
 
-// TODO TZDB Investigate
-// XFAIL: target={{armv(7|8)l-linux-gnueabihf}}
-
 #include <chrono>
 #include <format>
 #include <fstream>
diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index 97cdb0349885d6..ba2111c3278833 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -352,6 +352,21 @@ def _mingwSupportsModules(cfg):
         name="has-no-zdump",
         when=lambda cfg: runScriptExitCode(cfg, ["zdump --version"]) != 0,
     ),
+    # zdump built with 32 bit time_t will truncate times into the 32 bit range.
+    # Starting with glibc 2.34, some architectures, like armhf, started building
+    # zdump with time_t as 64 bit which allows it to handle the maximum range.
+    # If zdump was built with time_t 64 bit, it will show the 1869 entries for
+    # this zone, which would be out of range for 32 bit.
+    Feature(
+        name="zdump-time_t-32bit",
+        when=lambda cfg: BooleanExpression.evaluate(
+            "!has-no-zdump", cfg.available_features
+        )
+        and runScriptExitCode(
+            cfg, ["zdump -V -c1800,2100 Africa/Addis_Ababa | grep -q 1869"]
+        )
+        != 0,
+    ),
 ]
 
 # Deduce and add the test features that that are implied by the #defines in

Until #103056 lands or another more appropriate check can be found. This test fails on Ubuntu Focal where zdump is built with 32 bit time_t but passes on Ubuntu Jammy where zdump is built with 64 bit time_t. Marking it unsupported means Linaro can upgrade its bots to Ubuntu Jammy without getting an unexpected pass.

DavidSpickett · 2024-08-15T08:18:13Z

In the meantime I've marked this UNSUPPORTED so we can upgrade our bots to Jammy - 6f6422f.

…bit time_t This test started passing on Armhf Ubuntu Jammy, but fails on Focal. This is due to extra output from zdump on Jammy, which makes it match the information libcxx gets from the time zone database. Focal: ``` $ zdump --version zdump (Ubuntu GLIBC 2.31-0ubuntu9.16) 2.31 tzdata/focal-updates,now 2024a-0ubuntu0.20.04.1 all [installed,automatic] $ zdump -V -c1800,2100 Africa/Addis_Ababa Africa/Addis_Ababa Mon May 4 21:24:39 1936 UT = Mon May 4 23:59:59 1936 ADMT isdst=0 gmtoff=9320 Africa/Addis_Ababa Mon May 4 21:24:40 1936 UT = Tue May 5 00:24:40 1936 EAT isdst=0 gmtoff=10800 ``` Jammy: ``` $ zdump --version zdump (Ubuntu GLIBC 2.35-0ubuntu3.8) 2.35 tzdata/jammy-updates,now 2024a-0ubuntu0.22.04.1 all [installed,automatic] $ zdump -V -c1800,2100 Africa/Addis_Ababa Africa/Addis_Ababa Fri Dec 31 21:25:11 1869 UT = Fri Dec 31 23:59:59 1869 LMT isdst=0 gmtoff=9288 Africa/Addis_Ababa Fri Dec 31 21:25:12 1869 UT = Sat Jan 1 00:00:32 1870 ADMT isdst=0 gmtoff=9320 Africa/Addis_Ababa Mon May 4 21:24:39 1936 UT = Mon May 4 23:59:59 1936 ADMT isdst=0 gmtoff=9320 Africa/Addis_Ababa Mon May 4 21:24:40 1936 UT = Tue May 5 00:24:40 1936 EAT isdst=0 gmtoff=10800 ``` (lots more zones have differences) This is due to zdump being built with time_t as 32 bit on armhf in glibc 2.31. The input years get truncated into the 32 bit range, putting some entries outside the possible range. In glibc 2.34 this changed so that zdump has time_t as 64 bit on armhf, so it shows all the entries. https://sourceware.org/git/?p=glibc.git;a=blob;f=NEWS;h=d488874aba371b2bfa1d99fb607eb653fdd19e17;hb=HEAD#l1331 ``` * Add support for 64-bit time_t on configurations like x86 where time_t is traditionally 32-bit. Although time_t still defaults to 32-bit on these configurations, this default may change in future versions. This is enabled with the _TIME_BITS preprocessor macro set to 64 and is only supported when LFS (_FILE_OFFSET_BITS=64) is also enabled. It is only enabled for Linux and the full support requires a minimum kernel version of 5.1. ``` This commit adds a test feature to detect this change. The alternative is to disable it on any armhf Linux, which seems too broad to me.

mordante

I have some comments for features.py. However I would prefer to look at fixing the test for 32-bit time_t instead of loosing coverage. Can you look at my suggestion?

mordante · 2024-08-18T13:15:26Z

libcxx/test/std/time/time.zone/time.zone.timezone/time.zone.members/sys_info.zdump.pass.cpp

 // REQUIRES: long_tests

 // XFAIL: libcpp-has-no-experimental-tzdb
 // XFAIL: availability-tzdb-missing

-// TODO TZDB Investigate
-// UNSUPPORTED: target={{armv(7|8)l-linux-gnueabihf}}
-
 #include <chrono>


How changing constexpr std::chrono::year last{2100}; at line 28 to 2037 for 32-bit time_t instead?

mordante · 2024-08-18T13:18:11Z

Thanks for investigating the issue!

DavidSpickett · 2024-09-13T08:15:19Z

Abandoning in favour of #104762.

Until llvm#103056 lands or another more appropriate check can be found. This test fails on Ubuntu Focal where zdump is built with 32 bit time_t but passes on Ubuntu Jammy where zdump is built with 64 bit time_t. Marking it unsupported means Linaro can upgrade its bots to Ubuntu Jammy without getting an unexpected pass. (cherry picked from commit 6f6422f)

DavidSpickett requested a review from a team as a code owner August 13, 2024 13:05

DavidSpickett mentioned this pull request Aug 13, 2024

[libc++][chrono] Adds the sys_info class. #85619

Merged

DavidSpickett requested review from zatrazz and mordante August 13, 2024 13:06

DavidSpickett force-pushed the libcxx-zdump branch from 58aecd3 to 4f4f834 Compare August 13, 2024 13:08

DavidSpickett commented Aug 13, 2024

View reviewed changes

libcxx/utils/libcxx/test/features.py Show resolved Hide resolved

DavidSpickett force-pushed the libcxx-zdump branch from f0d5348 to 406e69c Compare August 14, 2024 09:46

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 14, 2024

DavidSpickett added 2 commits August 15, 2024 12:50

Don't add feature when zdump isn't present at all.

42755a5

DavidSpickett force-pushed the libcxx-zdump branch from 406e69c to 42755a5 Compare August 15, 2024 12:50

mordante requested changes Aug 18, 2024

View reviewed changes

mordante self-assigned this Aug 18, 2024

DavidSpickett mentioned this pull request Aug 19, 2024

[libcxx][test] Use smaller time range for 32 bit time_t #104762

Merged

DavidSpickett closed this Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libcxx][test] Skip sys_info zdump test when zdump was built with 32 bit time_t #103056

[libcxx][test] Skip sys_info zdump test when zdump was built with 32 bit time_t #103056

Uh oh!

DavidSpickett commented Aug 13, 2024

Uh oh!

zatrazz commented Aug 13, 2024

Uh oh!

Uh oh!

llvmbot commented Aug 14, 2024

Uh oh!

DavidSpickett commented Aug 15, 2024

Uh oh!

mordante left a comment

Uh oh!

mordante Aug 18, 2024

Uh oh!

DavidSpickett Aug 19, 2024

Uh oh!

mordante commented Aug 18, 2024

Uh oh!

DavidSpickett commented Sep 13, 2024

Uh oh!

Uh oh!

[libcxx][test] Skip sys_info zdump test when zdump was built with 32 bit time_t #103056

[libcxx][test] Skip sys_info zdump test when zdump was built with 32 bit time_t #103056

Uh oh!

Conversation

DavidSpickett commented Aug 13, 2024

Uh oh!

zatrazz commented Aug 13, 2024

Uh oh!

Uh oh!

llvmbot commented Aug 14, 2024

Uh oh!

DavidSpickett commented Aug 15, 2024

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

mordante Aug 18, 2024

Choose a reason for hiding this comment

Uh oh!

DavidSpickett Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

mordante commented Aug 18, 2024

Uh oh!

DavidSpickett commented Sep 13, 2024

Uh oh!

Uh oh!