Details

Reviewers

RyanGlScott
bgamari
hvr

Commits

rGHCdcf1f9268f6b: Fix #15953 by consistently using dumpIfSet_dyn to print debug output

Trac Issues

#15953

Summary

In some modules we directly dump the debugging output to STDOUT
via 'putLogMsg', 'printInfoForUser' etc. However, if -ddump-to-file
is enabled, that output should be written to a file. Easily fixed.

Certain tests (T3017, Roles3, T12763 etc.) expect part of the
output generated by -ddump-types to be in 'PprUser' style. However,
generally we want all other debugging output to use 'PprDump'
style. traceTcRn and traceTcRnForUser help us accomplish this.

This patch also documents some missing flags in the users guide.

Diff Detail

Repository

rGHC Glasgow Haskell Compiler

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

ckoparkar created this revision.Nov 26 2018, 10:54 AM

Herald added subscribers: carter, rwbarton. · View Herald TranscriptNov 26 2018, 10:54 AM

As @RyanGlScott pointed out, there are a few other places where when (dopt ...) print ... is used directly. Fixing those now.

Harbormaster failed remote builds in B25289: Diff 18892!Nov 26 2018, 11:12 AM

@ckoparkar, do you need help with this?

Fix other instances of when (dopt ...) print ....

Herald added a reviewer: hvr. · View Herald TranscriptNov 26 2018, 4:53 PM

Harbormaster failed remote builds in B25291: Diff 18898!Nov 26 2018, 5:11 PM

@bgamari I started looking at DriverPipeline, and got distracted.
Also, even after this patch, '-ddump-mod-map' (used in Packages.mkPackageState) still prints the output to STDOUT,
because dumpPrefix is still Nothing. Does DriverPipeline.runPipeline not run before we get to mkPackageState?
On the other hand, it works ok if we use -ddump-file-prefix=blah by hand.

Another unrelated question: if we set -ddump-file-prefix=FOO, the dump* files are missing a period in their name: e.g. FOOdump-ds.
Should this be updated to be FOO.dump* ? Or is the idea that whatever the user gave must be correct.

Also, this patch is ready for review :)

ckoparkar retitled this revision from Fix #15953 by using removing certain uses of putLogMsg to Fix #15953 by consistently using dumpIfSet_dyn to print debug output.Nov 26 2018, 5:13 PM

ckoparkar edited the summary of this revision. (Show Details)

ckoparkar added inline comments.Nov 26 2018, 5:22 PM

compiler/main/InteractiveEval.hs
581	I'm a bit confused about how is flag supposed to be used. Via GHCI ?
compiler/typecheck/TcRnDriver.hs
2669	This is a bit sketchy. We dump some information if either of these flags is enabled, but only pass `Opt_D_dump_types` to `traceTcRn`. The flag that we pass to `traceTcRn` is subsequently used to determine the suffix of the `dump` filename. In this case, `short_dump` would always be written to'X.dump-types'. Is this OK ? I wanted to avoid creating two separate files with the same information.
docs/users_guide/debugging.rst
621	I forgot to add a really easy one: `-ddump-file-prefix`. I'll do it when I update this patch after a round of review.
testsuite/tests/utils/should_run/Makefile
5	What's the right place for this test?

(fixing the build)

Fixed some failing tests using 'traceTcRnForUser'.

ckoparkar edited the summary of this revision. (Show Details)Nov 27 2018, 6:55 PM

ckoparkar marked an inline comment as done.

Harbormaster failed remote builds in B25306: Diff 18914!Nov 27 2018, 7:12 PM

LGTM, although one thing has me puzzled. (See my inline comment beginning with "Remind me"...)

compiler/main/InteractiveEval.hs
581	My best guess is that this is intended to be used in tandem with the interactive debugger (although I'll admit that I've never used this flag myself).
compiler/typecheck/TcRnDriver.hs
2669	That is a bit sketchy, but I don't think it's worth fretting over this too much (no one has complained about this yet, at least). At the very least, it now actually prints to some `dump` file when requested.
docs/users_guide/debugging.rst
586	Hooray for documentation!
testsuite/tests/roles/should_compile/all.T
1 ↗	(On Diff #18914)	Remind me why these were switched from `-ddump-tc` to `-ddump-types`? (I can't tell if this was motivated by a particular problem or not.)
testsuite/tests/utils/should_run/Makefile
5	I'd say this is as good of a place as any. (One of GHC's most poorly guarded secrets is that there's no real rhyme or reason behind the organization of `testsuite`'s subdirectories.)

