Recompilation check should include flags

[simonpj]

GHC skips recompilation of a module if the module code 
hasn't changed, even if the flags in use *have* changed.  
A benign version is that switching on -O won't recompile 
modules that were compiled without -O.  But here's a 
nastier version: changing the -main-is flag can lead to an 
obscure link error.  Details below supplied by Niels 
[cpjvelde@cs.uu.nl]

Simon

Actually, i reproduced it now and the reason is a bit 
different. I have an
application Test and Test2. They both have a main 
function. Test imports Test2
for some other function. So this is how those files look 
like:

~/pancake > cat Test.hs
module Test where
import Test2 hiding (main)

main = doit

~/pancake > cat Test2.hs
module Test2 where

doit = print "Test2.doit"
main = print "Test2.main"

I then first compile the first app:
~/pancake > ghc --make -main-is Test.main Test.hs
Chasing modules from: Test.hs
Compiling Test2            ( ./Test2.hs, ./Test2.o )
Compiling Test             ( Test.hs, Test.o )
Linking ...

then i compile the second app:
~/pancake > ghc --make -main-is Test2.main Test2.hs
Chasing modules from: Test2.hs
Skipping  Test2            ( Test2.hs, Test2.o )
Linking ...
/usr/lib/ghc-6.4/libHSrts.a(Main.o)(.text+0xe): In function 
`main':
: undefined reference to `__stginit_ZCMain'
/usr/lib/ghc-6.4/libHSrts.a(Main.o)(.text+0x28): In 
function `main':
: undefined reference to `ZCMain_main_closure'
collect2: ld returned 1 exit status

So I guess generating Test2.o the first time and using -
main-is renamed the main
in Test2.o . Since it is not recompiled when I want to 
compile the second app,
it fails because it cant find the main...I thought providing 
a -main-is flag
again when compiling the second app would somehow 
force recompilation of Test2.o
or at least fixing the 'renaming' that the first compilation 
of Test2.o had 
caused.

greetings, Niels.

[igloo]

See also #106.

[dterei]

Was doing some work in recompilation avoidance for the SafeHaskell work and came over this bug. I was thinking, the easiest way to fix this would be to store a new fingerprint in the interface file for a module that is a hash of the flags that if changed should trigger recompilation. So '-main-is', probably '-O' should be part of the hash but '-Wall' shouldn't for example. If this sounds like a reasonable plan then I would be happy to code it up since it would be useful somewhat to the SafeHaskell work. Recompilation of a module on this condition I don't think should trigger recompilation of modules depending on it though.

[simonmar]

Replying to dterei:

I was thinking, the easiest way to fix this would be to store a new fingerprint in the interface file for a module that is a hash of the flags that if changed should trigger recompilation. So '-main-is', probably '-O' should be part of the hash but '-Wall' shouldn't for example. If this sounds like a reasonable plan then I would be happy to code it up since it would be useful somewhat to the SafeHaskell work. Recompilation of a module on this condition I don't think should trigger recompilation of modules depending on it though.

Sounds good. You would need to be careful to normalise the flags (sort them), and as you say filter out any flags that shouldn't trigger recompilation. It's not clear to me that changing optimisation settings should trigger recompilation: for example, you might have a large program compiled unoptimised, and then decide that you want to optimise just one or two modules, so you remove a few .o files, add -O and recompile.

Rather than using the text of the flags themselves, you might want to serialise the internal representation, as we do for the linker flags (see DriverPipeline.getLinkInfo).

[igloo]

Flag ordering is important (e.g. -O0 -O is not the same as -O -O0). Also, some flags are equivalent to others.

Rather than storing the set of flags (-O etc), it might be better to use the set of enabled DynFlags (Opt_Specialise etc) and ExtensionFlags.

It might also make sense to split DynFlag into 2 (or more) types, so e.g. Opt_D_dump* and Opt_Warn* are in a different type than the flags that actually affect optimisation etc. Then the recompilation checker can just look at the set of values of the second type (and the ExtensionFlag type). That may mean it sometimes recompiles when it isn't strictly necessary, but I don't think that's a major problem.

[simonmar]

Replying to igloo:

Flag ordering is important (e.g. -O0 -O is not the same as -O -O0). Also, some flags are equivalent to others.

You're right, literally sorting the flags would be wrong. I had in mind serialising DynFlags?, but making sure that the DynFlag? list is sorted to normalise it, and omitting things that don't affect the compilation result, plus throwing in a few things that aren't in DynFlags? (static flags).

Rather than storing the set of flags (-O etc), it might be better to use the set of enabled DynFlags (Opt_Specialise etc) and ExtensionFlags.

Right.

[dterei]

So I'm finally actually working on this. There are some questions though:

• Should optmisation flag changes be candidates for recompilation?

• There is the whole hierarchy of hashes GHC uses (Interface File Hash, ABI Hash and then individual more specialised hashes). I've created a new flags hash. Should the ABI and Interface File Hash be dependent on the flags hash? The interface file hash should be. However I think for the ABI hash we could do something a little more complex if its worth the effort. There are probably some flags that if changed we want it to trigger a recompilation of the module M but we don't want the modules ABI hash to change and trigger recompilation of modules depending on M. However some flags like '--main-is' should change M's ABI hash. Is it worth differentiating between these two flag cases?

[dterei]

Other flags not sure if we should be including in the hash:

• includePaths :: [String]
• libraryPaths :: [String]
• frameworkPaths :: [String]
• cmdlineFrameworks :: [String]

• rtsOpts :: Maybe String
• rtsOptsEnabled :: RtsOptsEnabled?

• hpcDir :: String

• settings :: Settings -- contains the programs and options to use for various phases

• packageFlags :: [PackageFlag?]

• language :: [Maybe Language]

[davidterei@…]

commit 55991bf6631c95b620aec23ae50d25968f4a7f6e

Author: David Terei <davidterei@gmail.com>
Date:   Wed Nov 9 22:53:07 2011 -0800

    Fix #437: recompilation check includes flags

 compiler/ghc.cabal.in         |    1 +
 compiler/iface/BinIface.hs    |    4 +
 compiler/iface/FlagChecker.hs |  143 ++++++++++++++++++++++++++++++++
 compiler/iface/LoadIface.lhs  |    1 +
 compiler/iface/MkIface.lhs    |  184 ++++++++++++++++++++++++-----------------
 compiler/main/HscTypes.lhs    |    5 +-
 compiler/utils/Binary.hs      |   32 +++++---
 7 files changed, 282 insertions(+), 88 deletions(-)

[dterei]

Fixed. But there is a question of which flags should or shouldn't be included in the check.

[marlowsd@…]

commit 9df7f9b4022ffda25f004cb099de358d7d240fc1

Author: Simon Marlow <marlowsd@gmail.com>
Date:   Thu Nov 10 11:14:20 2011 +0000

    Add more flags to the recompilation check (#437)
    
    Now included:
        - all language flags
        - all filename-related flags (-i, -osuf, -hidir etc.)
        - all CPP-related flags (-I, -D, -U)

 compiler/iface/FlagChecker.hs |   40 +++++++++++++++++++++-------------------
 compiler/iface/MkIface.lhs    |    3 +--
 compiler/main/DynFlags.hs     |    3 ++-
 3 files changed, 24 insertions(+), 22 deletions(-)