#9231 closed bug (fixed)

7.8 regression in Read instance of Data.Fixed.Pico

Reported by: leonbaum2 Owned by:
Priority: highest Milestone: 7.8.3
Component: libraries/base Version: 7.8.2
Keywords: regression Cc: hvr, ekmett, thoughtpolice, igloo, ashley@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #9240 #7483 Differential Revisions:

Description

If I run the following with ghc-7.8.2, the runtime system eats up all of my memory and doesn't produce a result:

read "1" :: Data.Fixed.Pico

Change History (12)

comment:1 Changed 11 months ago by hvr

The parser code seems totally wrong, e.g. consider

convertFixed :: forall a . HasResolution a => Lexeme -> ReadPrec (Fixed a)
convertFixed (Number n)
 | Just (i, f) <- numberToFixed r n =
    return (fromInteger i + (fromInteger f / (10 ^ r)))
    where r = resolution (undefined :: Fixed a)
convertFixed _ = pfail

And note that resolution doesn't return an exponent but rather a power of 10.

comment:2 Changed 11 months ago by hvr

  • Cc thoughtpolice added
  • Keywords regression added
  • Milestone set to 7.8.3
  • Priority changed from normal to highest

comment:3 Changed 11 months ago by hvr

I can't think of any cleaner fix for this other than radically rewriting Data.Fixed or extracting the 10-base exponent via

e = ceiling (logBase 10 (fromInteger r) :: Double)

and having the code read like

convertFixed :: forall a . HasResolution a => Lexeme -> ReadPrec (Fixed a)
convertFixed (Number n)
 | Just (i, f) <- numberToFixed e n =
    return (fromInteger i + (fromInteger f / fromInteger r))
    where r = resolution (undefined :: Fixed a)
          e = ceiling (logBase 10 (fromInteger r) :: Double)
convertFixed _ = pfail

(Note that numberToFixed also expects an exponent rather than a power-of-10)

comment:4 Changed 11 months ago by hvr

The really weird thing though is why the test-case at libraries/base/tests/readFixed001.hs actually works as expected....

comment:5 Changed 11 months ago by simonpj

  • Cc igloo ashley@… added

convertFixed was last modified (or written) by Ian Lynagh. Much of the rest of Data.Fixed is by Ashley Yakeley.

I'm copying both, not in a blaming way, but because they would both be well positioned to offer help on fixing.

But others, please go ahead and suggest fixes.

Simon

comment:6 Changed 11 months ago by hvr

I've just submitted https://phabricator.haskell.org/D25 for review to get the ball rolling

comment:7 Changed 11 months ago by Herbert Valerio Riedel <hvr@…>

In c1035d51edaac2f388a0630e2ad25391e7e3c1ab/ghc:

Fix regression in Data.Fixed Read instance (re #9231)

`convertFixed` operates under the wrong assumption that
`Data.Fixed.resolution` returns a base-10 exponent `e`, whereas it
actually returns the power-of-10 value `10^e`.

So while the code (that was introduced in 5f19f951d8 / #7483) happens to
compute a correct result by coincidence, it allocates huge integer values
in the process (i.e. `10^(10^e)`) which cause long computations which
eventually result in out-of-memory conditions for `e`-values beyond
`Data.Fixed.Milli`.

A proper long-term fix would probably be to extend the `HasResolution`
type-class to have a 2nd method providing the `e` value.

Signed-off-by: Herbert Valerio Riedel <[email protected]>

Differential Revision: https://phabricator.haskell.org/D25

comment:8 Changed 11 months ago by hvr

  • Status changed from new to merge

comment:9 follow-up: Changed 11 months ago by Ashley Yakeley

Just to clarify, r is not necessarily a power of 10.

comment:10 in reply to: ↑ 9 Changed 11 months ago by hvr

Replying to Ashley Yakeley:

Just to clarify, r is not necessarily a power of 10.

Now that you say it, I had the impression an implicit power-of-10 assumption was baked into Data.Fixed, for instance consider the following weird behaviour (can be observed in GHC 7.6 as well):

Prelude> import Data.Fixed
Prelude Data.Fixed> data B7
Prelude Data.Fixed> instance HasResolution B7 where resolution _ = 128
Prelude Data.Fixed> 1.070 :: Fixed B7
1.062
Prelude Data.Fixed> 1.062 :: Fixed B7
1.054
Prelude Data.Fixed> 1.054 :: Fixed B7
1.046
Prelude Data.Fixed> 1.046 :: Fixed B7
1.039

comment:11 Changed 11 months ago by Herbert Valerio Riedel <hvr@…>

In ec550e8f951e50fb91c89389e2e77a3358079c3a/ghc:

Fixup c1035d51e to behave more like in GHC 7.6

The fix in c1035d51e (addressing #9231) was done under the assumption
that `Data.Fixed` is used only with power-of-10 `resolution`. This
follow-up fixup changes the behavior to be more consistent with the
previous behavior in GHC 7.6

For instance, for the following `B7` resolution

   > data B7
   > instance HasResolution B7 where resolution _ = 128

After this patch, the following behavior is now observable:

   > 1.070 :: Fixed B7
   1.062
   > 1.062 :: Fixed B7
   1.054

   > read "1.070" :: Fixed B7
   1.062
   > read "1.062" :: Fixed B7
   1.054

This doesn't provide the desirable "read . show == id" property
yet (which didn't hold in GHC 7.6 either), but at least `fromRational`
and `read` are consistent.

Signed-off-by: Herbert Valerio Riedel <[email protected]>

comment:12 Changed 11 months ago by hvr

  • Resolution set to fixed
  • Status changed from merge to closed

The fix for the regression has been merged to ghc-7.8 via [99462b6877308003442942cbddb3296f29cfb9a8/base] and [4254e158e05ac86b922dea6ba3c3c330732d991b/base]. The remaining "read . show == id" has been moved into a ticket of its own (#9240)

Note: See TracTickets for help on using tickets.