Opened 3 years ago

Closed 2 years ago

Last modified 10 months ago

#10052 closed bug (fixed)

Panic (something to do with floatExpr?)

Reported by: edsko Owned by: simonmar
Priority: high Milestone: 7.10.2
Component: Compiler Version: 7.10.1-rc2
Keywords: Cc: scpmw, simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHCi crash Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D727
Wiki Page:

Description

Loading

main = let (x :: String) = "hello" in putStrLn x

using a very simple driver for the GHC API (see T145.hs) causes a ghc panic:

[1 of 1] Compiling Main             ( T145-input.hs, interpreted )
T145: T145: panic! (the 'impossible' happened)
  (GHC version 7.10.0.20150128 for x86_64-apple-darwin):
	floatExpr tick
<<details unavailable>>

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

This panic is arising in our test case for #8333, so it may be related to that bug.

Attachments (2)

T145.hs (910 bytes) - added by edsko 3 years ago.
GHC API driver
T145-input.hs (49 bytes) - added by edsko 3 years ago.
Input to the driver

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by edsko

Attachment: T145.hs added

GHC API driver

Changed 3 years ago by edsko

Attachment: T145-input.hs added

Input to the driver

comment:1 Changed 3 years ago by edsko

Call the driver with

# ./T145 ./T145-input.hs -XScopedTypeVariables -O

