Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#10320 closed bug (fixed)

-ddump-to-file should create empty dump files when there was nothing to dump

Reported by: rwbarton Owned by: kaiha
Priority: low Milestone: 8.0.1
Component: Compiler Version: 7.10.1
Keywords: newcomer Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D2015
Wiki Page:

Description (last modified by bgamari)

For example, I ran with -ddump-rule-firings -ddump-to-file when there were no rule firings. GHC should have created an empty foo.dump-rule-firings file. (Instead it left the existing foo.dump-rule-firings from a previous run intact, which could have been misleading if I didn't already know that the most recent run had no rule firings.)

Change History (19)

comment:1 Changed 5 years ago by MarceColl

Owner: set to MarceColl

comment:2 Changed 4 years ago by thomie

MarceColl: any progress? Pleas unassign yourself if you don't plan to work on this anymore.

comment:3 Changed 4 years ago by thomie

Owner: MarceColl deleted

Or just delete the old foo.dump-rule-firings file?

comment:4 Changed 4 years ago by tvv

Owner: set to tvv

I think it's better to keep one but truncate its size. At least it can show that flags were specified correctly and there's nothing to dump.

Seems that -ddump-rule-rewrites should be handled too.

comment:5 Changed 4 years ago by tvv

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

comment:6 Changed 4 years ago by Ben Gamari <ben@…>

In 8cba907a/ghc:

Create empty dump files when there was nothing to dump

This patch creates empty dump file when GHC was run with
`-ddump-rule-firings` (or `-ddump-rule-rewrites`) and `-ddump-to-file`
specified, and there were no rules applied. If dump already exists it
will be overwritten by empty one.

Test Plan: ./validate

Reviewers: austin, thomie, bgamari

Reviewed By: thomie, bgamari

Subscribers: thomie

Projects: #ghc

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

GHC Trac Issues: #10320

comment:7 Changed 4 years ago by osa1

This patch breaks things; try -ddump-to-file -ddump-simpl, it's failing with:

Main.dump-simpl: hPutStr: illegal operation (handle is closed)

I had to comment out bracket_s second argument that closes handles. It seems like it's closing handles too soon.

(We should probably add some tests that dump everything(ds, simpl, stg, cmm, asm...), just to make sure file operations are working)

Last edited 4 years ago by osa1 (previous) (diff)

comment:8 Changed 4 years ago by tvv

It happens because I forgot to clear map in closeDumpFiles. And yes, looks like I picked the wrong place to close handles.

This patch should help to avoid the error:

diff --git a/compiler/main/ErrUtils.hs b/compiler/main/ErrUtils.hs
index 5e585da..2d29452 100644
--- a/compiler/main/ErrUtils.hs
+++ b/compiler/main/ErrUtils.hs
@@ -333,8 +333,11 @@ openDumpFiles dflags
 --
 closeDumpFiles :: DynFlags -> IO ()
 closeDumpFiles dflags
-  = do gd <- readIORef $ generatedDumps dflags
+  = do
+       let gdref = generatedDumps dflags
+       gd <- readIORef gdref
        mapM_ hClose $ Map.elems gd
+       writeIORef gdref Map.empty

 -- | Write out a dump.
 -- If --dump-to-file is set then this goes to a file.

but the compiler will produce incomplete dumps in some cases.

Let me check it again and I will re-submit the patch.

Thank you.

comment:9 Changed 4 years ago by osa1

I tried to add a test for this, unfortunately as far as I can see this only happens when GHC panics(CoreLint errors may also trigger this, but I couldn't try this yet). Does anyone know a good way to add a test for this?

In the meantime, can we revert this patch until someone comes up with a proper solution that works even when GHC panics?

comment:10 Changed 4 years ago by tvv

How about to switch back to Phab:D1514 Diff 5330? It fixes the ticket's issue and does not introduce any side-effects like described. We can leave the dump optimization task as feature request.

Last edited 4 years ago by tvv (previous) (diff)

comment:11 Changed 4 years ago by tvv

Owner: tvv deleted

comment:12 Changed 4 years ago by tvv

Status: patchnew

comment:13 Changed 4 years ago by thomie

commit c5597bb6da612e0578290c3bccdac089d6519e19

Author: Ben Gamari <ben@smart-cactus.org>
Date:   Thu Dec 3 14:59:18 2015 +0100

    Revert "Create empty dump files when there was nothing to dump"
    
    This reverts commit 8cba907ad404ba4005558b5a8966390159938172 which
    broke `-ddump-to-file`.

comment:14 in reply to:  10 Changed 4 years ago by thomie

Replying to tvv:

How about to switch back to Phab:D1514 Diff 5330? It fixes the ticket's issue and does not introduce any side-effects like described.

pipeLoop could be throwing exceptions, and dump files should be closed when that happens (there could be ghc api users recovering from the exception).

Maybe someone else can have a look? It can't be that hard to fix this issue properly?

comment:15 Changed 4 years ago by kaiha

Owner: set to kaiha

comment:16 Changed 4 years ago by kaiha

Differential Rev(s): Phab:D1514Phab:D2015
Status: newpatch

comment:17 Changed 4 years ago by Ben Gamari <ben@…>

In 9f9345e5/ghc:

Create empty dump files (fixes #10320)

Test Plan: ./validate

Reviewers: austin, bgamari

Reviewed By: bgamari

Subscribers: thomie

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

GHC Trac Issues: #10320

comment:18 Changed 4 years ago by bgamari

Description: modified (diff)
Milestone: 8.0.1
Status: patchmerge

comment:19 Changed 4 years ago by bgamari

Resolution: fixed
Status: mergeclosed
Last edited 4 years ago by bgamari (previous) (diff)
Note: See TracTickets for help on using tickets.