ckoparkar added inline comments.Nov 28 2018, 7:52 AM

testsuite/tests/roles/should_compile/all.T
1 ↗	(On Diff #18914)	Not really. This was a result of our discussion that we shouldn't test the output of `-ddump-tc`. Should I revert this part ?

RyanGlScott added inline comments.Nov 28 2018, 7:57 AM

testsuite/tests/roles/should_compile/all.T
1 ↗	(On Diff #18914)	Ah. I think I was under the false impression that the output of `-ddump-tc` was constantly changing and inherently brittle to test. But it turns out I was wrong—I was thinking of `-ddump-tc-trace`, not `-ddump-tc`. In light of this, I'd opt to keep using `-ddump-tc` here.

Bring back -ddump-tc.

@RyanGlScott Oops, sorry. Fixed it :)

Macro andtherewasmuchrejoicing:

This revision is now accepted and ready to land.Nov 28 2018, 8:24 AM

Harbormaster failed remote builds in B25309: Diff 18918!Nov 28 2018, 8:30 AM

Closed by commit rGHCdcf1f9268f6b: Fix #15953 by consistently using dumpIfSet_dyn to print debug output (authored by ckoparkar, committed by RyanGlScott). · Explain WhyNov 29 2018, 6:45 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		compiler/deSugar/Coverage.hs (6 lines)
M		compiler/ghci/Debugger.hs (16 lines)
M		compiler/main/ErrUtils.hs (27 lines)
M		compiler/main/InteractiveEval.hs (6 lines)
M		compiler/main/Packages.hs (7 lines)
M		compiler/main/TidyPgm.hs (16 lines)
M		compiler/simplCore/SimplMonad.hs (9 lines)
M		compiler/typecheck/TcRnDriver.hs (2 lines)
M		compiler/typecheck/TcRnMonad.hs (32 lines)
M		docs/users_guide/debugging.rst (42 lines)
M		testsuite/tests/utils/should_run/Makefile (10 lines)
A	M	testsuite/tests/utils/should_run/T15953.hs (9 lines)
M		testsuite/tests/utils/should_run/all.T (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	18892	7e57067		Nov 26 2018, 10:54 AM	★	★
Diff 2	18898	7e57067	Fix other instances of `when (dopt ...) print ...`.	Nov 26 2018, 4:53 PM	★	★
Diff 3	18914	7e57067	Fixed some failing tests using 'traceTcRnForUser'.	Nov 27 2018, 6:54 PM	★	★
Diff 4	18918	7e57067	Bring back `-ddump-tc`.	Nov 28 2018, 8:12 AM	★	★
Diff 5	18942	9e3aaf8	rGHCdcf1f9268f6b338997f2c03891d7bc57da2ee78a	Nov 29 2018, 5:45 PM	★	★

Fix #15953 by consistently using dumpIfSet_dyn to print debug output
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 18942

compiler/deSugar/Coverage.hs

compiler/ghci/Debugger.hs

compiler/main/ErrUtils.hs

compiler/main/InteractiveEval.hs

compiler/main/Packages.hs

compiler/main/TidyPgm.hs

compiler/simplCore/SimplMonad.hs

compiler/typecheck/TcRnDriver.hs

compiler/typecheck/TcRnMonad.hs

docs/users_guide/debugging.rst

testsuite/tests/utils/should_run/Makefile

testsuite/tests/utils/should_run/T15953.hs

testsuite/tests/utils/should_run/all.T

Fix #15953 by consistently using dumpIfSet_dyn to print debug outputClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 18942

compiler/deSugar/Coverage.hs

compiler/ghci/Debugger.hs

compiler/main/ErrUtils.hs

compiler/main/InteractiveEval.hs

compiler/main/Packages.hs

compiler/main/TidyPgm.hs

compiler/simplCore/SimplMonad.hs

compiler/typecheck/TcRnDriver.hs

compiler/typecheck/TcRnMonad.hs

docs/users_guide/debugging.rst

testsuite/tests/utils/should_run/Makefile

testsuite/tests/utils/should_run/T15953.hs

testsuite/tests/utils/should_run/all.T

Fix #15953 by consistently using dumpIfSet_dyn to print debug output
ClosedPublic
Actions

Revision Contents
Changeset List