Introduce flag -keep-hscpp-files (#10869)
ClosedPublic

Authored by RolandSenn on Jun 17 2018, 5:39 AM.

Details

Summary

Keep the output of the CPP preprocessor phase as .hscpp files. A .hscpp file is only created, if a module gets compiled and uses the preprocessor.

Test Plan

Testcase T10869

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.
RolandSenn created this revision.Jun 17 2018, 5:39 AM

This is my first contribution to GHC, where I wrote real Haskell code. Therefore this is just a draft version and it needs some additional improvments.

1: CPP processing. See: ./compiler/main/DriverPipeline.hs.runPhase (RealPhase (Cpp sf)) (ca line 940):
When the -keep-hspp-file flag is set, the output file of the CPP preprocessor is copied to the *.hspp file.

2: END processing See: ./compiler/main/DriverPipeline.hs.pipeLoop (ca line 680):
The output of the last compiler phase is copied. With the -E option, GHC processing stops early and copies the output of the CPP phase to the *.hspp file.
If the user specifies both flags -E and -keep-hspp-file, then the file will be copied a first time in my new code during CPP processing and a second time here.
This is bad! Therefore I added code to avoid duplicate copying.
However, with the -E flag set, it would be better to skip the copying during CPP processing.
Unfortunately I was unable to check the -E flag in the CPP processing code! The "Mode Flags" like the -E flag, only exist in ./ghc/Main.hs.
I didn't find anything like gopt for the mode flags!! How can I check them??
I looked at the 'stop_phase' field in the PipeEnv Structure, but with or without the -E flag, it has the same value (Hsc).

Please advice! Many thanks!

bgamari retitled this revision from Docu for flag: keep-hspp-files (#10869) to Introduce flag -keep-hspp-files (#10869).Jun 17 2018, 8:25 AM
bgamari updated the Trac tickets for this revision.
bgamari requested changes to this revision.Jun 17 2018, 9:40 AM

Thanks for your contribution @RolandSenn!

The patch itself looks great. However, do you suppose you could amend the differential summary to describe what the patch is doing?

docs/users_guide/separate_compilation.rst
415

Can you wrap the .hspp in quotes? Also, replace "precompiler" with "preprocessor".

This revision now requires changes to proceed.Jun 17 2018, 9:40 AM
RolandSenn updated this revision to Diff 16989.Jun 17 2018, 2:39 PM
RolandSenn marked an inline comment as done.Jun 17 2018, 2:44 PM

I added the requested changes by bgamari and by the linter. Please feel free to request additional changes!

@bgamari : You wrote:

However, do you suppose you could amend the differential summary to describe what the patch is doing?

What do you exactly mean by this? Should I change/improve the description in docs/users_guide/separate_compilation.rst? Or do you simply mean the two little changes (quotes and preprocessor)? Please advice! Many thanks!

@bgamari : You wrote:

However, do you suppose you could amend the differential summary to describe what the patch is doing?

What do you exactly mean by this? Should I change/improve the description in docs/users_guide/separate_compilation.rst? Or do you simply mean the two little changes (quotes and preprocessor)? Please advice! Many thanks!

The differential summary (the bit shown under the "Summary" heading of this page) will become the commit message when this patch is landed. I was simply suggesting that you add a bit of text describing the goal of the patch for posterity.

Hello Ben,
I'll do this. But this week I'm away from my desktop computer. So maybe
it will take till mid next week. 
Thanks and best wishes.   Roland

RolandSenn edited the summary of this revision. (Show Details)Jun 25 2018, 11:53 AM
thomie requested changes to this revision.Aug 4 2018, 10:35 AM

@RolandSenn: sorry for the late reply.

The phases go like this:

  • Unlit (.lhs -> .hs)
  • Cpp (.hs -> .hscpp)
  • HsPp (.hscpp -> .hspp)
  • Hsc (.hspp -> ...)

So it is the HsPp phase (https://downloads.haskell.org/~ghc/8.4.3/docs/html/users_guide/phases.html?highlight=hspp#options-affecting-a-haskell-pre-processor) that outputs .hspp, not the Cpp phase, as your patch seems to imply.

It is probably better to compare -keep-hspp-files with the likes of -keep-hc/s/llvm-files, instead of with -E. -E is special, in that it stops compilation early:

, defFlag "E"            (PassFlag (setMode (stopBeforeMode anyHsc)))
compiler/main/DriverPipeline.hs
767

I think adding keep_hspp = gopt Opt_KeepHsppFiles dflags here could make your patch much shorter.

This revision now requires changes to proceed.Aug 4 2018, 10:35 AM

@thomie: Many thanks for your code review and your kind hints to improve the patch.

Your table about the phases and the corresponding output file names suggests, that we should have 2 new flags:
-keep-hscpp-file: it will keep the output file of the C pre-processor and name it *.hscpp
-keep-hspp-file: it will keep the output file of the Haskell pre-processor (specified with the -F and -pgmF flags) and name it *.hspp.

The file produced by the -E flag however, will continue to be named *.hspp, although it is the output of the C pre-processor. So I will NOT change the behaviour of the -E flag!

thomie added a comment.Aug 7 2018, 2:33 PM

The file produced by the -E flag however, will continue to be named *.hspp, although it is the output of the C pre-processor.

Not quite.

-E stops after the HsPp phase, and produces a *.hspp file.

Phases always follow upon each other.

Unlit -> CPP -> HsPp

So the file that -E produces has undergone all 3 of Unlit and Cpp and HsPp, in that order.

See also the function runPhase (+ docstring).

we should have 2 new flags

Two new flags seems excessive, unless someone is asking for it.

RolandSenn updated this revision to Diff 17609.Aug 8 2018, 8:31 AM
  • Improve patch after code review (Trac #10869).
  • Rename flag form keep-hspp-files to kepp-hscpp-files.
  • Do not copy the file but reuse code from the keep-hc/s/llvm-files flags.
RolandSenn retitled this revision from Introduce flag -keep-hspp-files (#10869) to Introduce flag -keep-hscpp-files (#10869).Aug 8 2018, 8:33 AM
RolandSenn edited the summary of this revision. (Show Details)
RolandSenn marked an inline comment as done.EditedAug 8 2018, 8:40 AM

@thomie The patch is now much shorter. Thanks for your kind help.

thomie accepted this revision.Aug 8 2018, 12:28 PM

LGTM

I guess specifying -E together with -keep-hscpp-files will write both files? That's ok though.

I guess specifying -E together with -keep-hscpp-files will write both files?

Yes! There exists a use case for this: If you additionally specifiy the -F and -pgmF=XYZ flags, then you see in the *.hscpp file the input and in the *.hspp file the output of your XYZ pre-processor.

RolandSenn: one last thing, before @bgamari gets here. Maybe you could add a blurp to the release notes about this new flag.

docs/users_guide/8.8.1-notes.rst

RolandSenn updated this revision to Diff 17645.Aug 12 2018, 6:23 AM

@thomie Thanks for the hint. I have added a line to the 8.8.1 release notes.

bgamari accepted this revision.Aug 21 2018, 11:18 AM

This looks great!

Thanks for the reviewing @thomie.

This revision is now accepted and ready to land.Aug 21 2018, 11:18 AM
This revision was automatically updated to reflect the committed changes.