#8987 closed bug (fixed)

TH: panic in reportWarning when the message throws an exception

Reported by: Yuras Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.8.1
Keywords: Cc: shumovichy@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time crash Test Case: th/T8987
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

reportWarning and reportError doesn't force it's argument, so ghc panics when printing the message.

To reproduce:

test :: Q [Dec]
test = do
  reportWarning undefined
  return []

Output:

main.hs:5:3: Warning:ghc: panic! (the 'impossible' happened)
  (GHC version 7.8.1 for x86_64-unknown-linux):
	Prelude.undefined

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

The fix could be:

--- a/compiler/typecheck/TcSplice.lhs
+++ b/compiler/typecheck/TcSplice.lhs
@@ -875,8 +875,8 @@ instance TH.Quasi (IOEnv (Env TcGblEnv TcLclEnv)) where
                   ; let i = getKey u
                   ; return (TH.mkNameU s i) }
 
-  qReport True msg  = addErr  (text msg)
-  qReport False msg = addWarn (text msg)
+  qReport True !msg  = addErr  (text msg)
+  qReport False !msg = addWarn (text msg)
 
   qLocation = do { m <- getModule
                  ; l <- getSrcSpanM

But possibly it should deepseq the argument?

Attachments (3)

Change History (13)

comment:1 Changed 16 months ago by Yuras

  • Cc shumovichy@… added

comment:2 follow-up: Changed 16 months ago by simonpj

I have not thought about this deeply, but wouldn't something very similar happen with your change? The program will still crash, won't it?

Would you like to give a test case?

In any case, you are right that if you are going to do this at all, then something deepseq-like would be more thorough.

By all means send a patch (with comments to explain). Thanks.

Simon

comment:3 in reply to: ↑ 2 Changed 16 months ago by Yuras

Replying to simonpj:

I have not thought about this deeply, but wouldn't something very similar happen with your change? The program will still crash, won't it?

No, ghc is prepared for exceptions in splice, including nested exceptions. See Note [Exceptions in TH] and Note [Concealed TH exceptions] So with my change compilation gracefully fails with the next message:

main.hs:1:1:
    Exception when trying to run compile-time code:
      Prelude.undefined
    Code: test

Would you like to give a test case?

In any case, you are right that if you are going to do this at all, then something deepseq-like would be more thorough.

By all means send a patch (with comments to explain). Thanks.

Well, looks like it is not possible to use deepseq in ghc. Should I implement ad-hoc solution? Something like deepseqString? I don't believe it is the first time we need deepseq, so most likely it is already implemented somewhere.

Thanks, Yuras

comment:4 Changed 16 months ago by simonpj

Yes, I see a seqString in AsmCodeGen, so perhaps pull it out into Utils and use it.

Thanks

Simon

Changed 16 months ago by Yuras

comment:5 Changed 16 months ago by Yuras

  • Status changed from new to patch

comment:6 follow-up: Changed 16 months ago by simonpj

Looks great. But where is Note [Exceptions in TH]? If you could just add that, it'd be great.

Simon

comment:7 in reply to: ↑ 6 Changed 16 months ago by Yuras

Replying to simonpj:

Looks great. But where is Note [Exceptions in TH]? If you could just add that, it'd be great.

It is already here, 50 lines above. It perfectly explains how TH handles exceptions, I have little to nothing to add.

I don't think we need special Note about qReport, but I added few lines to an existent Note. But I'm not sure it worth adding, the notes are pretty clear already.

Let me know if prefer one patch, combining all the 3 attached.

comment:8 Changed 16 months ago by Simon Peyton Jones <simonpj@…>

In 241c6601568969156403fde8089c97024b082de0/ghc:

Make qReport force its error message before printing it

Fixes Trac #8987.  See Note [Exceptions in TH]

Thanks to Yuras Shumovich for doing this.

comment:9 Changed 16 months ago by Yuras

  • Test Case set to th/T8987

Should it be marked as closed or merge?

comment:10 Changed 16 months ago by simonpj

  • Resolution set to fixed
  • Status changed from patch to closed
Note: See TracTickets for help on using tickets.