[Ada] Improve precision of Ada.Directories.Modification_Time

[Ada] Improve precision of Ada.Directories.Modification_Time

[Pierre-Marie de Rodat]

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

The modification file time precision now defined by OS.

Tested on x86_64-pc-linux-gnu, committed on trunk

gcc/ada/

	* adaint.c (__gnat_file_time): New routine.
	(__gnat_copy_attribs): Copy timestamps in nanoseconds.
	* libgnat/a-direct.adb (C_Modification_Time): Bind to
	__gnat_file_time.
	(Modification_Time): Call to C_Modification_Time.

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

diff --git a/gcc/ada/adaint.c b/gcc/ada/adaint.c
--- a/gcc/ada/adaint.c
+++ b/gcc/ada/adaint.c
@@ -60,6 +60,7 @@
 /* We want to use the POSIX variants of include files.  */
 #define POSIX
 #include "vxWorks.h"
+#include <sys/time.h>
 
 #if defined (__mips_vxworks)
 #include "cacheLib.h"
@@ -1474,6 +1475,74 @@ __gnat_file_time_fd (int fd)
    return __gnat_file_time_fd_attr (fd, &attr);
 }
 
+extern long long __gnat_file_time(char* name)
+{
+  long long result;
+
+  if (name == NULL) {
+    return LLONG_MIN;
+  }
+  /* Number of seconds between <Jan 1st 1970> and <Jan 1st 2150>. */
+  static const long long ada_epoch_offset = (136 * 365 + 44 * 366) * 86400LL;
+#if defined(_WIN32)
+
+  /* Number of 100 nanoseconds between <Jan 1st 1601> and <Jan 1st 2150>. */
+  static const long long w32_epoch_offset =
+  (11644473600LL + ada_epoch_offset) * 1E7;
+
+  WIN32_FILE_ATTRIBUTE_DATA fad;
+  union
+  {
+    FILETIME ft_time;
+    long long ll_time;
+  } t_write;
+
+  if (!GetFileAttributesExA(name, GetFileExInfoStandard, &fad)) {
+    return LLONG_MIN;
+  }
+
+  t_write.ft_time = fad.ftLastWriteTime;
+
+  /* Next code similar to (t_write.ll_time - w32_epoch_offset) * 100
+     but on overflow returns LLONG_MIN value. */
+
+  if (__builtin_ssubll_overflow(t_write.ll_time, w32_epoch_offset, &result)) {
+    return LLONG_MIN;
+  }
+
+  if (__builtin_smulll_overflow(result, 100, &result)) {
+    return LLONG_MIN;
+  }
+
+#else
+
+  struct stat sb;
+  if (stat(name, &sb) != 0) {
+    return LLONG_MIN;
+  }
+
+  /* Next code similar to
+     (sb.st_mtime - ada_epoch_offset) * 1E9 + sb.st_mtim.tv_nsec
+     but on overflow returns LLONG_MIN value. */
+
+  if (__builtin_ssubll_overflow(sb.st_mtime, ada_epoch_offset, &result)) {
+    return LLONG_MIN;
+  }
+
+  if (__builtin_smulll_overflow(result, 1E9, &result)) {
+    return LLONG_MIN;
+  }
+
+#if defined(st_mtime)
+  if (__builtin_saddll_overflow(result, sb.st_mtim.tv_nsec, &result)) {
+    return LLONG_MIN;
+  }
+#endif
+
+#endif
+  return result;
+}
+
 /* Set the file time stamp.  */
 
 void
