[Bug libfortran/52413] New: Incorrect behavior of FRACTION when applied to a constant

[Bug libfortran/52413] New: Incorrect behavior of FRACTION when applied to a constant

[frouet at enseeiht dot fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

             Bug #: 52413
           Summary: Incorrect behavior of FRACTION when applied to a
                    constant
    Classification: Unclassified
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libfortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: frouet@enseeiht.fr


Hi,

I have a problem when applying "fraction" to a numerical constant, e.g.
fraction(-2.0); a minimal example says it all:

program test_frac

  real::x,y
  x=-2.0
  x=fraction(x)  
  write(*,*)x
  y=fraction(-2.0) 
  write(*,*)y

end program test_frac

The output is 
 -0.50000000    
  0.50000000

while one would expect
 -0.50000000    
 -0.50000000

I experienced this with gfortran 4.4, 4.5 and 4.6 on Ubuntu systems. On ifort
12, the result is correct.

Regards,

FHR

[Bug libfortran/52413] Incorrect behavior of FRACTION when applied to a constant

[kargl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-02-28
                 CC|                            |kargl at gcc dot gnu.org
         AssignedTo|unassigned at gcc dot       |kargl at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1
      Known to fail|                            |4.2.5, 4.3.6, 4.4.7, 4.5.4,
                   |                            |4.6.3, 4.7.0

--- Comment #1 from kargl at gcc dot gnu.org 2012-02-28 16:11:10 UTC ---
Confirmed.  This fails on all versions of gfortran 4.2 and up.
I don't have a 4.1 version for testing.  I have a patch, but
as this is not a regression, I will not apply it until after
4.7.0 is released.

Tentative patch. Not regression tested, yet.

Index: simplify.c
===================================================================
--- simplify.c  (revision 184485)
+++ simplify.c  (working copy)
@@ -2331,35 +2331,16 @@ gfc_expr *
 gfc_simplify_fraction (gfc_expr *x)
 {
   gfc_expr *result;
-  mpfr_t absv, exp, pow2;
+  mpfr_exp_t e;

   if (x->expr_type != EXPR_CONSTANT)
     return NULL;

   result = gfc_get_constant_expr (BT_REAL, x->ts.kind, &x->where);

-  if (mpfr_sgn (x->value.real) == 0)
-    {
-      mpfr_set_ui (result->value.real, 0, GFC_RND_MODE);
-      return result;
-    }
-
   gfc_set_model_kind (x->ts.kind);
-  mpfr_init (exp);
-  mpfr_init (absv);
-  mpfr_init (pow2);
-
-  mpfr_abs (absv, x->value.real, GFC_RND_MODE);
-  mpfr_log2 (exp, absv, GFC_RND_MODE);
-
-  mpfr_trunc (exp, exp);
-  mpfr_add_ui (exp, exp, 1, GFC_RND_MODE);
-
-  mpfr_ui_pow (pow2, 2, exp, GFC_RND_MODE);
-
-  mpfr_div (result->value.real, absv, pow2, GFC_RND_MODE);

-  mpfr_clears (exp, absv, pow2, NULL);
+  mpfr_frexp (&e, result->value.real, x->value.real, GFC_RND_MODE);

   return range_check (result, "FRACTION");
 }

[Bug libfortran/52413] Incorrect behavior of FRACTION when applied to a constant

[anlauf at gmx dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

Harald Anlauf <anlauf at gmx dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |anlauf at gmx dot de

--- Comment #2 from Harald Anlauf <anlauf at gmx dot de> 2012-02-28 20:43:00 UTC ---
(In reply to comment #1)
> Confirmed.  This fails on all versions of gfortran 4.2 and up.
> I don't have a 4.1 version for testing.

FWIW, it also fails on 4.1.2 (tested on SLES10).

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #3 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> Tentative patch. Not regression tested, yet.

I have applied the patch in comment #1 on top of revision 200321. I have
regtested without regression and tested thet this PR is fixed for all the
available kinds (4, 8, 10, and 16).

Steven, would it help if I do the packaging and the submission on your behalf
(someone will have to commit after approval)?

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[sgk at troutmask dot apl.washington.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #4 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Sat, Jun 22, 2013 at 12:57:56PM +0000, dominiq at lps dot ens.fr wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413
> 
> --- Comment #3 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> > Tentative patch. Not regression tested, yet.
> 
> I have applied the patch in comment #1 on top of revision 200321. I have
> regtested without regression and tested thet this PR is fixed for all the
> available kinds (4, 8, 10, and 16).
> 
> Steven, would it help if I do the packaging and the submission on your behalf
> (someone will have to commit after approval)?
> 

I do not have any problems if you want to pick up where I left
off with this bug.  I simply do not have much time for gfortran
anymore.

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[fxcoudert at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #6 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
The current patch is also lacking handling of the sign if signed zero is used.
This should do the trick:


   if (mpfr_sgn (x->value.real) == 0)
     {
-      mpfr_set_ui (result->value.real, 0, GFC_RND_MODE);
+      mpfr_set (result->value.real, x->value.real, GFC_RND_MODE);
       return result;
     }


Using mpfr_frexp() is a good idea, and probably a gain in the long term (easier
maintenance), so we may wrap it inside version checks. Tentative patch:


Index: simplify.c
===================================================================
--- simplify.c    (revision 200350)
+++ simplify.c    (working copy)
@@ -2342,16 +2342,25 @@ gfc_expr *
 gfc_simplify_fraction (gfc_expr *x)
 {
   gfc_expr *result;
+
+#if MPFR_VERSION < MPFR_VERSION_NUM(3,1,0)
   mpfr_t absv, exp, pow2;
+#else
+  mpfr_exp_t e;
+#endif

   if (x->expr_type != EXPR_CONSTANT)
     return NULL;

   result = gfc_get_constant_expr (BT_REAL, x->ts.kind, &x->where);

+#if MPFR_VERSION < MPFR_VERSION_NUM(3,1,0)
+
+  /* MPFR versions before 3.1.0 do not include mpfr_frexp.  */
+
   if (mpfr_sgn (x->value.real) == 0)
     {
-      mpfr_set_ui (result->value.real, 0, GFC_RND_MODE);
+      mpfr_set (result->value.real, x->value.real, GFC_RND_MODE);
       return result;
     }

@@ -2369,9 +2378,17 @@ gfc_simplify_fraction (gfc_expr *x)
   mpfr_ui_pow (pow2, 2, exp, GFC_RND_MODE);

   mpfr_div (result->value.real, absv, pow2, GFC_RND_MODE);
+  mpfr_copysign (result->value.real, result->value.real,
+         x->value.real, GFC_RND_MODE);

   mpfr_clears (exp, absv, pow2, NULL);

+#else
+
+  mpfr_frexp (&e, result->value.real, x->value.real, GFC_RND_MODE);
+
+#endif
+
   return range_check (result, "FRACTION");
 }



However, I don't have a machine set up for bootstrapping (and regtesting) this
change. I just happened to pass by here :)

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[fxcoudert at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #7 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
I forgot in the last comment to say: handling of sign for non-zero cases, in
old MPFR versions, is done by this line which was missing in the existing code:


+  mpfr_copysign (result->value.real, result->value.real,
+         x->value.real, GFC_RND_MODE);

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #8 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
Before going to the machinery in comment #6, the following patch (i.e., without
any mpfr_copysign)

--- ../_clean/gcc/fortran/simplify.c    2013-06-08 21:50:33.000000000 +0200
+++ gcc/fortran/simplify.c    2013-06-23 15:02:54.000000000 +0200
@@ -2351,7 +2351,7 @@ gfc_simplify_fraction (gfc_expr *x)

   if (mpfr_sgn (x->value.real) == 0)
     {
-      mpfr_set_ui (result->value.real, 0, GFC_RND_MODE);
+      mpfr_set (result->value.real, x->value.real, GFC_RND_MODE);
       return result;
     }

@@ -2368,7 +2368,7 @@ gfc_simplify_fraction (gfc_expr *x)

   mpfr_ui_pow (pow2, 2, exp, GFC_RND_MODE);

-  mpfr_div (result->value.real, absv, pow2, GFC_RND_MODE);
+  mpfr_div (result->value.real, x->value.real, pow2, GFC_RND_MODE);

   mpfr_clears (exp, absv, pow2, NULL);

passes my tests.

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[fxcoudert at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #9 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
> Before going to the machinery in comment #6, the following patch (i.e.,
> without any mpfr_copysign)

Yep, you're right, no need for mpfr_copysign. Your patch looks good, if we
don't want to introduce mpfr_frexp.

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #10 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
Here is the patch I plan to package and submit:

--- ../_clean/gcc/fortran/simplify.c    2013-06-08 21:50:33.000000000 +0200
+++ gcc/fortran/simplify.c    2013-06-23 17:19:55.000000000 +0200
@@ -2342,16 +2342,26 @@ gfc_expr *
 gfc_simplify_fraction (gfc_expr *x)
 {
   gfc_expr *result;
+
+#if MPFR_VERSION < MPFR_VERSION_NUM(3,1,0)
   mpfr_t absv, exp, pow2;
+#else
+  mpfr_exp_t e;
+#endif

   if (x->expr_type != EXPR_CONSTANT)
     return NULL;

   result = gfc_get_constant_expr (BT_REAL, x->ts.kind, &x->where);

+#if MPFR_VERSION < MPFR_VERSION_NUM(3,1,0)
+
+  /* MPFR versions before 3.1.0 do not include mpfr_frexp.  
+     TODO: remove the kludge when MPFR 3.1.0 or newer will be required */
+
   if (mpfr_sgn (x->value.real) == 0)
     {
-      mpfr_set_ui (result->value.real, 0, GFC_RND_MODE);
+      mpfr_set (result->value.real, x->value.real, GFC_RND_MODE);
       return result;
     }

@@ -2368,10 +2378,16 @@ gfc_simplify_fraction (gfc_expr *x)

   mpfr_ui_pow (pow2, 2, exp, GFC_RND_MODE);

-  mpfr_div (result->value.real, absv, pow2, GFC_RND_MODE);
+  mpfr_div (result->value.real, x->value.real, pow2, GFC_RND_MODE);

   mpfr_clears (exp, absv, pow2, NULL);

+#else
+
+  mpfr_frexp (&e, result->value.real, x->value.real, GFC_RND_MODE);
+
+#endif
+
   return range_check (result, "FRACTION");
 }

--- ../_clean/gcc/testsuite/gfortran.dg/fraction.f90    1970-01-01
01:00:00.000000000 +0100
+++ gcc/testsuite/gfortran.dg/fraction.f90    2013-06-23 17:25:09.000000000
+0200
@@ -0,0 +1,16 @@
+! { dg-do run }
+! Test for pr52413
+
+program test_frac
+
+  character(len=26) :: buf
+  real :: y
+  y=fraction (-2.0) 
+  write (buf, *) y
+  if (buf(1:10) /= " -0.500000") call abort ()
+  write (buf, *) fraction (-0.0)
+  if (buf(1:11) /= "  -0.000000") call abort ()
+  write (buf, *) fraction (-2.0_8)
+  if (buf(1:18) /= " -0.50000000000000") call abort ()
+
+end program test_frac

Any comment?

Since the PR is about wrong code, I think it qualifies for some backport with
the patch in comment #8. Should I do the tests or will the backport rejected?

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #11 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
Last question: should I include some tests for the other available kinds?

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[fxcoudert at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #12 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
(In reply to Dominique d'Humieres from comment #10)
> +  y=fraction (-2.0) 
> +  write (buf, *) y
> +  if (buf(1:10) /= " -0.500000") call abort ()

Why involve I/O in your test, and not just test the value like that:

  if (fraction(-2.0) /= -0.5) call abort()

and, slightly more complicated to handle negative zero, checking both value and
sign:

  if (fraction(-0.0) /= 0.0) call abort
  if (sign(1.0, fraction(-0.0)) /= -1.0) call abort

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

--- Comment #13 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> Why involve I/O in your test, and not just test the value like that:
>
>   if (fraction(-2.0) /= -0.5) call abort()
>
> and, slightly more complicated to handle negative zero, checking both 
> value and sign:
>
>  if (fraction(-0.0) /= 0.0) call abort
>  if (sign(1.0, fraction(-0.0)) /= -1.0) call abort

No good reason, I just did not think of a direct comparison. I'll do the change
later. Thanks for the tips.

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #15 from Tobias Burnus <burnus at gcc dot gnu.org> ---
FIXED on the trunk (GCC 4.9).

Thanks for the report!

[Bug fortran/52413] Incorrect behavior of FRACTION when applied to a constant

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52413

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #14 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Author: burnus
Date: Mon Jun 24 08:43:55 2013
New Revision: 200361

URL: http://gcc.gnu.org/viewcvs?rev=200361&root=gcc&view=rev
Log:
2013-06-24  Steven G. Kargl  <sgk@troutmask.apl.washington.edu>
            Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>
            Dominique d'Humieres  <dominiq@lps.ens.fr>

        PR fortran/52413
        * simplify.c (gfc_simplify_fraction): Fix the sign of negative values.

2013-06-24  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>
            Dominique d'Humieres  <dominiq@lps.ens.fr>

        PR fortran/52413
        * gfortran.dg/fraction.f90: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/fraction.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/simplify.c
    trunk/gcc/testsuite/ChangeLog