[PATCH] Define new filesystem::__file_clock type

[PATCH] Define new filesystem::__file_clock type

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

In C++17 the clock used for filesystem::file_time_type is unspecified,
allowing it to be chrono::system_clock. The C++2a draft requires it to
be a distinct type, with additional member functions to convert to/from
other clocks (either the system clock or UTC). In order to avoid an ABI
change later, this patch defines a new distinct type now, which will be
used for std::chrono::file_clock later.

	* include/bits/fs_fwd.h (__file_clock): Define new clock.
	(file_time_type): Redefine in terms of __file_clock.
	* src/filesystem/ops-common.h (file_time): Add FIXME comment about
	overflow.
	* src/filesystem/std-ops.cc (is_set(perm_options, perm_options)): Give
	internal linkage.
	(internal_file_lock): New helper type for accessing __file_clock.
	(do_copy_file): Use internal_file_lock to convert system time to
	file_time_type.
	(last_write_time(const path&, error_code&)): Likewise.
	(last_write_time(const path&, file_time_type, error_code&)): Likewise.

Tested powerpc64-linux, committed to trunk.


[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 7075 bytes --]

commit c91a86730ed8df76325dd0649d8837e6feb24aab
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sat Jan 5 12:52:40 2019 +0000

    Define new filesystem::__file_clock type
    
    In C++17 the clock used for filesystem::file_time_type is unspecified,
    allowing it to be chrono::system_clock. The C++2a draft requires it to
    be a distinct type, with additional member functions to convert to/from
    other clocks (either the system clock or UTC). In order to avoid an ABI
    change later, this patch defines a new distinct type now, which will be
    used for std::chrono::file_clock later.
    
            * include/bits/fs_fwd.h (__file_clock): Define new clock.
            (file_time_type): Redefine in terms of __file_clock.
            * src/filesystem/ops-common.h (file_time): Add FIXME comment about
            overflow.
            * src/filesystem/std-ops.cc (is_set(perm_options, perm_options)): Give
            internal linkage.
            (internal_file_lock): New helper type for accessing __file_clock.
            (do_copy_file): Use internal_file_lock to convert system time to
            file_time_type.
            (last_write_time(const path&, error_code&)): Likewise.
            (last_write_time(const path&, file_time_type, error_code&)): Likewise.

diff --git a/libstdc++-v3/include/bits/fs_fwd.h b/libstdc++-v3/include/bits/fs_fwd.h
index 3bcf2dd650c..a0e3d73e2a3 100644
--- a/libstdc++-v3/include/bits/fs_fwd.h
+++ b/libstdc++-v3/include/bits/fs_fwd.h
@@ -294,7 +294,49 @@ _GLIBCXX_END_NAMESPACE_CXX11
   operator^=(directory_options& __x, directory_options __y) noexcept
   { return __x = __x ^ __y; }
 
-  using file_time_type = std::chrono::system_clock::time_point;
+  struct __file_clock
+  {
+    using duration                  = chrono::nanoseconds;
+    using rep                       = duration::rep;
+    using period                    = duration::period;
+    using time_point                = chrono::time_point<__file_clock>;
+    static constexpr bool is_steady = false;
+
+    static time_point
+    now() noexcept
+    { return _S_from_sys(chrono::system_clock::now()); }
+
+  private:
+    using __sys_clock = chrono::system_clock;
+
+    // This clock's (unspecified) epoch is 2174-01-01 00:00:00 UTC.
+    // A signed 64-bit duration with nanosecond resolution gives roughly
+    // +/- 292 years, which covers the 1901-2446 date range for ext4.
+    static constexpr chrono::seconds _S_epoch_diff{6437664000};
+
+  protected:
+    // For internal use only
+    template<typename _Dur>
+      static
+      chrono::time_point<__file_clock, _Dur>
+      _S_from_sys(const chrono::time_point<__sys_clock, _Dur>& __t) noexcept
+      {
+	using __file_time = chrono::time_point<__file_clock, _Dur>;
+	return __file_time{__t.time_since_epoch()} - _S_epoch_diff;
+      }
+
+    // For internal use only
+    template<typename _Dur>
+      static
+      chrono::time_point<__sys_clock, _Dur>
+      _S_to_sys(const chrono::time_point<__file_clock, _Dur>& __t) noexcept
+      {
+	using __sys_time = chrono::time_point<__sys_clock, _Dur>;
+	return __sys_time{__t.time_since_epoch()} + _S_epoch_diff;
+      }
+  };
+
+  using file_time_type = __file_clock::time_point;
 
   // operational functions
 