to cause the panic (no panic happens if you don't specify -O).

comment:2 Changed 3 years ago by simonpj

Peter Wortman says: We are clearly trying to float past a breakpoint here, which is simply impossible. Pretty sure this would have been a panic before my changes too (it would have tried to "mkNoCount" the breakpoint). Guess I was wrong reading a "breakpoints don't appear here" invariant out of that... The quick fix would be to drop all floats in-place:

    -- scoped, counting and unsplittable, can't be floated through
    | otherwise
    = floatBody tOP_LEVEL expr

This fixes the panic, but is a bit awkward. Probably better to change SetLevels? Not a piece of code I'm very familiar with...

comment:3 Changed 3 years ago by simonpj

Peter says (further)

Why do you say "we are clearly trying to float past a breakpoint"? Why is it so clear?

It's the only kind of Tickish that is scoped, counting and not splittable.

This means that we can't

  • Simply float code out of it, because the payload must still be covered (scoped)
  • Copy the tick, because it would change entry counts (here: duplicate breakpoints)

Why is it wrong to float a lazy thunk out of a breakpoint?

Good questions - I haven't given breakpoint semantics a lot of thought, to be honest. My assumption was that most optimisation passes would never see them. And where they did, they should just leave them in peace as much as possible.

For whatever it's worth, the source cautions against making breakpoints unscoped:

    -- Breakpoints are scoped: eventually we're going to do call
    -- stacks, but also this helps prevent the simplifier from moving
    -- breakpoints around and changing their result type (see #1531).

Hm. We might try to make them pseudo-splittable, with non-counting breakpoints being NOPs for now? This might still allow us to implement breakpoint-based stack traces if we really want them...

comment:4 Changed 3 years ago by simonpj

Milestone: 7.10.1
Owner: set to simonmar, peter wortman
Priority: normalhigh

Peter, Simon M: can you work together to fix this? A flat-out panic is really not good.

Simon

comment:5 Changed 3 years ago by thoughtpolice

Cc: scpmw added
Owner: changed from simonmar, peter wortman to simonmar

comment:6 Changed 3 years ago by simonpj

Cc: simonmar added

comment:7 Changed 3 years ago by simonmar

I think the problem here is that you're trying to use -O in conjunction with the interpreter, which we normally don't do because optimisation can introduce unboxed tuples, which the interpreter can't handle. I suspect that you've uncovered another case that isn't handled when we try to use -O with the interpreter, but since we don't have any plans to handle that combination I'm not inclined to try to fix this particular problem.

We will look into why we aren't catching the -O/interpreter combination and make it fail in a civilized way.

comment:8 Changed 3 years ago by simonpj

So the fixes needed are:

  • Complain in a civilised way if you are using the interpreter with -O. Add a clear Note to explain why, enumerating all the things (that we know of) which can go wrong.
  • Add a clear Note in FloatOut to explain that the un-handled case concerns breakpoints (which isn't at all clear right now), and that breakpoints means the interpreter, and the interpreter means no -O (refer to Note from the first bullet).

comment:9 Changed 3 years ago by thoughtpolice

(For interested readers, the check we currently have is in setOptLevel in DynFlags.hs)

comment:10 Changed 3 years ago by simonpj

Is anyone willing to execute on this? Simple, a little fiddly. Thanks

Simon

comment:11 Changed 3 years ago by thoughtpolice

Differential Rev(s): Phab:D727
Status: newpatch

comment:12 Changed 3 years ago by thoughtpolice

Milestone: 7.10.17.10.2

Moving to GHC 7.10.2.

comment:13 Changed 3 years ago by George

Type of failure: None/UnknownCompile-time crash

comment:14 Changed 3 years ago by George

Type of failure: Compile-time crashGHCi crash

comment:15 Changed 3 years ago by Austin Seipp <austin@…>

In 9736c042f4292b4fb94ca9faca6a010372a0f92f/ghc:

compiler: make sure we reject -O + HscInterpreted

When using GHCi, we explicitly reject optimization, because the
compilers optimization passes can introduce unboxed tuples, which the
interpreter is not able to handle. But this goes the other way too: using
GHCi on optimized code may cause the optimizer to float out breakpoints
that the interpreter introduces. This manifests itself in weird ways,
particularly if you as an API client use custom DynFlags to introduce
optimization in combination with HscInterpreted.

It turns out we weren't checking for consistent DynFlag settings when
doing `setSessionDynFlags`, as #10052 showed. While the main driver
handled it in `DynFlags` via `parseDynamicFlags`, we didn't check this
elsewhere.

This does a little refactoring to split out some of the common code, and
immunizes the various `DynFlags` utilities in the `GHC` module from this
particular bug. We should probably be checking other general invariants
too.

This fixes #10052, and adds some notes about the behavior in `GHC` and
`FloatOut`

As a bonus, expose `warningMsg` from `ErrUtils` as a helper since it
didn't exist (somehow).

Signed-off-by: Austin Seipp <austin@well-typed.com>

Reviewed By: edsko

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

GHC Trac Issues: #10052

comment:16 Changed 3 years ago by thoughtpolice

Status: patchmerge

comment:17 Changed 3 years ago by Austin Seipp <austin@…>

In b199536be25ea046079587933cc73d0a948a0626/ghc:

compiler: make sure we reject -O + HscInterpreted

When using GHCi, we explicitly reject optimization, because the
compilers optimization passes can introduce unboxed tuples, which the
interpreter is not able to handle. But this goes the other way too: using
GHCi on optimized code may cause the optimizer to float out breakpoints
that the interpreter introduces. This manifests itself in weird ways,
particularly if you as an API client use custom DynFlags to introduce
optimization in combination with HscInterpreted.

It turns out we weren't checking for consistent DynFlag settings when
doing `setSessionDynFlags`, as #10052 showed. While the main driver
handled it in `DynFlags` via `parseDynamicFlags`, we didn't check this
elsewhere.

This does a little refactoring to split out some of the common code, and
immunizes the various `DynFlags` utilities in the `GHC` module from this
particular bug. We should probably be checking other general invariants
too.

This fixes #10052, and adds some notes about the behavior in `GHC` and
`FloatOut`

As a bonus, expose `warningMsg` from `ErrUtils` as a helper since it
didn't exist (somehow).

Signed-off-by: Austin Seipp <austin@well-typed.com>

Reviewed By: edsko

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

GHC Trac Issues: #10052

comment:18 Changed 2 years ago by Austin Seipp <austin@…>

In 091944e3aec736b440a9c1204f152004e382c967/ghc:

compiler: make sure we reject -O + HscInterpreted

When using GHCi, we explicitly reject optimization, because the
compilers optimization passes can introduce unboxed tuples, which the
interpreter is not able to handle. But this goes the other way too: using
GHCi on optimized code may cause the optimizer to float out breakpoints
that the interpreter introduces. This manifests itself in weird ways,
particularly if you as an API client use custom DynFlags to introduce
optimization in combination with HscInterpreted.

It turns out we weren't checking for consistent DynFlag settings when
doing `setSessionDynFlags`, as #10052 showed. While the main driver
handled it in `DynFlags` via `parseDynamicFlags`, we didn't check this
elsewhere.

This does a little refactoring to split out some of the common code, and
immunizes the various `DynFlags` utilities in the `GHC` module from this
particular bug. We should probably be checking other general invariants
too.

This fixes #10052, and adds some notes about the behavior in `GHC` and
`FloatOut`

As a bonus, expose `warningMsg` from `ErrUtils` as a helper since it
didn't exist (somehow).

Signed-off-by: Austin Seipp <austin@well-typed.com>

Reviewed By: edsko

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

GHC Trac Issues: #10052

comment:19 Changed 2 years ago by thoughtpolice

Resolution: fixed
Status: mergeclosed

Merged to ghc-7.10.

comment:20 Changed 2 years ago by Sergei Trofimovich <siarheit@…>

In 2c6a0411dcd921ea6ec1cbe5eaf93d17adcf33a2/ghc:

Fix a couple of tests for GHCi/-O* (Trac #10052)

Tests use unboxed types (or optimizer gets to them),
those can't be handled by ghci. Fixed by using -fobject-code.

Signed-off-by: Sergei Trofimovich <siarheit@google.com>

comment:21 Changed 10 months ago by Ben Gamari <ben@…>

In 44f079f7/ghc:

FloatOut: Allow floating through breakpoint ticks

I believe this is actually a completely valid thing to do, despite the
arguments put forth in #10052. All that was missing was logic in
SetLevels to correctly substitute the cloned binders into the
breakpoint's free variable list.

This is a prerequisite for enabling StaticPointer support in the
interpreter.

Test Plan: Validate

Reviewers: austin, scpmw

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D3049
Note: See TracTickets for help on using tickets.