hppa: fix __O_SYNC to match the kernel
diff mbox

Message ID 1424755185-27627-1-git-send-email-vapier@gentoo.org
State Accepted
Headers show

Commit Message

Mike Frysinger Feb. 24, 2015, 5:19 a.m. UTC
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(-)

Comments

John David Anglin Feb. 24, 2015, 2:48 p.m. UTC | #1
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
Joseph Myers Feb. 24, 2015, 4:20 p.m. UTC | #2
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.
Carlos O'Donell Feb. 27, 2015, 5:06 a.m. UTC | #3
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.
Mike Frysinger Feb. 27, 2015, 6:46 a.m. UTC | #4
i've pushed this now with Carlos' blessing
-mike
Mike Frysinger Feb. 27, 2015, 6:53 a.m. UTC | #5
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
John David Anglin March 1, 2015, 7:55 p.m. UTC | #6
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
Joseph Myers March 1, 2015, 10:24 p.m. UTC | #7
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).
John David Anglin March 2, 2015, 1:18 p.m. UTC | #8
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
Joseph Myers March 2, 2015, 2:09 p.m. UTC | #9
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.
Carlos O'Donell March 8, 2015, 7:52 p.m. UTC | #10
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.
John David Anglin March 8, 2015, 9:30 p.m. UTC | #11
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
Carlos O'Donell March 9, 2015, 12:36 p.m. UTC | #12
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.

Patch
diff mbox

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 */