@@ -3173,22 +3242,45 @@ __gnat_copy_attribs (char *from ATTRIBUTE_UNUSED, char *to ATTRIBUTE_UNUSED,
 
 #else
   GNAT_STRUCT_STAT fbuf;
-  struct utimbuf tbuf;
 
   if (GNAT_STAT (from, &fbuf) == -1) {
      return -1;
   }
 
-  /* Do we need to copy timestamp ? */
+#if _POSIX_C_SOURCE >= 200809L
+  struct timespec tbuf[2];
+
   if (mode != 2) {
-     tbuf.actime = fbuf.st_atime;
-     tbuf.modtime = fbuf.st_mtime;
+     tbuf[0] = fbuf.st_atim;
+     tbuf[1] = fbuf.st_mtim;
 
-     if (utime (to, &tbuf) == -1) {
+     if (utimensat (AT_FDCWD, to, tbuf, 0) == -1) {
         return -1;
      }
   }
 
+#else
+  struct timeval tbuf[2];
+  /* Do we need to copy timestamp ? */
+
+  if (mode != 2) {
+     tbuf[0].tv_sec  = fbuf.st_atime;
+     tbuf[1].tv_sec  = fbuf.st_mtime;
+
+     #if defined(st_mtime)
+     tbuf[0].tv_usec = fbuf.st_atim.tv_nsec / 1000;
+     tbuf[1].tv_usec = fbuf.st_mtim.tv_nsec / 1000;
+     #else
+     tbuf[0].tv_usec = 0;
+     tbuf[1].tv_usec = 0;
+     #endif
+
+     if (utimes (to, tbuf) == -1) {
+        return -1;
+     }
+  }
+#endif
+
   /* Do we need to copy file permissions ? */
   if (mode != 0 && (chmod (to, fbuf.st_mode) == -1)) {
 	  return -1;


diff --git a/gcc/ada/libgnat/a-direct.adb b/gcc/ada/libgnat/a-direct.adb
--- a/gcc/ada/libgnat/a-direct.adb
+++ b/gcc/ada/libgnat/a-direct.adb
@@ -30,7 +30,6 @@
 ------------------------------------------------------------------------------
 
 with Ada.Calendar;               use Ada.Calendar;
-with Ada.Calendar.Formatting;    use Ada.Calendar.Formatting;
 with Ada.Characters.Handling;    use Ada.Characters.Handling;
 with Ada.Directories.Validity;   use Ada.Directories.Validity;
 with Ada.Directories.Hierarchical_File_Names;
@@ -70,6 +69,15 @@ package body Ada.Directories is
    pragma Import (C, Max_Path, "__gnat_max_path_len");
    --  The maximum length of a path
 
+   function C_Modification_Time (N : System.Address) return Ada.Calendar.Time;
+   pragma Import (C, C_Modification_Time, "__gnat_file_time");
+   --  Get modification time for file with name referenced by N
+
+   Invalid_Time : constant Ada.Calendar.Time :=
+                    C_Modification_Time (System.Null_Address);
+   --  Result returned from C_Modification_Time call when routine unable to get
+   --  file modification time.
+
    type Search_Data is record
       Is_Valid      : Boolean := False;
       Name          : Unbounded_String;
@@ -991,14 +999,9 @@ package body Ada.Directories is
    -----------------------
 
    function Modification_Time (Name : String) return Time is
-      Date   : OS_Time;
-      Year   : Year_Type;
-      Month  : Month_Type;
-      Day    : Day_Type;
-      Hour   : Hour_Type;
-      Minute : Minute_Type;
-      Second : Second_Type;
 
+      Date   : Time;
+      C_Name : aliased String (1 .. Name'Length + 1);
    begin
       --  First, the invalid cases
 
@@ -1006,19 +1009,15 @@ package body Ada.Directories is
          raise Name_Error with '"' & Name & """ not a file or directory";
 
       else
-         Date := File_Time_Stamp (Name);
-
-         --  Break down the time stamp into its constituents relative to GMT.
-         --  This version of Split does not recognize leap seconds or buffer
-         --  space for time zone processing.
+         C_Name := Name & ASCII.NUL;
+         Date := C_Modification_Time (C_Name'Address);
 
-         GM_Split (Date, Year, Month, Day, Hour, Minute, Second);
-
-         --  The result must be in GMT. Ada.Calendar.
-         --  Formatting.Time_Of with default time zone of zero (0) is the
-         --  routine of choice.
+         if Date = Invalid_Time then
+            raise Use_Error with
+              "Unable to get modification time of the file """ & Name & '"';
+         end if;
 
-         return Time_Of (Year, Month, Day, Hour, Minute, Second, 0.0);
+         return Date;
       end if;
    end Modification_Time;
 

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Iain Sandoe]

Hi Folks,

This patch breaks bootstrap on Darwin platforms.

Pierre-Marie de Rodat <derodat@adacore.com> wrote:

> The modification file time precision now defined by OS.
> 
> Tested on x86_64-pc-linux-gnu, committed on trunk
> 
> gcc/ada/
> 
> 	* adaint.c (__gnat_file_time): New routine.
> 	(__gnat_copy_attribs): Copy timestamps in nanoseconds.
> 	* libgnat/a-direct.adb (C_Modification_Time): Bind to
> 	__gnat_file_time.
> 	(Modification_Time): Call to C_Modification_Time.<patch.diff>

#if defined(st_mtime)

is a necessary test - but the fields in the stat structure on Darwin platforms are
named st_{a,c,m}timespec rather than the Linux st_{a,c,m}tim.

The following patch is a fix lightly tested,
OK for master (if remaining testing is successful) or you have an alternate suggestion?

thanks
Iain

diff --git a/gcc/ada/adaint.c b/gcc/ada/adaint.c
index b7406a03c31..ac5738a60d2 100644
--- a/gcc/ada/adaint.c
+++ b/gcc/ada/adaint.c
@@ -1528,8 +1528,12 @@ extern long long __gnat_file_time(char* name)
 #if defined(__GNUG__) && __GNUG__ <= 4
     result = (sb.st_mtime - ada_epoch_offset) * 1E9;
 #if defined(st_mtime)
+#if __APPLE__
+    result += sb.st_mtimespec.tv_nsec;
+#else
     result += sb.st_mtim.tv_nsec;
 #endif
+#endif
 #else
   /* Next code similar to
      (sb.st_mtime - ada_epoch_offset) * 1E9 + sb.st_mtim.tv_nsec
@@ -1544,11 +1548,17 @@ extern long long __gnat_file_time(char* name)
   }
 
 #if defined(st_mtime)
+#if __APPLE__
+  if (__builtin_saddll_overflow(result, sb.st_mtimespec.tv_nsec, &result)) {
+    return LLONG_MIN;
+  }
+#else
   if (__builtin_saddll_overflow(result, sb.st_mtim.tv_nsec, &result)) {
     return LLONG_MIN;
   }
 #endif
 #endif
+#endif
 #endif
   return result;
 }
@@ -3278,8 +3288,13 @@ __gnat_copy_attribs (char *from ATTRIBUTE_UNUSED, char *to ATTRIBUTE_UNUSED,
      tbuf[1].tv_sec  = fbuf.st_mtime;
 
      #if defined(st_mtime)
+     #if __APPLE__
+     tbuf[0].tv_usec = fbuf.st_atimespec.tv_nsec / 1000;
+     tbuf[1].tv_usec = fbuf.st_mtimespec.tv_nsec / 1000;
+     #else
      tbuf[0].tv_usec = fbuf.st_atim.tv_nsec / 1000;
      tbuf[1].tv_usec = fbuf.st_mtim.tv_nsec / 1000;
+     #endif
      #else
      tbuf[0].tv_usec = 0;
      tbuf[1].tv_usec = 0;

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Arnaud Charlet]

> This patch breaks bootstrap on Darwin platforms.
> 
> Pierre-Marie de Rodat <derodat@adacore.com> wrote:
> 
> > The modification file time precision now defined by OS.
> > 
> > Tested on x86_64-pc-linux-gnu, committed on trunk
> > 
> > gcc/ada/
> > 
> > 	* adaint.c (__gnat_file_time): New routine.
> > 	(__gnat_copy_attribs): Copy timestamps in nanoseconds.
> > 	* libgnat/a-direct.adb (C_Modification_Time): Bind to
> > 	__gnat_file_time.
> > 	(Modification_Time): Call to C_Modification_Time.<patch.diff>
> 
> #if defined(st_mtime)
> 
> is a necessary test - but the fields in the stat structure on Darwin platforms are
> named st_{a,c,m}timespec rather than the Linux st_{a,c,m}tim.

What about instead putting above extern long long __gnat_file_time the
following:

#if __APPLE__
#define st_mtim st_mtimespec
#define st_atim st_atimespec
#endif

To avoid having the two (nested) #if __APPLE__ and keep the code easier
to follow?

Arno

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Iain Sandoe]

Arnaud Charlet <charlet@adacore.com> wrote:

>> This patch breaks bootstrap on Darwin platforms.
>>
>> Pierre-Marie de Rodat <derodat@adacore.com> wrote:
>>
>>> The modification file time precision now defined by OS.
>>>
>>> Tested on x86_64-pc-linux-gnu, committed on trunk
>>>
>>> gcc/ada/
>>>
>>> 	* adaint.c (__gnat_file_time): New routine.
>>> 	(__gnat_copy_attribs): Copy timestamps in nanoseconds.
>>> 	* libgnat/a-direct.adb (C_Modification_Time): Bind to
>>> 	__gnat_file_time.
>>> 	(Modification_Time): Call to C_Modification_Time.<patch.diff>
>>
>> #if defined(st_mtime)
>>
>> is a necessary test - but the fields in the stat structure on Darwin  
>> platforms are
>> named st_{a,c,m}timespec rather than the Linux st_{a,c,m}tim.
>
> What about instead putting above extern long long __gnat_file_time the
> following:
>
> #if __APPLE__
> #define st_mtim st_mtimespec
> #define st_atim st_atimespec
> #endif
>
> To avoid having the two (nested) #if __APPLE__ and keep the code easier
> to follow?

works for me (the test patch was drafted quickly to allow bootstrap to  
continue)
- I can amend the patch and (re-)test more widely.

Iain

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Arnaud Charlet]

> >What about instead putting above extern long long __gnat_file_time the
> >following:
> >
> >#if __APPLE__
> >#define st_mtim st_mtimespec
> >#define st_atim st_atimespec
> >#endif
> >
> >To avoid having the two (nested) #if __APPLE__ and keep the code easier

two => three :-)

> >to follow?
> 
> works for me (the test patch was drafted quickly to allow bootstrap
> to continue)
> - I can amend the patch and (re-)test more widely.

OK then with these changes, assuming successfully build/test.

Arno

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Hans-Peter Nilsson]

On Wed, 21 Oct 2020, Iain Sandoe wrote:

> Arnaud Charlet <charlet@adacore.com> wrote:
>
> > > This patch breaks bootstrap on Darwin platforms.
> > >
> > > Pierre-Marie de Rodat <derodat@adacore.com> wrote:
> > >
> > > > The modification file time precision now defined by OS.
> > > >
> > > > Tested on x86_64-pc-linux-gnu, committed on trunk
> > > >
> > > > gcc/ada/
> > > >
> > > > 	* adaint.c (__gnat_file_time): New routine.
> > > > 	(__gnat_copy_attribs): Copy timestamps in nanoseconds.
> > > > 	* libgnat/a-direct.adb (C_Modification_Time): Bind to
> > > > 	__gnat_file_time.
> > > > 	(Modification_Time): Call to C_Modification_Time.<patch.diff>
> > >
> > > #if defined(st_mtime)
> > >
> > > is a necessary test - but the fields in the stat structure on Darwin
> > > platforms are
> > > named st_{a,c,m}timespec rather than the Linux st_{a,c,m}tim.
> >
> > What about instead putting above extern long long __gnat_file_time the
> > following:
> >
> > #if __APPLE__
> > #define st_mtim st_mtimespec
> > #define st_atim st_atimespec
> > #endif
> >
> > To avoid having the two (nested) #if __APPLE__ and keep the code easier
> > to follow?
>
> works for me (the test patch was drafted quickly to allow bootstrap to
> continue)
> - I can amend the patch and (re-)test more widely.
>
> Iain

For future reference, TRT for this kind of problem is to
autoconf for the right struct field name, using AC_CHECK_MEMBER
or AC_CHECK_MEMBERS (then use e.g. #if HAVE_STAT_ST_MTIM / #if
HAVE_STAT_ST_MTIMESPEC, definitely not #if __APPLE__).

brgds, H-P

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Iain Sandoe]

Hans-Peter Nilsson <hp@bitrange.com> wrote:

> On Wed, 21 Oct 2020, Iain Sandoe wrote:
> 
>> Arnaud Charlet <charlet@adacore.com> wrote:
>> 
>>>> This patch breaks bootstrap on Darwin platforms.
>>>> 
>>>> Pierre-Marie de Rodat <derodat@adacore.com> wrote:
>>>> 
>>>>> The modification file time precision now defined by OS.
>>>>> 
>>>>> Tested on x86_64-pc-linux-gnu, committed on trunk
>>>>> 
>>>>> gcc/ada/
>>>>> 
>>>>> 	* adaint.c (__gnat_file_time): New routine.
>>>>> 	(__gnat_copy_attribs): Copy timestamps in nanoseconds.
>>>>> 	* libgnat/a-direct.adb (C_Modification_Time): Bind to
>>>>> 	__gnat_file_time.
>>>>> 	(Modification_Time): Call to C_Modification_Time.<patch.diff>
>>>> 
>>>> #if defined(st_mtime)
>>>> 
>>>> is a necessary test - but the fields in the stat structure on Darwin
>>>> platforms are
>>>> named st_{a,c,m}timespec rather than the Linux st_{a,c,m}tim.
>>> 
>>> What about instead putting above extern long long __gnat_file_time the
>>> following:
>>> 
>>> #if __APPLE__
>>> #define st_mtim st_mtimespec
>>> #define st_atim st_atimespec
>>> #endif
>>> 
>>> To avoid having the two (nested) #if __APPLE__ and keep the code easier
>>> to follow?
>> 
>> works for me (the test patch was drafted quickly to allow bootstrap to
>> continue)
>> - I can amend the patch and (re-)test more widely.
>> 
>> Iain
> 
> For future reference, TRT for this kind of problem is to
> autoconf for the right struct field name, using AC_CHECK_MEMBER
> or AC_CHECK_MEMBERS (then use e.g. #if HAVE_STAT_ST_MTIM / #if
> HAVE_STAT_ST_MTIMESPEC, definitely not #if __APPLE__).

I’m not diasgreeing with your technical comment; now I am in a difficult position.
I don’t have resources at the moment to make the changes you suggest, and
Darwin is bootstrap-broken (at least for Ada).

so .. the attached is a workaround - now I’ve resolved the second bootstrap issue
on powerpc-darwin, I was able to test it more widely.

I have *not* applied the patch, pending a “proper” solution at some unspecified
time in the future .. 

Iain


======

Darwin has timeval entries for file access and modification
times but they are named differently from those on Linux (and
presumably everywhere else, since no other platform has
reported an issue).
---
 gcc/ada/adaint.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gcc/ada/adaint.c b/gcc/ada/adaint.c
index b7406a03c31..560f3529442 100644
--- a/gcc/ada/adaint.c
+++ b/gcc/ada/adaint.c
@@ -237,6 +237,11 @@ UINT __gnat_current_ccs_encoding;
 
 #include "adaint.h"
 
+#if defined (__APPLE__) && defined (st_mtime)
+#define st_atim st_atimespec
+#define st_mtim st_mtimespec
+#endif
+
 /* Define symbols O_BINARY and O_TEXT as harmless zeroes if they are not
    defined in the current system. On DOS-like systems these flags control
    whether the file is opened/created in text-translation mode (CR/LF in
-- 
2.24.1

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Arnaud Charlet]

> > For future reference, TRT for this kind of problem is to
> > autoconf for the right struct field name, using AC_CHECK_MEMBER
> > or AC_CHECK_MEMBERS (then use e.g. #if HAVE_STAT_ST_MTIM / #if
> > HAVE_STAT_ST_MTIMESPEC, definitely not #if __APPLE__).
> 
> I'm not diasgreeing with your technical comment; now I am in a difficult position.
> I don't have resources at the moment to make the changes you suggest, and
> Darwin is bootstrap-broken (at least for Ada).
> 
> so .. the attached is a workaround - now I've resolved the second bootstrap issue
> on powerpc-darwin, I was able to test it more widely.
> 
> I have *not* applied the patch, pending a 'proper' solution at some unspecified
> time in the future .. 

Your patch is OK, the "proper" solution is unlikely to appear anytime soon,
so let's not have best be the enemy of good (enough).

Arno

> Darwin has timeval entries for file access and modification
> times but they are named differently from those on Linux (and
> presumably everywhere else, since no other platform has
> reported an issue).
> ---
>  gcc/ada/adaint.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/gcc/ada/adaint.c b/gcc/ada/adaint.c
> index b7406a03c31..560f3529442 100644
> --- a/gcc/ada/adaint.c
> +++ b/gcc/ada/adaint.c
> @@ -237,6 +237,11 @@ UINT __gnat_current_ccs_encoding;
>  
>  #include "adaint.h"
>  
> +#if defined (__APPLE__) && defined (st_mtime)
> +#define st_atim st_atimespec
> +#define st_mtim st_mtimespec
> +#endif
> +
>  /* Define symbols O_BINARY and O_TEXT as harmless zeroes if they are not
>     defined in the current system. On DOS-like systems these flags control
>     whether the file is opened/created in text-translation mode (CR/LF in

Re: [Ada] Improve precision of Ada.Directories.Modification_Time

[Hans-Peter Nilsson]

On Fri, 23 Oct 2020, Arnaud Charlet wrote:

> > > For future reference, TRT for this kind of problem is to
> > > autoconf for the right struct field name, using AC_CHECK_MEMBER
> > > or AC_CHECK_MEMBERS (then use e.g. #if HAVE_STAT_ST_MTIM / #if
> > > HAVE_STAT_ST_MTIMESPEC, definitely not #if __APPLE__).
> >
> > I'm not diasgreeing with your technical comment; now I am in a difficult position.
> > I don't have resources at the moment to make the changes you suggest, and
> > Darwin is bootstrap-broken (at least for Ada).
> >
> > so .. the attached is a workaround - now I've resolved the second bootstrap issue
> > on powerpc-darwin, I was able to test it more widely.
> >
> > I have *not* applied the patch, pending a 'proper' solution at some unspecified
> > time in the future ..
>
> Your patch is OK, the "proper" solution is unlikely to appear anytime soon,
> so let's not have best be the enemy of good (enough).
>
> Arno

Absolutely, hence my "for future reference"; not a suggestion
to put resources to *this* instance.

brgds, H-P


>
> > Darwin has timeval entries for file access and modification
> > times but they are named differently from those on Linux (and
> > presumably everywhere else, since no other platform has
> > reported an issue).
> > ---
> >  gcc/ada/adaint.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/gcc/ada/adaint.c b/gcc/ada/adaint.c
> > index b7406a03c31..560f3529442 100644
> > --- a/gcc/ada/adaint.c
> > +++ b/gcc/ada/adaint.c
> > @@ -237,6 +237,11 @@ UINT __gnat_current_ccs_encoding;
> >
> >  #include "adaint.h"
> >
> > +#if defined (__APPLE__) && defined (st_mtime)
> > +#define st_atim st_atimespec
> > +#define st_mtim st_mtimespec
> > +#endif
> > +
> >  /* Define symbols O_BINARY and O_TEXT as harmless zeroes if they are not
> >     defined in the current system. On DOS-like systems these flags control
> >     whether the file is opened/created in text-translation mode (CR/LF in
>