[GHC] #9231: 7.8 regression in Read instance of Data.Fixed.Pico
#9231: 7.8 regression in Read instance of Data.Fixed.Pico ------------------------------------+------------------------------------- Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 7.8.2 Keywords: | Operating System: Unknown/Multiple Architecture: Unknown/Multiple | Type of failure: None/Unknown Difficulty: Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | ------------------------------------+------------------------------------- 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 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by hvr): The parser code seems totally wrong, e.g. consider {{{#!hs 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. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Changes (by hvr): * cc: thoughtpolice (added) * keywords: => regression * priority: normal => highest * milestone: => 7.8.3 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (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 {{{#!hs e = ceiling (logBase 10 (fromInteger r) :: Double) }}} and having the code read like {{{#!hs 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) -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by hvr): The really weird thing though is why the test-case at [[GhcFile(libraries/base/tests/readFixed001.hs)]] actually works as expected.... -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:4> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Changes (by simonpj): * cc: igloo, ashley@… (added) Comment: `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 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by hvr): I've just submitted https://phabricator.haskell.org/D25 for review to get the ball rolling -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:6> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: new Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Herbert Valerio Riedel <hvr@…>): In [changeset:"c1035d51edaac2f388a0630e2ad25391e7e3c1ab/ghc"]: {{{ #!CommitTicketReference repository="ghc" revision="c1035d51edaac2f388a0630e2ad25391e7e3c1ab" 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 <hvr@gnu.org> Differential Revision: https://phabricator.haskell.org/D25 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:7> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: merge Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Changes (by hvr): * status: new => merge -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:8> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: merge Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Ashley Yakeley): Just to clarify, r is not necessarily a power of 10. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:9> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: merge Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by hvr): Replying to [comment:9 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 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:10> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: merge Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: -------------------------------------+------------------------------------ Comment (by Herbert Valerio Riedel <hvr@…>): In [changeset:"ec550e8f951e50fb91c89389e2e77a3358079c3a/ghc"]: {{{ #!CommitTicketReference repository="ghc" revision="ec550e8f951e50fb91c89389e2e77a3358079c3a" 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 <hvr@gnu.org> }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:11> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9231: 7.8 regression in Read instance of Data.Fixed.Pico -------------------------------------+------------------------------------ Reporter: leonbaum2 | Owner: Type: bug | Status: closed Priority: highest | Milestone: 7.8.3 Component: libraries/base | Version: 7.8.2 Resolution: fixed | Keywords: regression Operating System: Unknown/Multiple | Architecture: Unknown/Multiple Type of failure: None/Unknown | Difficulty: Unknown Test Case: | Blocked By: Blocking: | Related Tickets: #9240 #7483 -------------------------------------+------------------------------------ Changes (by hvr): * status: merge => closed * resolution: => fixed * related: => #9240 #7483 Comment: 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) -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9231#comment:12> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC