[PATCH] Make st_*tim visible in stat for POSIX.1-2008

[PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Dionna Amalie Glaze via newlib]

The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 newlib/libc/include/sys/stat.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/include/sys/stat.h b/newlib/libc/include/sys/stat.h
index eee98db64..052ef5a66 100644
--- a/newlib/libc/include/sys/stat.h
+++ b/newlib/libc/include/sys/stat.h
@@ -34,10 +34,12 @@ struct      stat
   gid_t                st_gid;
   dev_t                st_rdev;
   off_t                st_size;
-#if defined(__rtems__)
+#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809
   struct timespec st_atim;
   struct timespec st_mtim;
   struct timespec st_ctim;
+#endif
+#if defined(__rtems__)
   blksize_t     st_blksize;
   blkcnt_t     st_blocks;
 #else
@@ -60,7 +62,7 @@ struct        stat
 #endif
 };

-#if defined(__rtems__)
+#if __POSIX_VISIBLE >= 200809
 #define st_atime st_atim.tv_sec
 #define st_ctime st_ctim.tv_sec
 #define st_mtime st_mtim.tv_sec

-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Joel Sherrill]

On Tue, Aug 13, 2019 at 1:30 PM Dionna Amalie Glaze via newlib <
newlib@sourceware.org> wrote:

> The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  newlib/libc/include/sys/stat.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/newlib/libc/include/sys/stat.h
> b/newlib/libc/include/sys/stat.h
> index eee98db64..052ef5a66 100644
> --- a/newlib/libc/include/sys/stat.h
> +++ b/newlib/libc/include/sys/stat.h
> @@ -34,10 +34,12 @@ struct      stat
>    gid_t                st_gid;
>    dev_t                st_rdev;
>    off_t                st_size;
> -#if defined(__rtems__)
> +#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809
>    struct timespec st_atim;
>    struct timespec st_mtim;
>    struct timespec st_ctim;
> +#endif
>

If I am reading this change correctly, this is breakage for RTEMS. Both of
those
terms are defined as a consequence of user provided defines. If the user
application
and RTEMS are not compiled with the same options, the fields in the
structure will
not agree and these below will be overlaid on the time fields.

Most of the newlib targets are single address space so changing the fields
in
the structure based on user provided conditionals is going to introduce
breakage.

Perhaps adding a new define to sys/config.h so the fields are always there
for
the targets that support them. I couldn't find any example that wasn't
based on
a hard-coded always on value in either sys/config.h or sys/features.h. Maybe
someone else has another idea.

Preprocessing this test program confirms that __POSIX_VISIBLE isn't defined
by default for RTEMS.

=========================
#include <sys/stat.h>

int X = __POSIX_VISIBLE;
=========================




> +#if defined(__rtems__)
>    blksize_t     st_blksize;
>    blkcnt_t     st_blocks;
>  #else
> @@ -60,7 +62,7 @@ struct        stat
>  #endif
>  };
>



>
> -#if defined(__rtems__)
> +#if __POSIX_VISIBLE >= 200809
>  #define st_atime st_atim.tv_sec
>  #define st_ctime st_ctim.tv_sec
>  #define st_mtime st_mtim.tv_sec
>
> --
> -Dionna Glaze, PhD (she/her)
>

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Corinna Vinschen]

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

On Aug 13 13:46, Joel Sherrill wrote:
> On Tue, Aug 13, 2019 at 1:30 PM Dionna Amalie Glaze via newlib <
> newlib@sourceware.org> wrote:
> 
> > The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.
> >
> > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > ---
> >  newlib/libc/include/sys/stat.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/newlib/libc/include/sys/stat.h
> > b/newlib/libc/include/sys/stat.h
> > index eee98db64..052ef5a66 100644
> > --- a/newlib/libc/include/sys/stat.h
> > +++ b/newlib/libc/include/sys/stat.h
> > @@ -34,10 +34,12 @@ struct      stat
> >    gid_t                st_gid;
> >    dev_t                st_rdev;
> >    off_t                st_size;
> > -#if defined(__rtems__)
> > +#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809
> >    struct timespec st_atim;
> >    struct timespec st_mtim;
> >    struct timespec st_ctim;
> > +#endif
> >
> 
> If I am reading this change correctly, this is breakage for RTEMS. Both of
> those
> terms are defined as a consequence of user provided defines. If the user
> application
> and RTEMS are not compiled with the same options, the fields in the
> structure will
> not agree and these below will be overlaid on the time fields.
> 
> Most of the newlib targets are single address space so changing the fields
> in
> the structure based on user provided conditionals is going to introduce
> breakage.
> 
> Perhaps adding a new define to sys/config.h so the fields are always there
> for
> the targets that support them. I couldn't find any example that wasn't
> based on
> a hard-coded always on value in either sys/config.h or sys/features.h. Maybe
> someone else has another idea.

