Bug 6506 - fesetenv() does not work on hppa with gcc 4.3
Summary: fesetenv() does not work on hppa with gcc 4.3
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: ports (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Carlos O'Donell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-10 18:14 UTC by Aurelien Jarno
Modified: 2014-07-03 11:33 UTC (History)
1 user (show)

See Also:
Host: hppa1.1-unknown-linux-gnu
Target: hppa1.1-unknown-linux-gnu
Build: hppa1.1-unknown-linux-gnu
Last reconfirmed:
fweimer: security-


Attachments
Patch to fix the problem. (499 bytes, patch)
2008-05-10 18:15 UTC, Aurelien Jarno
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aurelien Jarno 2008-05-10 18:14:43 UTC
fesetenv() does not work on hppa with gcc 4.3, as gcc optimizes out most of the 
code. This is due to wrong input/output operands/constraints.
Comment 1 Aurelien Jarno 2008-05-10 18:15:06 UTC
Created attachment 2730 [details]
Patch to fix the problem.
Comment 2 Carlos O'Donell 2008-05-11 12:41:29 UTC
Aurel,

Thanks for doing the work in this case. I appreciate your efforts.

@@ -35,7 +35,7 @@ fesetenv (const fenv_t *envp)
   bufptr = temp.buf;
   __asm__ (
 	   "fstd,ma %%fr0,8(%1)\n"
-	   : "=m" (temp), "+r" (bufptr) : : "%r0");
+	   : "=m" (temp) : "r" (bufptr) : "%r0");

This looks correct, and if it prevents the compiler from optimizing away later
stores into the union, then that's good. I don't think this is the bug.

@@ -56,7 +56,7 @@ fesetenv (const fenv_t *envp)
      is loaded last and T-Bit is enabled. */
   __asm__ (
 	   "fldd,mb -8(%1),%%fr0\n"
-	   : "=m" (temp), "+r" (bufptr) : : "%r0" );
+	   : : "m" (temp), "r" (bufptr) : "%r0" );

This is probably the real bug, temp should never have been an output. Removing
the "=" (write-only) constraint and the "+" read/write constraint was the right
thing to do (along with moving them to inputs).

What is the results of running the testsuite after this patch?
Comment 3 Aurelien Jarno 2008-05-11 13:05:17 UTC
(In reply to comment #2)
> Aurel,
> 
> Thanks for doing the work in this case. I appreciate your efforts.
> 
> @@ -35,7 +35,7 @@ fesetenv (const fenv_t *envp)
>    bufptr = temp.buf;
>    __asm__ (
>            "fstd,ma %%fr0,8(%1)\n"
> -          : "=m" (temp), "+r" (bufptr) : : "%r0");
> +          : "=m" (temp) : "r" (bufptr) : "%r0");
> 
> This looks correct, and if it prevents the compiler from optimizing away 
later
> stores into the union, then that's good. I don't think this is the bug.

I confirm this change is actually not necessary to get fesetenv() working, but 
I prefered to do the change, so that it don't break with a future version of 
gcc.
 
> @@ -56,7 +56,7 @@ fesetenv (const fenv_t *envp)
>       is loaded last and T-Bit is enabled. */
>    __asm__ (
>            "fldd,mb -8(%1),%%fr0\n"
> -          : "=m" (temp), "+r" (bufptr) : : "%r0" );
> +          : : "m" (temp), "r" (bufptr) : "%r0" );
> 
> This is probably the real bug, temp should never have been an output. 
Removing
> the "=" (write-only) constraint and the "+" read/write constraint was the 
right
> thing to do (along with moving them to inputs).
> 
> What is the results of running the testsuite after this patch?

Oh, I forget to tell that with this patch test-fenv now passes with gcc-4.3. 
Otherwise, it does not include any regression.

Comment 4 Sourceware Commits 2008-05-12 12:10:01 UTC
Subject: Bug 6506

CVSROOT:	/cvs/glibc
Module name:	ports
Changes by:	carlos@sourceware.org	2008-05-12 12:09:21

Modified files:
	.              : ChangeLog.hppa 
	sysdeps/hppa/fpu: fesetenv.c 

Log message:
	2008-05-12  Aurelien Jarno  <aurelien@aurel32.net>
	
	[BZ #6506]
	* sysdeps/hppa/fpu/fesetenv.c: bufptr is always read, temp is
	read while writing back status word.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/ports/ChangeLog.hppa.diff?cvsroot=glibc&r1=1.45&r2=1.46
http://sourceware.org/cgi-bin/cvsweb.cgi/ports/sysdeps/hppa/fpu/fesetenv.c.diff?cvsroot=glibc&r1=1.5&r2=1.6

Comment 5 Carlos O'Donell 2008-05-12 12:41:03 UTC
Aurel,

Thank for the good work. The patch looks good. I've checked it in. Marking this
fixed.