* [PATCH] Make st_*tim visible in stat for POSIX.1-2008 @ 2019-08-13 18:30 Dionna Amalie Glaze via newlib 2019-08-13 18:46 ` Joel Sherrill 2019-08-14 18:52 ` Yaakov Selkowitz 0 siblings, 2 replies; 12+ messages in thread From: Dionna Amalie Glaze via newlib @ 2019-08-13 18:30 UTC (permalink / raw) To: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-13 18:30 [PATCH] Make st_*tim visible in stat for POSIX.1-2008 Dionna Amalie Glaze via newlib @ 2019-08-13 18:46 ` Joel Sherrill 2019-08-14 8:41 ` Corinna Vinschen 2019-08-14 18:52 ` Yaakov Selkowitz 1 sibling, 1 reply; 12+ messages in thread From: Joel Sherrill @ 2019-08-13 18:46 UTC (permalink / raw) To: Dionna Amalie Glaze; +Cc: Newlib 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) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-13 18:46 ` Joel Sherrill @ 2019-08-14 8:41 ` Corinna Vinschen 2019-08-14 18:45 ` Dionna Amalie Glaze via newlib 0 siblings, 1 reply; 12+ messages in thread From: Corinna Vinschen @ 2019-08-14 8:41 UTC (permalink / raw) To: newlib [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-14 8:41 ` Corinna Vinschen @ 2019-08-14 18:45 ` Dionna Amalie Glaze via newlib 0 siblings, 0 replies; 12+ messages in thread From: Dionna Amalie Glaze via newlib @ 2019-08-14 18:45 UTC (permalink / raw) To: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-13 18:30 [PATCH] Make st_*tim visible in stat for POSIX.1-2008 Dionna Amalie Glaze via newlib 2019-08-13 18:46 ` Joel Sherrill @ 2019-08-14 18:52 ` Yaakov Selkowitz 2019-08-14 19:06 ` Dionna Amalie Glaze via newlib 1 sibling, 1 reply; 12+ messages in thread From: Yaakov Selkowitz @ 2019-08-14 18:52 UTC (permalink / raw) To: newlib 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-14 18:52 ` Yaakov Selkowitz @ 2019-08-14 19:06 ` Dionna Amalie Glaze via newlib 2019-08-14 19:25 ` Yaakov Selkowitz 0 siblings, 1 reply; 12+ messages in thread From: Dionna Amalie Glaze via newlib @ 2019-08-14 19:06 UTC (permalink / raw) To: Yaakov Selkowitz; +Cc: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-14 19:06 ` Dionna Amalie Glaze via newlib @ 2019-08-14 19:25 ` Yaakov Selkowitz 2019-08-14 19:50 ` Dionna Amalie Glaze via newlib 0 siblings, 1 reply; 12+ messages in thread From: Yaakov Selkowitz @ 2019-08-14 19:25 UTC (permalink / raw) To: newlib 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-14 19:25 ` Yaakov Selkowitz @ 2019-08-14 19:50 ` Dionna Amalie Glaze via newlib 2019-08-15 10:03 ` Corinna Vinschen 0 siblings, 1 reply; 12+ messages in thread From: Dionna Amalie Glaze via newlib @ 2019-08-14 19:50 UTC (permalink / raw) To: Yaakov Selkowitz; +Cc: 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-14 19:50 ` Dionna Amalie Glaze via newlib @ 2019-08-15 10:03 ` Corinna Vinschen 2019-08-15 15:59 ` Dionna Amalie Glaze via newlib 0 siblings, 1 reply; 12+ messages in thread From: Corinna Vinschen @ 2019-08-15 10:03 UTC (permalink / raw) To: Dionna Amalie Glaze; +Cc: Yaakov Selkowitz, newlib [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-15 10:03 ` Corinna Vinschen @ 2019-08-15 15:59 ` Dionna Amalie Glaze via newlib 2019-08-15 18:29 ` Joel Sherrill 0 siblings, 1 reply; 12+ messages in thread From: Dionna Amalie Glaze via newlib @ 2019-08-15 15:59 UTC (permalink / raw) To: newlib, Dionna Amalie Glaze, Yaakov Selkowitz 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-15 15:59 ` Dionna Amalie Glaze via newlib @ 2019-08-15 18:29 ` Joel Sherrill 2019-08-16 8:59 ` Corinna Vinschen 0 siblings, 1 reply; 12+ messages in thread From: Joel Sherrill @ 2019-08-15 18:29 UTC (permalink / raw) To: Dionna Amalie Glaze; +Cc: Newlib, Yaakov Selkowitz 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) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Make st_*tim visible in stat for POSIX.1-2008 2019-08-15 18:29 ` Joel Sherrill @ 2019-08-16 8:59 ` Corinna Vinschen 0 siblings, 0 replies; 12+ messages in thread From: Corinna Vinschen @ 2019-08-16 8:59 UTC (permalink / raw) To: Joel Sherrill; +Cc: Dionna Amalie Glaze, Newlib, Yaakov Selkowitz [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-08-16 8:59 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-13 18:30 [PATCH] Make st_*tim visible in stat for POSIX.1-2008 Dionna Amalie Glaze via newlib 2019-08-13 18:46 ` Joel Sherrill 2019-08-14 8:41 ` Corinna Vinschen 2019-08-14 18:45 ` Dionna Amalie Glaze via newlib 2019-08-14 18:52 ` Yaakov Selkowitz 2019-08-14 19:06 ` Dionna Amalie Glaze via newlib 2019-08-14 19:25 ` Yaakov Selkowitz 2019-08-14 19:50 ` Dionna Amalie Glaze via newlib 2019-08-15 10:03 ` Corinna Vinschen 2019-08-15 15:59 ` Dionna Amalie Glaze via newlib 2019-08-15 18:29 ` Joel Sherrill 2019-08-16 8:59 ` Corinna Vinschen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).