I'm not sure why the timestamps are not defined for non-rtems
targets but I guess it's all about size again.  Don't we have
an already existing definition for small targets we can use here?
_REENT_SMALL or something like that?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Dionna Amalie Glaze via newlib]

The timestamps are defined for non-rtems targets, it's just not in the
patch. The #else shows that they are defined as st_{a,m,c}time instead of
st_{a,m,c}tim with later #defines for st_([amc])time as st_\1tim.tv_sec.
This patch is just about having both field names available: st_{a,m,c}tim
and st_{a,m,c}time. It's a weird compatibility issue I'm hitting.

On Wed, Aug 14, 2019 at 1:41 AM Corinna Vinschen <vinschen@redhat.com>
wrote:

> On Aug 13 13:46, Joel Sherrill wrote:
> > On Tue, Aug 13, 2019 at 1:30 PM Dionna Amalie Glaze via newlib <
> > newlib@sourceware.org> wrote:
> >
> > > The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.
> > >
> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > > ---
> > >  newlib/libc/include/sys/stat.h | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/newlib/libc/include/sys/stat.h
> > > b/newlib/libc/include/sys/stat.h
> > > index eee98db64..052ef5a66 100644
> > > --- a/newlib/libc/include/sys/stat.h
> > > +++ b/newlib/libc/include/sys/stat.h
> > > @@ -34,10 +34,12 @@ struct      stat
> > >    gid_t                st_gid;
> > >    dev_t                st_rdev;
> > >    off_t                st_size;
> > > -#if defined(__rtems__)
> > > +#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809
> > >    struct timespec st_atim;
> > >    struct timespec st_mtim;
> > >    struct timespec st_ctim;
> > > +#endif
> > >
> >
> > If I am reading this change correctly, this is breakage for RTEMS. Both
> of
> > those
> > terms are defined as a consequence of user provided defines. If the user
> > application
> > and RTEMS are not compiled with the same options, the fields in the
> > structure will
> > not agree and these below will be overlaid on the time fields.
> >
> > Most of the newlib targets are single address space so changing the
> fields
> > in
> > the structure based on user provided conditionals is going to introduce
> > breakage.
> >
> > Perhaps adding a new define to sys/config.h so the fields are always
> there
> > for
> > the targets that support them. I couldn't find any example that wasn't
> > based on
> > a hard-coded always on value in either sys/config.h or sys/features.h.
> Maybe
> > someone else has another idea.
>
> I'm not sure why the timestamps are not defined for non-rtems
> targets but I guess it's all about size again.  Don't we have
> an already existing definition for small targets we can use here?
> _REENT_SMALL or something like that?
>
>
> Corinna
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
>


-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Yaakov Selkowitz]

On Tue, 2019-08-13 at 11:29 -0700, Dionna Amalie Glaze via newlib
wrote:
> The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  newlib/libc/include/sys/stat.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/newlib/libc/include/sys/stat.h b/newlib/libc/include/sys/stat.h
> index eee98db64..052ef5a66 100644
> --- a/newlib/libc/include/sys/stat.h
> +++ b/newlib/libc/include/sys/stat.h
> @@ -34,10 +34,12 @@ struct      stat
>    gid_t                st_gid;
>    dev_t                st_rdev;
>    off_t                st_size;
> -#if defined(__rtems__)
> +#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809

Nak.

1) __USE_MISC is a glibc internal and has no meaning within Newlib. 
The proper guards must be used.

2) This would cause the struct size to vary based on FTMs.  This is a
big no-no.  There needs to be size equivalent no-ops in an #else chunk
to match this.

>    struct timespec st_atim;
>    struct timespec st_mtim;
>    struct timespec st_ctim;
> +#endif
> +#if defined(__rtems__)
>    blksize_t     st_blksize;
>    blkcnt_t     st_blocks;
>  #else
> @@ -60,7 +62,7 @@ struct        stat
>  #endif
>  };
> 
> -#if defined(__rtems__)
> +#if __POSIX_VISIBLE >= 200809
>  #define st_atime st_atim.tv_sec
>  #define st_ctime st_ctim.tv_sec
>  #define st_mtime st_mtim.tv_sec
> 

