[BZ,18234] struct stat is not posix conform

  the generic definition of struct stat on linux miss the
st_atim, st_mtim and st_ctim members when _POSIX_C_SOURCE
is defined (affects at least aarch64 and arm).

i did not see regressions on aarch64.

2015-05-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	[BZ #18234]
	* sysdeps/unix/sysv/linux/generic/bits/stat.h (struct stat): Make
	st_atim, st_ctim, st_mtim visible under __USE_XOPEN2K8.
  

On 05/20/2015 05:20 PM, Szabolcs Nagy wrote:
> the generic definition of struct stat on linux miss the
> st_atim, st_mtim and st_ctim members when _POSIX_C_SOURCE
> is defined (affects at least aarch64 and arm).

Does this need to be conditional on the POSIX version?
  

On 05/20/2015 11:58 AM, Florian Weimer wrote:
> On 05/20/2015 05:20 PM, Szabolcs Nagy wrote:
>> the generic definition of struct stat on linux miss the
>> st_atim, st_mtim and st_ctim members when _POSIX_C_SOURCE
>> is defined (affects at least aarch64 and arm).
> 
> Does this need to be conditional on the POSIX version?
> 

This is already done by features.h.

e.g.
#if (_POSIX_C_SOURCE - 0) >= 200809L
# define __USE_XOPEN2K8         1
# undef  _ATFILE_SOURCE
# define _ATFILE_SOURCE 1
#endif

Which should define __USE_XOPEN2K8 as expected.

My worry is that you've changed the semantics since __USE_MISC
was previously used.

Cheers,
Carlos.
  

On 05/20/2015 11:20 AM, Szabolcs Nagy wrote:
> the generic definition of struct stat on linux miss the
> st_atim, st_mtim and st_ctim members when _POSIX_C_SOURCE
> is defined (affects at least aarch64 and arm).
> 
> i did not see regressions on aarch64.
> 
> 2015-05-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	[BZ #18234]
> 	* sysdeps/unix/sysv/linux/generic/bits/stat.h (struct stat): Make
> 	st_atim, st_ctim, st_mtim visible under __USE_XOPEN2K8.

I like the idea of the change.

Firstly it needs a test case.

Secondly, it changes when these members are made available, and that
might break applications unless you show that __USE_MISC is a subset
of __USE_XOPEN2K8, which I don't think it is.

Therefore you will have to use "__USE_MISC || __USE_XOPEN2K8" preceeded
by a lengthy comment about the fact that in _DEFAULT_SOURCE you'll get
these members, but in strict compliance you need to have POSIX Issue 7.
 
Joseph, does that sound right to you?

> diff --git a/sysdeps/unix/sysv/linux/generic/bits/stat.h b/sysdeps/unix/sysv/linux/generic/bits/stat.h
> index 42cb198..82e6b1d 100644
> --- a/sysdeps/unix/sysv/linux/generic/bits/stat.h
> +++ b/sysdeps/unix/sysv/linux/generic/bits/stat.h
> @@ -66,7 +66,7 @@ struct stat
>      __blksize_t st_blksize;	/* Optimal block size for I/O.  */
>      int __pad2;
>      __field64(__blkcnt_t, __blkcnt64_t, st_blocks);  /* 512-byte blocks */
> -#ifdef __USE_MISC
> +#ifdef __USE_XOPEN2K8
>      /* Nanosecond resolution timestamps are stored in a format
>         equivalent to 'struct timespec'.  This is the type used
>         whenever possible but the Unix namespace rules do not allow the

Cheers,
Carlos.
  

On Wed, 20 May 2015, Carlos O'Donell wrote:

> On 05/20/2015 11:20 AM, Szabolcs Nagy wrote:
> > the generic definition of struct stat on linux miss the
> > st_atim, st_mtim and st_ctim members when _POSIX_C_SOURCE
> > is defined (affects at least aarch64 and arm).
> > 
> > i did not see regressions on aarch64.
> > 
> > 2015-05-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> > 
> > 	[BZ #18234]
> > 	* sysdeps/unix/sysv/linux/generic/bits/stat.h (struct stat): Make
> > 	st_atim, st_ctim, st_mtim visible under __USE_XOPEN2K8.
> 
> I like the idea of the change.
> 
> Firstly it needs a test case.

I.e. adding

#if defined XOPEN2K8 || defined POSIX2008
element {struct stat} {struct timespec} st_atim
element {struct stat} {struct timespec} st_mtim
element {struct stat} {struct timespec} st_ctim
#endif

to conform/data/sys/stat.h-data (the conform/ data hasn't generally been 
fully reviewed against current POSIX, hence such assertions being 
missing).

> Secondly, it changes when these members are made available, and that
> might break applications unless you show that __USE_MISC is a subset
> of __USE_XOPEN2K8, which I don't think it is.
> 
> Therefore you will have to use "__USE_MISC || __USE_XOPEN2K8" preceeded
> by a lengthy comment about the fact that in _DEFAULT_SOURCE you'll get
> these members, but in strict compliance you need to have POSIX Issue 7.

The API should be consistent across architectures.

Most widely-used cases use __USE_XOPEN2K8.  The exceptions are: toplevel 
bits/stat.h (not used in fact) doesn't have the timespec members; 
sysdeps/nacl/bits/stat.h uses __USE_MISC || __USE_XOPEN2K8; as discussed, 
linux/generic uses __USE_MISC; linux/ia64 uses __USE_MISC; 
linux/microblaze uses __USE_MISC.

So, I think a change to use just __USE_XOPEN2K8 is most appropriate.  This 
would fix bugs with the elements not being made available when they should 
be.  The default API level includes __USE_XOPEN2K8.  If __USE_MISC is 
defined, so is __USE_XOPEN2K8: __USE_MISC is only defined when 
_DEFAULT_SOURCE is defined (explicitly or implicitly), and _DEFAULT_SOURCE 
implies __USE_XOPEN2K8.  So __USE_MISC || __USE_XOPEN2K8 is entirely 
redundant and means the same as __USE_XOPEN2K8.

Thus: fix nacl, linux/generic, linux/ia64 and linux/microblaze to use 
__USE_XOPEN2K8 here (all in one patch, and the bug in Bugzilla might 
reasonably be rescoped to cover all those cases).

linux/alpha does its own more complicated thing with a macro __ST_TIME; I 
see no real reason for one architecture to do things differently from all 
the test, but also no reason to change it for this bug since it's already 
using __USE_XOPEN2K8.
  

On 20/05/15 17:44, Joseph Myers wrote:
> Most widely-used cases use __USE_XOPEN2K8.  The exceptions are: toplevel 
> bits/stat.h (not used in fact) doesn't have the timespec members; 
> sysdeps/nacl/bits/stat.h uses __USE_MISC || __USE_XOPEN2K8; as discussed, 
> linux/generic uses __USE_MISC; linux/ia64 uses __USE_MISC; 
> linux/microblaze uses __USE_MISC.
> 
> So, I think a change to use just __USE_XOPEN2K8 is most appropriate.  This 
> would fix bugs with the elements not being made available when they should 
> be.  The default API level includes __USE_XOPEN2K8.  If __USE_MISC is 
> defined, so is __USE_XOPEN2K8: __USE_MISC is only defined when 
> _DEFAULT_SOURCE is defined (explicitly or implicitly), and _DEFAULT_SOURCE 
> implies __USE_XOPEN2K8.  So __USE_MISC || __USE_XOPEN2K8 is entirely 
> redundant and means the same as __USE_XOPEN2K8.
> 
> Thus: fix nacl, linux/generic, linux/ia64 and linux/microblaze to use 
> __USE_XOPEN2K8 here (all in one patch, and the bug in Bugzilla might 
> reasonably be rescoped to cover all those cases).
> 

ok,

i guess it's ok for consistency if i fix struct stat64
too to use __USE_XOPEN2K8.

i will run some tests and come back with a patch

> linux/alpha does its own more complicated thing with a macro __ST_TIME; I 
> see no real reason for one architecture to do things differently from all 
> the test, but also no reason to change it for this bug since it's already 
> using __USE_XOPEN2K8.
> 
> -- Joseph S. Myers joseph@codesourcery.com
>
  

On Wed, May 20, 2015 at 06:55:02PM +0100, Szabolcs Nagy wrote:
> i guess it's ok for consistency if i fix struct stat64
> too to use __USE_XOPEN2K8.
> 
> i will run some tests and come back with a patch

I also think it would be appropriate to change this code in other
architectures (microblaze and nacl IIRC) to make all of them
consistent.  It is a mechanical enough change IMO that all arch
maintainer acks is not necessary.

Siddhesh