diff --git a/libstdc++-v3/src/filesystem/ops-common.h b/libstdc++-v3/src/filesystem/ops-common.h
index dcd61cc26cd..1c0d650f444 100644
--- a/libstdc++-v3/src/filesystem/ops-common.h
+++ b/libstdc++-v3/src/filesystem/ops-common.h
@@ -158,6 +158,24 @@ namespace __gnu_posix
     nanoseconds ns{};
 #endif
 
+    // FIXME
+    // There are possible timespec values which will overflow
+    // chrono::system_clock::time_point but would not overflow
+    // __file_clock::time_point, due to its different epoch.
+    //
+    // By checking for overflow of the intermediate system_clock::duration
+    // type, we report an error for values which are actually representable
+    // in the file_time_type result type.
+    //
+    // Howard Hinnant's solution for this problem is to use
+    // duration<__int128>{s} + ns, which doesn't overflow.
+    // An alternative would be to do the epoch correction on s before
+    // the addition, and then go straight to file_time_type instead of
+    // going via chrono::system_clock::time_point.
+    //
+    // (This only applies to the C++17 Filesystem library, because for the
+    // Filesystem TS we don't have a distinct __file_clock, we just use the
+    // system clock for file timestamps).
     if (s >= (nanoseconds::max().count() / 1e9))
       {
 	ec = std::make_error_code(std::errc::value_too_large); // EOVERFLOW
diff --git a/libstdc++-v3/src/filesystem/std-ops.cc b/libstdc++-v3/src/filesystem/std-ops.cc
index 26a7ad2a198..8c3ec1d9a9a 100644
--- a/libstdc++-v3/src/filesystem/std-ops.cc
+++ b/libstdc++-v3/src/filesystem/std-ops.cc
@@ -268,13 +268,32 @@ fs::copy(const path& from, const path& to, copy_options options)
 namespace std::filesystem
 {
   // Need this as there's no 'perm_options::none' enumerator.
-  inline bool is_set(fs::perm_options obj, fs::perm_options bits)
+  static inline bool is_set(fs::perm_options obj, fs::perm_options bits)
   {
     return (obj & bits) != fs::perm_options{};
   }
 }
 
 #ifdef _GLIBCXX_HAVE_SYS_STAT_H
+
+namespace
+{
+  struct internal_file_clock : fs::__file_clock
+  {
+    using __file_clock::_S_to_sys;
+    using __file_clock::_S_from_sys;
+
+    static fs::file_time_type
+    from_stat(const fs::stat_type& st, std::error_code& ec) noexcept
+    {
+      const auto sys_time = fs::file_time(st, ec);
+      if (sys_time == sys_time.min())
+	return fs::file_time_type::min();
+      return _S_from_sys(sys_time);
+    }
+  };
+}
+
 #ifdef NEED_DO_COPY_FILE
 bool
 fs::do_copy_file(const path::value_type* from, const path::value_type* to,
@@ -348,10 +367,10 @@ fs::do_copy_file(const path::value_type* from, const path::value_type* to,
 	}
       else if (options.update)
 	{
-	  const auto from_mtime = file_time(*from_st, ec);
+	  const auto from_mtime = internal_file_clock::from_stat(*from_st, ec);
 	  if (ec)
 	    return false;
-	  if ((from_mtime <= file_time(*to_st, ec)) || ec)
+	  if ((from_mtime <= internal_file_clock::from_stat(*to_st, ec)) || ec)
 	    return false;
 	}
       else if (!options.overwrite)
@@ -1122,7 +1141,10 @@ fs::last_write_time(const path& p)
 fs::file_time_type
 fs::last_write_time(const path& p, error_code& ec) noexcept
 {
-  return do_stat(p, ec, [&ec](const auto& st) { return file_time(st, ec); },
+  return do_stat(p, ec,
+		 [&ec](const auto& st) {
+		     return internal_file_clock::from_stat(st, ec);
+		 },
 		 file_time_type::min());
 }
 
@@ -1139,7 +1161,7 @@ void
 fs::last_write_time(const path& p __attribute__((__unused__)),
 		    file_time_type new_time, error_code& ec) noexcept
 {
-  auto d = new_time.time_since_epoch();
+  auto d = internal_file_clock::_S_to_sys(new_time).time_since_epoch();
   auto s = chrono::duration_cast<chrono::seconds>(d);
 #if _GLIBCXX_USE_UTIMENSAT
   auto ns = chrono::duration_cast<chrono::nanoseconds>(d - s);

Re: [PATCH] Define new filesystem::__file_clock type

[Jonathan Wakely]

On 05/01/19 20:03 +0000, Jonathan Wakely wrote:
>In C++17 the clock used for filesystem::file_time_type is unspecified,
>allowing it to be chrono::system_clock. The C++2a draft requires it to
>be a distinct type, with additional member functions to convert to/from
>other clocks (either the system clock or UTC). In order to avoid an ABI
>change later, this patch defines a new distinct type now, which will be
>used for std::chrono::file_clock later.
>
>	* include/bits/fs_fwd.h (__file_clock): Define new clock.
>	(file_time_type): Redefine in terms of __file_clock.
>	* src/filesystem/ops-common.h (file_time): Add FIXME comment about
>	overflow.
>	* src/filesystem/std-ops.cc (is_set(perm_options, perm_options)): Give
>	internal linkage.
>	(internal_file_lock): New helper type for accessing __file_clock.
>	(do_copy_file): Use internal_file_lock to convert system time to
>	file_time_type.
>	(last_write_time(const path&, error_code&)): Likewise.
>	(last_write_time(const path&, file_time_type, error_code&)): Likewise.
>
>Tested powerpc64-linux, committed to trunk.

There's a new failure on 32-bit x86:

/home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc:148: void test02(): Assertion 'approx_equal(last_write_time(f.path), time)' failed.
FAIL: 27_io/filesystem/operations/last_write_time.cc execution test

I'll deal with that ASAP.

Re: [PATCH] Define new filesystem::__file_clock type

[Christophe Lyon]

On Sun, 6 Jan 2019 at 22:45, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 05/01/19 20:03 +0000, Jonathan Wakely wrote:
> >In C++17 the clock used for filesystem::file_time_type is unspecified,
> >allowing it to be chrono::system_clock. The C++2a draft requires it to
> >be a distinct type, with additional member functions to convert to/from
> >other clocks (either the system clock or UTC). In order to avoid an ABI
> >change later, this patch defines a new distinct type now, which will be
> >used for std::chrono::file_clock later.
> >
> >       * include/bits/fs_fwd.h (__file_clock): Define new clock.
> >       (file_time_type): Redefine in terms of __file_clock.
> >       * src/filesystem/ops-common.h (file_time): Add FIXME comment about
> >       overflow.
> >       * src/filesystem/std-ops.cc (is_set(perm_options, perm_options)): Give
> >       internal linkage.
> >       (internal_file_lock): New helper type for accessing __file_clock.
> >       (do_copy_file): Use internal_file_lock to convert system time to
> >       file_time_type.
> >       (last_write_time(const path&, error_code&)): Likewise.
> >       (last_write_time(const path&, file_time_type, error_code&)): Likewise.
> >
> >Tested powerpc64-linux, committed to trunk.
>
> There's a new failure on 32-bit x86:
>
> /home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc:148: void test02(): Assertion 'approx_equal(last_write_time(f.path), time)' failed.
> FAIL: 27_io/filesystem/operations/last_write_time.cc execution test
>

I've seen the same error on arm.

> I'll deal with that ASAP.
Thanks!

Re: [PATCH] Define new filesystem::__file_clock type

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

On 06/01/19 21:45 +0000, Jonathan Wakely wrote:
>On 05/01/19 20:03 +0000, Jonathan Wakely wrote:
>>In C++17 the clock used for filesystem::file_time_type is unspecified,
>>allowing it to be chrono::system_clock. The C++2a draft requires it to
>>be a distinct type, with additional member functions to convert to/from
>>other clocks (either the system clock or UTC). In order to avoid an ABI
>>change later, this patch defines a new distinct type now, which will be
>>used for std::chrono::file_clock later.
>>
>>	* include/bits/fs_fwd.h (__file_clock): Define new clock.
>>	(file_time_type): Redefine in terms of __file_clock.
>>	* src/filesystem/ops-common.h (file_time): Add FIXME comment about
>>	overflow.
>>	* src/filesystem/std-ops.cc (is_set(perm_options, perm_options)): Give
>>	internal linkage.
>>	(internal_file_lock): New helper type for accessing __file_clock.
>>	(do_copy_file): Use internal_file_lock to convert system time to
>>	file_time_type.
>>	(last_write_time(const path&, error_code&)): Likewise.
>>	(last_write_time(const path&, file_time_type, error_code&)): Likewise.
>>
>>Tested powerpc64-linux, committed to trunk.
>
>There's a new failure on 32-bit x86:
>
>/home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc:148: void test02(): Assertion 'approx_equal(last_write_time(f.path), time)' failed.
>FAIL: 27_io/filesystem/operations/last_write_time.cc execution test

The problem here is 32-bit time_t. I've defined the file_clock epoch
as a date after 2038, so unrepresentable in a 32-bit time_t.

The test stores a value of file_time_type::zero() which is the epoch,
and so that value can't be converted to time_t for the utimensat call.

Fixed by skipping the parts of the test using the file clock's epoch.

Tested x86_64-linux (-m32 and -m64). Committed to trunk.


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 1813 bytes --]

commit 68908239cf8c8987ce4693f6769709f8f5b9fbc3
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jan 10 15:38:31 2019 +0000

    Fix filesystem::last_write_time failure with 32-bit time_t
    
            * testsuite/27_io/filesystem/operations/last_write_time.cc: Fix
            test failures on targets with 32-bit time_t.

diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc
index 7a693a1ddcb..3f31375f51b 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/last_write_time.cc
@@ -22,6 +22,7 @@
 // 15.25 Permissions [fs.op.last_write_time]
 
 #include <filesystem>
+#include <limits>
 #include <testsuite_fs.h>
 #include <testsuite_hooks.h>
 
@@ -141,14 +142,27 @@ test02()
   VERIFY( !ec );
   VERIFY( approx_equal(last_write_time(f.path), time) );
 
+  if (std::numeric_limits<std::time_t>::max()
+      < std::numeric_limits<std::int64_t>::max())
+    return; // file clock's epoch is out of range for 32-bit time_t
+
   ec = bad_ec;
+  // The file clock's epoch:
   time = time_type();
   last_write_time(f.path, time, ec);
   VERIFY( !ec );
   VERIFY( approx_equal(last_write_time(f.path), time) );
 
   ec = bad_ec;
-  time -= std::chrono::milliseconds(1000 * 60 * 10 + 15);
+  // A time after the epoch
+  time += std::chrono::milliseconds(1000 * 60 * 10 + 15);
+  last_write_time(f.path, time, ec);
+  VERIFY( !ec );
+  VERIFY( approx_equal(last_write_time(f.path), time) );
+
+  ec = bad_ec;
+  // A time before than the epoch
+  time -= std::chrono::milliseconds(1000 * 60 * 20 + 15);
   last_write_time(f.path, time, ec);
   VERIFY( !ec );
   VERIFY( approx_equal(last_write_time(f.path), time) );