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

Authored by ckoparkar on Mon, Nov 26, 10:54 AM.

Details

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.Mon, Nov 26, 10:54 AM
ckoparkar planned changes to this revision.Mon, Nov 26, 11:08 AM

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

@ckoparkar, do you need help with this?

ckoparkar updated this revision to Diff 18898.Mon, Nov 26, 4:53 PM

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

@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.Mon, Nov 26, 5:13 PM
ckoparkar edited the summary of this revision. (Show Details)
ckoparkar added inline comments.Mon, Nov 26, 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?

ckoparkar planned changes to this revision.Tue, Nov 27, 7:55 AM

(fixing the build)

ckoparkar updated this revision to Diff 18914.Tue, Nov 27, 6:54 PM

Fixed some failing tests using 'traceTcRnForUser'.

ckoparkar edited the summary of this revision. (Show Details)Tue, Nov 27, 6:55 PM
ckoparkar marked an inline comment as done.

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.Wed, Nov 28, 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.Wed, Nov 28, 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.

ckoparkar updated this revision to Diff 18918.Wed, Nov 28, 8:12 AM

Bring back -ddump-tc.

@RyanGlScott Oops, sorry. Fixed it :)

This revision is now accepted and ready to land.Wed, Nov 28, 8:24 AM
This revision was automatically updated to reflect the committed changes.