Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#10245 closed bug (fixed)

panic in new integer switch logic with "out-of-range" literals

Reported by: rwbarton Owned by: AndreasK
Priority: normal Milestone: 8.2.1
Component: Compiler (CodeGen) Version: 7.11
Keywords: Cc: nomeata, simonmar, AndreasK
Operating System: Unknown/Multiple Architecture: x86_64 (amd64)
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D4218
Wiki Page:

Description

Compiling this module

module D (f) where
f :: Int -> String
f n = case n of
  0x8000000000000000 -> "yes"
  _ -> "no"

crashes with the error

[1 of 1] Compiling D                ( /tmp/D.hs, /tmp/D.o )
ghc-stage1: panic! (the 'impossible' happened)
  (GHC version 7.11.20150403 for x86_64-unknown-linux):
	Map.findMin: empty map has no minimal element

The constant does not have to be exactly 0x8000000000000000, everything I tested from there up to 0xffffffffffffffff yields the same crash. Also occurs with Word and negative literals.

The bug seems to be tied to the target's word size somehow, though: a 64-bit compiler does not panic on Int32 and 0x80000000, but a 32-bit compiler does.

Change History (18)

comment:1 Changed 5 years ago by nomeata

Owner: set to nomeata

Thanks, I’ll look into it.

comment:2 Changed 5 years ago by nomeata

Interesting, as a test case it fails with

ghc-stage2: panic! (the 'impossible' happened)
  (GHC version 7.11.20150403 for x86_64-unknown-linux):
	ASSERT failed!
  file compiler/basicTypes/Literal.hs line 220 9223372036854775808

due to

mkMachInt :: DynFlags -> Integer -> Literal
mkMachInt dflags x   = ASSERT2( inIntRange dflags x,  integer x )
                       MachInt x

This makes we wonder: Is that even legal Haskell, or should we fail much earlier?

comment:3 Changed 5 years ago by nomeata

Resolution: fixed
Status: newclosed

Fixed. One could think about whether that is legal haskell or whether the ASSERT is wrong, but that’s a different story, and not one for Easter night.

comment:4 Changed 5 years ago by Joachim Breitner <mail@…>

In a838d1f7c668f6c5886ce350cb456f3ecbcb1499/ghc:

CmmSwitch: Do not trip over a case with no (valid) branches

This fixes #10245. I did not commit the test case, as it fails
unconditionally with a compiler built with -DDEBUG, so maybe it is bogus
Haskell anyways.

comment:5 Changed 5 years ago by rwbarton

FWIW I encountered this in the wild (https://github.com/ghc/packages-time/blob/master/lib/Data/Time/LocalTime/TimeZone.hs#L88) while trying to cross-compile GHC to x86.

I'm pretty sure it is valid Haskell; what if the type signature was f :: Num a => a -> String? Should specializing a to Int turn a valid program into an invalid one?

comment:6 Changed 5 years ago by rwbarton

Curiously for a 32-bit compiler I can reproduce the issue with Int but not with Int32. Maybe the ASSERT is really checking an intended invariant, and something went wrong earlier (desugaring?)

comment:7 Changed 5 years ago by nomeata

I'm pretty sure it is valid Haskell;

for some reason I was assuming you were writing unboxed literals. With regular literals, you are right of course!

But I think we are implementing it wrongly. According to the report, your code should be treated like

f n = if n == fromIntegral 0x8000000000000000 then "yes" else "no"

but my impression is that we produce a literal 0x8000000000000000 :: Int# internally, which is then dropped from the case. I’ll check.

comment:8 Changed 5 years ago by nomeata

Owner: nomeata deleted
Resolution: fixed
Status: closednew

Ok, I think I broke something else since 7.8. If I add

main = do
    let string = "0x8000000000000000"
    let i = read string :: Integer
    let i' = fromIntegral i :: Int
    print i
    print i'
    print (f i')

to your example, 7.8.4 produces

9223372036854775808
-9223372036854775808
"yes"

while HEAD produces

9223372036854775808
-9223372036854775808
"no"

I’ll investigate.

comment:9 Changed 5 years ago by Joachim Breitner <mail@…>

In 8f0709249826dd2eda9bf53be89f176a4f30c81f/ghc:

Test case for #10246

still marked known_broken. This also adds the test case for #10245,
which should pass once #10246 is fixed.

comment:10 Changed 5 years ago by nomeata

There is something larger at miss. I created #10246 for that, and left this for the (now fixed) partiality of the new switch logic.

comment:11 Changed 5 years ago by rwbarton

Good catch on the relation to #9533, I had totally forgotten about that ticket!

comment:12 Changed 4 years ago by Thomas Miedema <thomasmiedema@…>

In aee19d0/ghc:

Testsuite: T10245 is passing for WAY=ghci (#10245)

Needed to get closer to passing `validate --slow`.

comment:13 Changed 3 years ago by Ben Gamari <ben@…>

In 6dfc5eb/ghc:

Ensure that Literals are in range

This commit fixes several bugs related to case expressions
involving numeric literals which are not in the range of values of
their (fixed-width, integral) type.

There is a new invariant on Literal: The argument of a MachInt[64]
or MachWord[64] must lie within the range of the corresponding
primitive type Int[64]# or Word[64]#, as defined by the target machine.
This invariant is enforced in mkMachInt[64]/mkMachWord[64] by wrapping
the argument to the target type's range if necessary.

Test Plan: Test Plan: make slowtest TEST="T9533 T9533b T9533c T10245
T10246"

Trac issues: #9533, #10245, #10246, #13171

Reviewers: simonmar, simonpj, austin, bgamari, nomeata

Reviewed By: bgamari

Subscribers: thomie, rwbarton

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

comment:14 Changed 3 years ago by bgamari

Milestone: 8.2.1
Resolution: fixed
Status: newclosed

comment:15 Changed 2 years ago by AndreasK

Cc: AndreasK added
Resolution: fixed
Status: closednew
Type of failure: Compile-time crashIncorrect result at runtime

In MatchLit dsLit doesn't use the mkMachInt functions resulting in potentially faulty code.

y = I# 0x8000000000000000# :: Int compiles without warning with -Wall for example.

comment:16 Changed 2 years ago by AndreasK

Differential Rev(s): Phab:D4218
Owner: set to AndreasK
Priority: highnormal

comment:17 Changed 2 years ago by AndreasK

Status: newpatch

I have a patch up on Phab but it increases allocations. (Phab:D4218)

This seems to be caused by desugaring of derived constants. While the constants should be safe I don't see a way to differentiate them from user written constants. So either we check all literals increasing allocations or keep the current behaviour opening up edge cases.

So I would appreciate input on the issue for alternative solutions.

Last edited 2 years ago by AndreasK (previous) (diff)

comment:18 Changed 2 years ago by AndreasK

Resolution: fixed
Status: patchclosed

Replying to AndreasK:

In MatchLit dsLit doesn't use the mkMachInt functions resulting in potentially faulty code.

y = I# 0x8000000000000000# :: Int compiles without warning with -Wall for example.

Fixing this would incur a overhead when compiling Code which generates Prim Literals.

As these use the same code Path as user specified Literals we can't skip the checks on these even when we know these will not be out of bounds.

Closing this after a discussion on IRC and it's not deemed a big enough issue to warrant the overhead. (fixed as to not confuse this issue with the original one)

Last edited 2 years ago by AndreasK (previous) (diff)
Note: See TracTickets for help on using tickets.