compiler: make sure we reject -O + HscInterpreted
ClosedPublic

Authored by austin on Mar 12 2015, 2:53 PM.

Details

Summary

When using GHCi, we explicitly reject optimization, because the
compilers optimization passes can introduce unboxed tuples, which the
interpreter is not able to handle. But this goes the other way too: using
GHCi on optimized code may cause the optimizer to float out breakpoints
that the interpreter introduces. This manifests itself in weird ways,
particularly if you as an API client use custom DynFlags to introduce
optimization in combination with HscInterpreted.

It turns out we weren't checking for consistent DynFlag settings when
doing setSessionDynFlags, as Trac #10052 showed. While the main driver
handled it in DynFlags via parseDynamicFlags, we didn't check this
elsewhere.

This does a little refactoring to split out some of the common code, and
immunizes the various DynFlags utilities in the GHC module from this
particular bug. We should probably be checking other general invariants
too.

This fixes Trac #10052, and adds some notes about the behavior in GHC and
FloatOut

As a bonus, expose warningMsg from ErrUtils as a helper since it
didn't exist (somehow).

Signed-off-by: Austin Seipp <austin@well-typed.com>

Test Plan

iiam

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.
austin updated this revision to Diff 2436.Mar 12 2015, 2:53 PM
austin retitled this revision from to compiler: make sure we reject -O + HscInterpreted.
austin updated this object.
austin edited the test plan for this revision. (Show Details)
austin added reviewers: hvr, edsko, simonpj.
austin updated the Trac tickets for this revision.
edsko accepted this revision.Mar 12 2015, 3:13 PM
edsko edited edge metadata.
This revision is now accepted and ready to land.Mar 12 2015, 3:13 PM
thomie added inline comments.Mar 12 2015, 3:27 PM
compiler/main/DynFlags.hs
3866–3867

Does setOptLevel still need to check the optimization level, now that setSessionDynFlags already does so? Note that setSessionDynFlags is always called from Main.main'.

compiler/main/GHC.hs
660

This is different from what setOptLevel does (ignoring the flag). Why?

Two more questions:

  • What about these functions, maybe they should check the optimization level as well?
replaceDynFlags :: ContainsDynFlags t => t -> DynFlags -> t
setDynFlags :: DynFlags -> CompPipeline ()
setUnsafeGlobalDynFlags :: DynFlags -> IO ()
  • Main.main' also calls Main.checkOptions. It for example checks that '-prof' and '-interactive' are not used together. Shouldn't those checks also be done for API calls (maybe you're referring to this in the commit message).
austin planned changes to this revision.Mar 12 2015, 3:57 PM
In D727#20190, @thomie wrote:

Two more questions:

  • What about these functions, maybe they should check the optimization level as well?
replaceDynFlags :: ContainsDynFlags t => t -> DynFlags -> t
setDynFlags :: DynFlags -> CompPipeline ()
setUnsafeGlobalDynFlags :: DynFlags -> IO ()

Yes, they probably should.

  • Main.main' also calls Main.checkOptions. It for example checks that '-prof' and '-interactive' are not used together. Shouldn't those checks also be done for API calls (maybe you're referring to this in the commit message).

Yes, that's a good idea.

compiler/main/DynFlags.hs
3866–3867

Yes, I wondered about this. I think it's dead code now, but didn't remove it before submitting.

compiler/main/GHC.hs
660

DynFlags automatically sets optLevel to 0 by default in the initialization. This means if setOptLevel is called and detects a combo of -i + -O (e.g. optLevel = 1), it will return the original dflags, e.g. with 0 specified as the default optimization level, and will not update anything.

Here, when using the API, we specifically need to clear the optimization level. While someone might call parseDynamicFlags to get -O out of the command line (like T10052 does), someone could *also* just update the record and set optLevel = 2 unilaterally without any option, and we still need to reject that.

austin updated this object.Mar 19 2015, 11:58 AM
austin edited edge metadata.
This revision was automatically updated to reflect the committed changes.
In D727#20190, @thomie wrote:

Two more questions:

  • What about these functions, maybe they should check the optimization level as well?

    ` replaceDynFlags :: ContainsDynFlags t => t -> DynFlags -> t setDynFlags :: DynFlags -> CompPipeline () setUnsafeGlobalDynFlags :: DynFlags -> IO () `
  • Main.main' also calls Main.checkOptions. It for example checks that '-prof' and '-interactive' are not used together. Shouldn't those checks also be done for API calls (maybe you're referring to this in the commit message).

Okay, so it turns out fixing all of this was a bit tricker than expected when I tried, because of recursive dependencies with modules. I've pushed this patch in the mean time, but the remaining things should be fixed up (although, realistically, all those things are fairly internal).