-- 
Yaakov Selkowitz
Senior Software Engineer - Platform Enablement
Red Hat, Inc.

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Dionna Amalie Glaze via newlib]

Ah you're right, I didn't properly handle the block size definitions.
Here's another go at it that splits the timestamp declarations from the
blocksize declarations.

---
 newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/newlib/libc/include/sys/stat.h b/newlib/libc/include/sys/stat.h
index eee98db64..4a05081e4 100644
--- a/newlib/libc/include/sys/stat.h
+++ b/newlib/libc/include/sys/stat.h
@@ -24,7 +24,7 @@ extern "C" {
 #define stat64 stat
 #endif
 #else
-struct stat
+struct stat
 {
   dev_t                st_dev;
   ino_t                st_ino;
@@ -34,15 +34,11 @@ struct      stat
   gid_t                st_gid;
   dev_t                st_rdev;
   off_t                st_size;
-#if defined(__rtems__)
+#if defined(__MISC_VISIBLE) || __POSIX_VISIBLE >= 200809 ||
defined(__rtems__)
   struct timespec st_atim;
   struct timespec st_mtim;
   struct timespec st_ctim;
-  blksize_t     st_blksize;
-  blkcnt_t     st_blocks;
-#else
-  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
-#if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
+#elif defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
   time_t       st_atime;
   time_t       st_mtime;
   time_t       st_ctime;
@@ -53,14 +49,19 @@ struct      stat
   long         st_spare2;
   time_t       st_ctime;
   long         st_spare3;
+#endif
+#if defined(__rtems__)
+  blksize_t     st_blksize;
+  blkcnt_t     st_blocks;
+/* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
+#elif !defined(__svr4__) || defined(__PPC__) || defined(__sun__)
   blksize_t    st_blksize;
   blkcnt_t     st_blocks;
   long st_spare4[2];
 #endif
-#endif
};

-#if defined(__rtems__)
+#if __POSIX_VISIBLE >= 200809 || defined(__rtems__)

On Wed, Aug 14, 2019 at 11:53 AM Yaakov Selkowitz <yselkowi@redhat.com>
wrote:

> On Tue, 2019-08-13 at 11:29 -0700, Dionna Amalie Glaze via newlib
> wrote:
> > The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.
> >
> > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > ---
> >  newlib/libc/include/sys/stat.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/newlib/libc/include/sys/stat.h
> b/newlib/libc/include/sys/stat.h
> > index eee98db64..052ef5a66 100644
> > --- a/newlib/libc/include/sys/stat.h
> > +++ b/newlib/libc/include/sys/stat.h
> > @@ -34,10 +34,12 @@ struct      stat
> >    gid_t                st_gid;
> >    dev_t                st_rdev;
> >    off_t                st_size;
> > -#if defined(__rtems__)
> > +#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809
>
> Nak.
>
> 1) __USE_MISC is a glibc internal and has no meaning within Newlib.
> The proper guards must be used.
>
> 2) This would cause the struct size to vary based on FTMs.  This is a
> big no-no.  There needs to be size equivalent no-ops in an #else chunk
> to match this.
>
> >    struct timespec st_atim;
> >    struct timespec st_mtim;
> >    struct timespec st_ctim;
> > +#endif
> > +#if defined(__rtems__)
> >    blksize_t     st_blksize;
> >    blkcnt_t     st_blocks;
> >  #else
> > @@ -60,7 +62,7 @@ struct        stat
> >  #endif
> >  };
> >
> > -#if defined(__rtems__)
> > +#if __POSIX_VISIBLE >= 200809
> >  #define st_atime st_atim.tv_sec
> >  #define st_ctime st_ctim.tv_sec
> >  #define st_mtime st_mtim.tv_sec
> >
>
> --
> Yaakov Selkowitz
> Senior Software Engineer - Platform Enablement
> Red Hat, Inc.
>
>
>

-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Yaakov Selkowitz]

On Wed, 2019-08-14 at 12:06 -0700, Dionna Amalie Glaze wrote:
> Ah you're right, I didn't properly handle the block size definitions.
> Here's another go at it that splits the timestamp declarations from the
> blocksize declarations.
> 
> ---
>  newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/newlib/libc/include/sys/stat.h b/newlib/libc/include/sys/stat.h
> index eee98db64..4a05081e4 100644
> --- a/newlib/libc/include/sys/stat.h
> +++ b/newlib/libc/include/sys/stat.h
> @@ -24,7 +24,7 @@ extern "C" {
>  #define stat64 stat
>  #endif
>  #else
> -struct stat
> +struct stat
>  {
>    dev_t                st_dev;
>    ino_t                st_ino;
> @@ -34,15 +34,11 @@ struct      stat
>    gid_t                st_gid;
>    dev_t                st_rdev;
>    off_t                st_size;
> -#if defined(__rtems__)
> +#if defined(__MISC_VISIBLE) || __POSIX_VISIBLE >= 200809 || defined(__rtems__)

The __*_VISIBLE macros are *always* defined, question is to what.   
These must therefore be used as #if __MISC_VISIBLE etc.

>    struct timespec st_atim;
>    struct timespec st_mtim;
>    struct timespec st_ctim;
> -  blksize_t     st_blksize;
> -  blkcnt_t     st_blocks;
> -#else
> -  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
> -#if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
> +#elif defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)

What defined __srv4__ that should trigger this section?  It probably
needs to be moved before the #if __MISC_VISIBLE etc. hunk.

>    time_t       st_atime;
>    time_t       st_mtime;
>    time_t       st_ctime;
> @@ -53,14 +49,19 @@ struct      stat
>    long         st_spare2;
>    time_t       st_ctime;
>    long         st_spare3;
> +#endif
> +#if defined(__rtems__)
> +  blksize_t     st_blksize;
> +  blkcnt_t     st_blocks;
> +/* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
> +#elif !defined(__svr4__) || defined(__PPC__) || defined(__sun__)
>    blksize_t    st_blksize;
>    blkcnt_t     st_blocks;
>    long st_spare4[2];
>  #endif
> -#endif
> };
> 
> -#if defined(__rtems__)
> +#if __POSIX_VISIBLE >= 200809 || defined(__rtems__)
> 
> On Wed, Aug 14, 2019 at 11:53 AM Yaakov Selkowitz wrote:
> > On Tue, 2019-08-13 at 11:29 -0700, Dionna Amalie Glaze via newlib
> > wrote:
> > > The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.
> > > 
> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > > ---
> > >  newlib/libc/include/sys/stat.h | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/newlib/libc/include/sys/stat.h
> > b/newlib/libc/include/sys/stat.h
> > > index eee98db64..052ef5a66 100644
> > > --- a/newlib/libc/include/sys/stat.h
> > > +++ b/newlib/libc/include/sys/stat.h
> > > @@ -34,10 +34,12 @@ struct      stat
> > >    gid_t                st_gid;
> > >    dev_t                st_rdev;
> > >    off_t                st_size;
> > > -#if defined(__rtems__)
> > > +#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809
> > 
> > Nak.
> > 
> > 1) __USE_MISC is a glibc internal and has no meaning within Newlib.
> > The proper guards must be used.
> > 
> > 2) This would cause the struct size to vary based on FTMs.  This is a
> > big no-no.  There needs to be size equivalent no-ops in an #else chunk
> > to match this.
> > 
> > >    struct timespec st_atim;
> > >    struct timespec st_mtim;
> > >    struct timespec st_ctim;
> > > +#endif
> > > +#if defined(__rtems__)
> > >    blksize_t     st_blksize;
> > >    blkcnt_t     st_blocks;
> > >  #else
> > > @@ -60,7 +62,7 @@ struct        stat
> > >  #endif
> > >  };
> > > 
> > > -#if defined(__rtems__)
> > > +#if __POSIX_VISIBLE >= 200809
> > >  #define st_atime st_atim.tv_sec
> > >  #define st_ctime st_ctim.tv_sec
> > >  #define st_mtime st_mtim.tv_sec

-- 
Yaakov Selkowitz
Senior Software Engineer - Platform Enablement
Red Hat, Inc.

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Dionna Amalie Glaze via newlib]

Fixed the __MISC_VISIBLE part.
I'm not sure I understand your question. I'm just restructuring how that
code gets exposed. Where previously the timespec and blocks were defined if
rtems, and otherwise just the timespec if srv4 etc, I've changed the
timespec declarations to all be grouped together. The block declarations
are separate because only the #else after defined(__rtems__) is evaluated
false and `defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)` is
evaluated false.

---
 newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/newlib/libc/include/sys/stat.h b/newlib/libc/include/sys/stat.h
index eee98db64..d7d08e830 100644
--- a/newlib/libc/include/sys/stat.h
+++ b/newlib/libc/include/sys/stat.h
@@ -24,7 +24,7 @@ extern "C" {
 #define stat64 stat
 #endif
 #else
-struct stat
+struct stat
 {
   dev_t                st_dev;
   ino_t                st_ino;
@@ -34,15 +34,11 @@ struct      stat
   gid_t                st_gid;
   dev_t                st_rdev;
   off_t                st_size;
-#if defined(__rtems__)
+#if __MISC_VISIBLE || __POSIX_VISIBLE >= 200809 || defined(__rtems__)
   struct timespec st_atim;
   struct timespec st_mtim;
   struct timespec st_ctim;
-  blksize_t     st_blksize;
-  blkcnt_t     st_blocks;
-#else
-  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
-#if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
+#elif defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
   time_t       st_atime;
   time_t       st_mtime;
   time_t       st_ctime;
@@ -53,14 +49,19 @@ struct      stat
   long         st_spare2;
   time_t       st_ctime;
   long         st_spare3;
+#endif
+#if defined(__rtems__)
+  blksize_t     st_blksize;
+  blkcnt_t     st_blocks;
+/* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
+#elif !defined(__svr4__) || defined(__PPC__) || defined(__sun__)
   blksize_t    st_blksize;
   blkcnt_t     st_blocks;
   long st_spare4[2];
 #endif
-#endif
 };

-#if defined(__rtems__)
+#if __MISC_VISIBLE || __POSIX_VISIBLE >= 200809 || defined(__rtems__)

On Wed, Aug 14, 2019 at 12:25 PM Yaakov Selkowitz <yselkowi@redhat.com>
wrote:

> On Wed, 2019-08-14 at 12:06 -0700, Dionna Amalie Glaze wrote:
> > Ah you're right, I didn't properly handle the block size definitions.
> > Here's another go at it that splits the timestamp declarations from the
> > blocksize declarations.
> >
> > ---
> >  newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
> >  1 file changed, 34 insertions(+), 30 deletions(-)
> >
> > diff --git a/newlib/libc/include/sys/stat.h
> b/newlib/libc/include/sys/stat.h
> > index eee98db64..4a05081e4 100644
> > --- a/newlib/libc/include/sys/stat.h
> > +++ b/newlib/libc/include/sys/stat.h
> > @@ -24,7 +24,7 @@ extern "C" {
> >  #define stat64 stat
> >  #endif
> >  #else
> > -struct stat
> > +struct stat
> >  {
> >    dev_t                st_dev;
> >    ino_t                st_ino;
> > @@ -34,15 +34,11 @@ struct      stat
> >    gid_t                st_gid;
> >    dev_t                st_rdev;
> >    off_t                st_size;
> > -#if defined(__rtems__)
> > +#if defined(__MISC_VISIBLE) || __POSIX_VISIBLE >= 200809 ||
> defined(__rtems__)
>
> The __*_VISIBLE macros are *always* defined, question is to what.
> These must therefore be used as #if __MISC_VISIBLE etc.
>
> >    struct timespec st_atim;
> >    struct timespec st_mtim;
> >    struct timespec st_ctim;
> > -  blksize_t     st_blksize;
> > -  blkcnt_t     st_blocks;
> > -#else
> > -  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
> > -#if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
> > +#elif defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
>
> What defined __srv4__ that should trigger this section?  It probably
> needs to be moved before the #if __MISC_VISIBLE etc. hunk.
>
> >    time_t       st_atime;
> >    time_t       st_mtime;
> >    time_t       st_ctime;
> > @@ -53,14 +49,19 @@ struct      stat
> >    long         st_spare2;
> >    time_t       st_ctime;
> >    long         st_spare3;
> > +#endif
> > +#if defined(__rtems__)
> > +  blksize_t     st_blksize;
> > +  blkcnt_t     st_blocks;
> > +/* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
> > +#elif !defined(__svr4__) || defined(__PPC__) || defined(__sun__)
> >    blksize_t    st_blksize;
> >    blkcnt_t     st_blocks;
> >    long st_spare4[2];
> >  #endif
> > -#endif
> > };
> >
> > -#if defined(__rtems__)
> > +#if __POSIX_VISIBLE >= 200809 || defined(__rtems__)
> >
> > On Wed, Aug 14, 2019 at 11:53 AM Yaakov Selkowitz wrote:
> > > On Tue, 2019-08-13 at 11:29 -0700, Dionna Amalie Glaze via newlib
> > > wrote:
> > > > The st_{a,c,m}tim fields are needed for POSIX.1-2008, not just RTEMS.
> > > >
> > > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > > > ---
> > > >  newlib/libc/include/sys/stat.h | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/newlib/libc/include/sys/stat.h
> > > b/newlib/libc/include/sys/stat.h
> > > > index eee98db64..052ef5a66 100644
> > > > --- a/newlib/libc/include/sys/stat.h
> > > > +++ b/newlib/libc/include/sys/stat.h
> > > > @@ -34,10 +34,12 @@ struct      stat
> > > >    gid_t                st_gid;
> > > >    dev_t                st_rdev;
> > > >    off_t                st_size;
> > > > -#if defined(__rtems__)
> > > > +#if defined(__USE_MISC) || __POSIX_VISIBLE >= 200809
> > >
> > > Nak.
> > >
> > > 1) __USE_MISC is a glibc internal and has no meaning within Newlib.
> > > The proper guards must be used.
> > >
> > > 2) This would cause the struct size to vary based on FTMs.  This is a
> > > big no-no.  There needs to be size equivalent no-ops in an #else chunk
> > > to match this.
> > >
> > > >    struct timespec st_atim;
> > > >    struct timespec st_mtim;
> > > >    struct timespec st_ctim;
> > > > +#endif
> > > > +#if defined(__rtems__)
> > > >    blksize_t     st_blksize;
> > > >    blkcnt_t     st_blocks;
> > > >  #else
> > > > @@ -60,7 +62,7 @@ struct        stat
> > > >  #endif
> > > >  };
> > > >
> > > > -#if defined(__rtems__)
> > > > +#if __POSIX_VISIBLE >= 200809
> > > >  #define st_atime st_atim.tv_sec
> > > >  #define st_ctime st_ctim.tv_sec
> > > >  #define st_mtime st_mtim.tv_sec
>
> --
> Yaakov Selkowitz
> Senior Software Engineer - Platform Enablement
> Red Hat, Inc.
>
>
>

-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Corinna Vinschen]

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

On Aug 14 12:49, Dionna Amalie Glaze via newlib wrote:
> Fixed the __MISC_VISIBLE part.
> I'm not sure I understand your question. I'm just restructuring how that
> code gets exposed. Where previously the timespec and blocks were defined if
> rtems, and otherwise just the timespec if srv4 etc, I've changed the
> timespec declarations to all be grouped together. The block declarations
> are separate because only the #else after defined(__rtems__) is evaluated
> false and `defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)` is
> evaluated false.
> 
> ---
>  newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/newlib/libc/include/sys/stat.h b/newlib/libc/include/sys/stat.h
> index eee98db64..d7d08e830 100644
> --- a/newlib/libc/include/sys/stat.h
> +++ b/newlib/libc/include/sys/stat.h
> @@ -24,7 +24,7 @@ extern "C" {
>  #define stat64 stat
>  #endif
>  #else
> -struct stat
> +struct stat
>  {
>    dev_t                st_dev;
>    ino_t                st_ino;
> @@ -34,15 +34,11 @@ struct      stat
>    gid_t                st_gid;
>    dev_t                st_rdev;
>    off_t                st_size;
> -#if defined(__rtems__)
> +#if __MISC_VISIBLE || __POSIX_VISIBLE >= 200809 || defined(__rtems__)

Do we really need that?  I'm cringing at the idea to redefine a struct
based on macros set depending on user settings.  Can't we simplify this?
AFAICS, the timestamps definition of rtems is equivalent to the timestamps
definition of all other targets, except svr4 etc.  The only difference
is the additional st_spare4.

I'd like to make the following suggestion, so all targets except svr4 etc.
default to the POSIX compatible definition:

diff --git a/newlib/libc/include/sys/stat.h b/newlib/libc/include/sys/stat.h
index eee98db64a9a..e460c69c963f 100644
--- a/newlib/libc/include/sys/stat.h
+++ b/newlib/libc/include/sys/stat.h
@@ -34,27 +34,17 @@ struct	stat
   gid_t		st_gid;
   dev_t		st_rdev;
   off_t		st_size;
-#if defined(__rtems__)
-  struct timespec st_atim;
-  struct timespec st_mtim;
-  struct timespec st_ctim;
-  blksize_t     st_blksize;
-  blkcnt_t	st_blocks;
-#else
-  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
 #if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
   time_t	st_atime;
   time_t	st_mtime;
   time_t	st_ctime;
 #else
-  time_t	st_atime;
-  long		st_spare1;
-  time_t	st_mtime;
-  long		st_spare2;
-  time_t	st_ctime;
-  long		st_spare3;
-  blksize_t	st_blksize;
+  struct timespec st_atim;
+  struct timespec st_mtim;
+  struct timespec st_ctim;
+  blksize_t     st_blksize;
   blkcnt_t	st_blocks;
+#if !defined(__rtems__)
   long	st_spare4[2];
 #endif
 #endif


Thoughts?
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Dionna Amalie Glaze via newlib]

You would also need the st_*time defines to be defined if
!(defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)) instead of
the current defined(__rtems__). Otherwise that seems fine by me.

On Thu, Aug 15, 2019 at 3:03 AM Corinna Vinschen <vinschen@redhat.com>
wrote:

> On Aug 14 12:49, Dionna Amalie Glaze via newlib wrote:
> > Fixed the __MISC_VISIBLE part.
> > I'm not sure I understand your question. I'm just restructuring how that
> > code gets exposed. Where previously the timespec and blocks were defined
> if
> > rtems, and otherwise just the timespec if srv4 etc, I've changed the
> > timespec declarations to all be grouped together. The block declarations
> > are separate because only the #else after defined(__rtems__) is evaluated
> > false and `defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)`
> is
> > evaluated false.
> >
> > ---
> >  newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
> >  1 file changed, 34 insertions(+), 30 deletions(-)
> >
> > diff --git a/newlib/libc/include/sys/stat.h
> b/newlib/libc/include/sys/stat.h
> > index eee98db64..d7d08e830 100644
> > --- a/newlib/libc/include/sys/stat.h
> > +++ b/newlib/libc/include/sys/stat.h
> > @@ -24,7 +24,7 @@ extern "C" {
> >  #define stat64 stat
> >  #endif
> >  #else
> > -struct stat
> > +struct stat
> >  {
> >    dev_t                st_dev;
> >    ino_t                st_ino;
> > @@ -34,15 +34,11 @@ struct      stat
> >    gid_t                st_gid;
> >    dev_t                st_rdev;
> >    off_t                st_size;
> > -#if defined(__rtems__)
> > +#if __MISC_VISIBLE || __POSIX_VISIBLE >= 200809 || defined(__rtems__)
>
> Do we really need that?  I'm cringing at the idea to redefine a struct
> based on macros set depending on user settings.  Can't we simplify this?
> AFAICS, the timestamps definition of rtems is equivalent to the timestamps
> definition of all other targets, except svr4 etc.  The only difference
> is the additional st_spare4.
>
> I'd like to make the following suggestion, so all targets except svr4 etc.
> default to the POSIX compatible definition:
>
> diff --git a/newlib/libc/include/sys/stat.h
> b/newlib/libc/include/sys/stat.h
> index eee98db64a9a..e460c69c963f 100644
> --- a/newlib/libc/include/sys/stat.h
> +++ b/newlib/libc/include/sys/stat.h
> @@ -34,27 +34,17 @@ struct      stat
>    gid_t                st_gid;
>    dev_t                st_rdev;
>    off_t                st_size;
> -#if defined(__rtems__)
> -  struct timespec st_atim;
> -  struct timespec st_mtim;
> -  struct timespec st_ctim;
> -  blksize_t     st_blksize;
> -  blkcnt_t     st_blocks;
> -#else
> -  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
>  #if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
>    time_t       st_atime;
>    time_t       st_mtime;
>    time_t       st_ctime;
>  #else
> -  time_t       st_atime;
> -  long         st_spare1;
> -  time_t       st_mtime;
> -  long         st_spare2;
> -  time_t       st_ctime;
> -  long         st_spare3;
> -  blksize_t    st_blksize;
> +  struct timespec st_atim;
> +  struct timespec st_mtim;
> +  struct timespec st_ctim;
> +  blksize_t     st_blksize;
>    blkcnt_t     st_blocks;
> +#if !defined(__rtems__)
>    long st_spare4[2];
>  #endif
>  #endif
>
>
> Thoughts?
> Corinna
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
>


-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Joel Sherrill]

On Thu, Aug 15, 2019, 10:59 AM Dionna Amalie Glaze via newlib <
newlib@sourceware.org> wrote:

> You would also need the st_*time defines to be defined if
> !(defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)) instead of
> the current defined(__rtems__). Otherwise that seems fine by me.
>
> On Thu, Aug 15, 2019 at 3:03 AM Corinna Vinschen <vinschen@redhat.com>
> wrote:
>
> > On Aug 14 12:49, Dionna Amalie Glaze via newlib wrote:
> > > Fixed the __MISC_VISIBLE part.
> > > I'm not sure I understand your question. I'm just restructuring how
> that
> > > code gets exposed. Where previously the timespec and blocks were
> defined
> > if
> > > rtems, and otherwise just the timespec if srv4 etc, I've changed the
> > > timespec declarations to all be grouped together. The block
> declarations
> > > are separate because only the #else after defined(__rtems__) is
> evaluated
> > > false and `defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)`
> > is
> > > evaluated false.
> > >
> > > ---
> > >  newlib/libc/include/sys/stat.h | 64 ++++++++++++++++++----------------
> > >  1 file changed, 34 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/newlib/libc/include/sys/stat.h
> > b/newlib/libc/include/sys/stat.h
> > > index eee98db64..d7d08e830 100644
> > > --- a/newlib/libc/include/sys/stat.h
> > > +++ b/newlib/libc/include/sys/stat.h
> > > @@ -24,7 +24,7 @@ extern "C" {
> > >  #define stat64 stat
> > >  #endif
> > >  #else
> > > -struct stat
> > > +struct stat
> > >  {
> > >    dev_t                st_dev;
> > >    ino_t                st_ino;
> > > @@ -34,15 +34,11 @@ struct      stat
> > >    gid_t                st_gid;
> > >    dev_t                st_rdev;
> > >    off_t                st_size;
> > > -#if defined(__rtems__)
> > > +#if __MISC_VISIBLE || __POSIX_VISIBLE >= 200809 || defined(__rtems__)
> >
> > Do we really need that?  I'm cringing at the idea to redefine a struct
> > based on macros set depending on user settings.  Can't we simplify this?
> > AFAICS, the timestamps definition of rtems is equivalent to the
> timestamps
> > definition of all other targets, except svr4 etc.  The only difference
> > is the additional st_spare4.
> >
> > I'd like to make the following suggestion, so all targets except svr4
> etc.
> > default to the POSIX compatible definition:
> >
> > diff --git a/newlib/libc/include/sys/stat.h
> > b/newlib/libc/include/sys/stat.h
> > index eee98db64a9a..e460c69c963f 100644
> > --- a/newlib/libc/include/sys/stat.h
> > +++ b/newlib/libc/include/sys/stat.h
> > @@ -34,27 +34,17 @@ struct      stat
> >    gid_t                st_gid;
> >    dev_t                st_rdev;
> >    off_t                st_size;
> > -#if defined(__rtems__)
> > -  struct timespec st_atim;
> > -  struct timespec st_mtim;
> > -  struct timespec st_ctim;
> > -  blksize_t     st_blksize;
> > -  blkcnt_t     st_blocks;
> > -#else
> > -  /* SysV/sco doesn't have the rest... But Solaris, eabi does.  */
> >  #if defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)
> >    time_t       st_atime;
> >    time_t       st_mtime;
> >    time_t       st_ctime;
> >  #else
> > -  time_t       st_atime;
> > -  long         st_spare1;
> > -  time_t       st_mtime;
> > -  long         st_spare2;
> > -  time_t       st_ctime;
> > -  long         st_spare3;
> > -  blksize_t    st_blksize;
> > +  struct timespec st_atim;
> > +  struct timespec st_mtim;
> > +  struct timespec st_ctim;
> > +  blksize_t     st_blksize;
> >    blkcnt_t     st_blocks;
> > +#if !defined(__rtems__)
> >    long st_spare4[2];
> >  #endif
> >  #endif
> >
> >
> > Thoughts?


I think this looks good unless there is a concern for small memory targets.
But this isn't a structure that is forced on every thread so I don't see
any concern. Make it as standard as possible. :)

--joel

> Corinna
> >
> > --
> > Corinna Vinschen
> > Cygwin Maintainer
> > Red Hat
> >
>
>
> --
> -Dionna Glaze, PhD (she/her)
>

Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008

[Corinna Vinschen]

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

On Aug 15 13:28, Joel Sherrill wrote:
> On Thu, Aug 15, 2019, 10:59 AM Dionna Amalie Glaze via newlib <
> newlib@sourceware.org> wrote:
> 
> > You would also need the st_*time defines to be defined if
> > !(defined(__svr4__) && !defined(__PPC__) && !defined(__sun__)) instead of
> > the current defined(__rtems__). Otherwise that seems fine by me.
> >
> > On Thu, Aug 15, 2019 at 3:03 AM Corinna Vinschen <vinschen@redhat.com>
> > wrote:
> >
> > > On Aug 14 12:49, Dionna Amalie Glaze via newlib wrote:
> > > > [...]
> > > I'd like to make the following suggestion, so all targets except svr4
> > etc.
> > > default to the POSIX compatible definition:
> > > [...]
> 
> I think this looks good unless there is a concern for small memory targets.
> But this isn't a structure that is forced on every thread so I don't see
> any concern. Make it as standard as possible. :)

Pushed, including the guard fix for the backward compat macros.


HTH,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]