| Message ID | 1424755185-27627-1-git-send-email-vapier@gentoo.org |
|---|---|
| State | Accepted |
| Headers | show |
On 2015-02-24 12:19 AM, Mike Frysinger wrote: > From: John David Anglin <dave.anglin@bell.net> > > 2015-02-24 John David Anglin <dave.anglin@bell.net> > > * sysdeps/unix/sysv/linux/hppa/bits/fcntl.h (__O_SYNC): Change > to 00100000. Mike, thanks for sending this change. Reminds me I should go through the Debian glibc patches and see what else needs sending. Dave
On Tue, 24 Feb 2015, John David Anglin wrote: > On 2015-02-24 12:19 AM, Mike Frysinger wrote: > > From: John David Anglin <dave.anglin@bell.net> > > > > 2015-02-24 John David Anglin <dave.anglin@bell.net> > > > > * sysdeps/unix/sysv/linux/hppa/bits/fcntl.h (__O_SYNC): Change > > to 00100000. > Mike, thanks for sending this change. Reminds me I should go through the > Debian glibc > patches and see what else needs sending. See <https://sourceware.org/glibc/wiki/PortStatus> for a list of issues where the hppa port needs updating. Also see Bugzilla for hppa bugs. As usual, if the issue you're fixing was user-visible in a release, there should be a bug filed in Bugzilla for it.
On 02/24/2015 11:20 AM, Joseph Myers wrote: > On Tue, 24 Feb 2015, John David Anglin wrote: > >> On 2015-02-24 12:19 AM, Mike Frysinger wrote: >>> From: John David Anglin <dave.anglin@bell.net> >>> >>> 2015-02-24 John David Anglin <dave.anglin@bell.net> >>> >>> * sysdeps/unix/sysv/linux/hppa/bits/fcntl.h (__O_SYNC): Change >>> to 00100000. >> Mike, thanks for sending this change. Reminds me I should go through the >> Debian glibc >> patches and see what else needs sending. > > See <https://sourceware.org/glibc/wiki/PortStatus> for a list of issues > where the hppa port needs updating. Also see Bugzilla for hppa bugs. As > usual, if the issue you're fixing was user-visible in a release, there > should be a bug filed in Bugzilla for it. Thanks Mike! Cheers, Carlos.
i've pushed this now with Carlos' blessing -mike
On 24 Feb 2015 09:48, John David Anglin wrote: > Mike, thanks for sending this change. Reminds me I should go through > the Debian glibc > patches and see what else needs sending. this should be an easy one: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=1c1d3f4f11b4a911a3b6ffab0aac61d5f8e02873 if you could provide context, i could shepherd it through. -mike
Hi Mike, On 2015-02-27, at 1:53 AM, Mike Frysinger wrote: > On 24 Feb 2015 09:48, John David Anglin wrote: >> Mike, thanks for sending this change. Reminds me I should go through >> the Debian glibc >> patches and see what else needs sending. > > this should be an easy one: > https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=1c1d3f4f11b4a911a3b6ffab0aac61d5f8e02873 I started to write a ChangeLog entry and realized that there is a problem with the change to feholdexcept.c. I believe the second set of bufptr needs to be removed. > > if you could provide context, i could shepherd it through. The patch is intended to fix problems in accessing and updating the floating-point status register and exception registers. Originally, the routines were written to save and restore the status register and the entire set of (all seven) exception registers. At some point, the code was changed to just manipulate the status register and the first exception register. A couple of bugs were introduced when this was done. In fesetenv.c, the post increment of bufptr was retained in the first asm but the constraint for it does not indicate that bufptr is modified. As a result, GCC miscompiled fesetenv. We get better code by not modifying bufptr as GCC doesn't have to reload bufptr. The main bug in feholdexcept is the second set of bufptr. This existed to the restore the exception registers in reverse order. This statement should have been removed when the code was changed to only update the status and first exception registers. The offset used in the statement is also off by a factor two, so it probably never worked correctly. With the current patch, the code loads zero to the status and exception register. As a result, the T bit is not set properly. I will test a revised change to feholdexcept and get back to you. Dave -- John David Anglin dave.anglin@bell.net
On Sun, 1 Mar 2015, John David Anglin wrote: > The main bug in feholdexcept is the second set of bufptr. This existed > to the restore the exception registers in reverse order. This statement > should have been removed when the code was changed to only update the > status and first exception registers. The offset used in the statement > is also off by a factor two, so it probably never worked correctly. > With the current patch, the code loads zero to the status and exception > register. As a result, the T bit is not set properly. If this bug was user-visible in a release, there should be a bug filed in Bugzilla for it (this generally applies when fixing any bug that was user-visible in a release).
On 2015-03-01, at 5:24 PM, Joseph Myers wrote: > If this bug was user-visible in a release, there should be a bug filed in > Bugzilla for it (this generally applies when fixing any bug that was > user-visible in a release). https://sourceware.org/bugzilla/show_bug.cgi?id=18068 Dave -- John David Anglin dave.anglin@bell.net
On Mon, 2 Mar 2015, John David Anglin wrote: > On 2015-03-01, at 5:24 PM, Joseph Myers wrote: > > > If this bug was user-visible in a release, there should be a bug filed in > > Bugzilla for it (this generally applies when fixing any bug that was > > user-visible in a release). > > https://sourceware.org/bugzilla/show_bug.cgi?id=18068 The ports component should no longer be used (i.e. should only contain resolved bugs; I don't know if it's possible to disable filing new bugs in a component without affecting existing bugs in that component); bugs for all architectures should now go in components such as libc. As the fix for this has been checked in, the other steps needed are putting the [BZ #18068] marker in the ChangeLog entry, adding the bug to the list of fixed bugs in NEWS, and closing the bug as fixed.
On 03/01/2015 02:55 PM, John David Anglin wrote: > In fesetenv.c, the post increment of bufptr was retained in the first asm but the constraint for it > does not indicate that bufptr is modified. As a result, GCC miscompiled fesetenv. We get better > code by not modifying bufptr as GCC doesn't have to reload bufptr. It uses "+r" which means the operand, the register, is read and modified. The fact that we get better code, or work around a gcc bug, is a good reason to make the change, but I don't see what's wrong with the original code. > The main bug in feholdexcept is the second set of bufptr. This existed to the restore the exception > registers in reverse order. This statement should have been removed when the code was changed > to only update the status and first exception registers. The offset used in the statement is also off > by a factor two, so it probably never worked correctly. With the current patch, the code loads zero to > the status and exception register. As a result, the T bit is not set properly. This doesn't sound right either. fstd,ma %%fr0,8(%1) Results in a store to bufptr, with bubptr-=8 *after* the store. fldd,mb -8(%0),%%fr0 Results in a bufptr-=8, followed by a load from bufptr to fr0. Thus while the operations are left-over from previous iterations of the code where we did save/load all of the registers, the above code is idempotent with respect to your changes. The one problem is that bufptr is not marked as changed in the *second* assembly contstraint which just lists bufptr as input (which is wrong) and an optimization might reorder things such that this breaks. Cheers, Carlos.
On 2015-03-08, at 3:52 PM, Carlos O'Donell wrote: > On 03/01/2015 02:55 PM, John David Anglin wrote: >> In fesetenv.c, the post increment of bufptr was retained in the first asm but the constraint for it >> does not indicate that bufptr is modified. As a result, GCC miscompiled fesetenv. We get better >> code by not modifying bufptr as GCC doesn't have to reload bufptr. > > It uses "+r" which means the operand, the register, is read and modified. > > The fact that we get better code, or work around a gcc bug, is a good reason > to make the change, but I don't see what's wrong with the original code. > >> The main bug in feholdexcept is the second set of bufptr. This existed to the restore the exception >> registers in reverse order. This statement should have been removed when the code was changed >> to only update the status and first exception registers. The offset used in the statement is also off >> by a factor two, so it probably never worked correctly. With the current patch, the code loads zero to >> the status and exception register. As a result, the T bit is not set properly. > > This doesn't sound right either. > > fstd,ma %%fr0,8(%1) > > Results in a store to bufptr, with bubptr-=8 *after* the store. > > fldd,mb -8(%0),%%fr0 > > Results in a bufptr-=8, followed by a load from bufptr to fr0. > > Thus while the operations are left-over from previous iterations of > the code where we did save/load all of the registers, the above code > is idempotent with respect to your changes. > > The one problem is that bufptr is not marked as changed in the *second* > assembly contstraint which just lists bufptr as input (which is wrong) > and an optimization might reorder things such that this breaks. Exactly. The post and pre-increments could be used but one needs to ensure that the bufptr asm arguments are marked as written. I suspect instructions with post and pre-increment are slightly slower but the difference may not be visible given the length of the pipeline. We want to avoid re-initializing bufptr. Thus, I believe it's better to not modify bufptr in the asms in these two functions. Dave -- John David Anglin dave.anglin@bell.net
On 03/08/2015 05:30 PM, John David Anglin wrote:
> Thus, I believe it's better to not modify bufptr in the asms in these two functions.
Agreed. I'm running another test run right now and will commit your changes
if it makes things better (which I assume it does).
Cheers,
Carlos.
diff --git a/sysdeps/unix/sysv/linux/hppa/bits/fcntl.h b/sysdeps/unix/sysv/linux/hppa/bits/fcntl.h index 9aad095..a800e28 100644 --- a/sysdeps/unix/sysv/linux/hppa/bits/fcntl.h +++ b/sysdeps/unix/sysv/linux/hppa/bits/fcntl.h @@ -27,7 +27,7 @@ #define O_NONBLOCK 00200004 /* HPUX has separate NDELAY & NONBLOCK */ #define __O_DSYNC 01000000 #define __O_RSYNC 02000000 /* HPUX only */ -#define __O_SYNC 01000000 +#define __O_SYNC 00100000 #define O_SYNC (__O_SYNC|__O_DSYNC) #define O_BLKSEEK 00000100 /* HPUX only */
From: John David Anglin <dave.anglin@bell.net> 2015-02-24 John David Anglin <dave.anglin@bell.net> * sysdeps/unix/sysv/linux/hppa/bits/fcntl.h (__O_SYNC): Change to 00100000. --- sysdeps/unix/sysv/linux/hppa/bits/fcntl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)