Message ID | 555CA657.2050300@arm.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 46665 invoked by alias); 20 May 2015 15:21:00 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 46430 invoked by uid 89); 20 May 2015 15:20:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Message-ID: <555CA657.2050300@arm.com> Date: Wed, 20 May 2015 16:20:55 +0100 From: Szabolcs Nagy <szabolcs.nagy@arm.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: GNU C Library <libc-alpha@sourceware.org> CC: Marcus Shawcroft <marcus.shawcroft@arm.com>, Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com> Subject: [PATCH][BZ 18234] struct stat is not posix conform X-MC-Unique: p0VyPDLUQSG0Rg9CqkwfGA-1 Content-Type: multipart/mixed; boundary="------------030809020300040304020604" |
Commit Message
Szabolcs Nagy
May 20, 2015, 3:20 p.m. UTC
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.
Comments
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
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