Hadrian: support shake's --lint-fsatrace feature and fix lint errors

Shake's --lint-fsatrace feature uses fsatrace to track file accesses of external commands. This allows for more accurate dependency linting which is important e.g. for caching.

Hadrian already accepts the --lint-fsatrace command line option (it is passed on to shake automatically), but it currently has no effect. The shake options are not set correctly (In particular shakeLintInside must be set).

Required Changes:

1. shakeLintInside must be set
2. Out of tree build dirs don't get linted.

buildRoot -/- "lint-test" %> \out -> 
    let gen t f = cmd Shell "echo" t ">" (toNative f) :: Action ()
    gen "x" $ out <.> "txt"
    need [out <.> "txt"]     -- This need should set off a lint error.
    writeFile' out ""

• fsatrace gives absolute paths. Shake then tries and fails to convert to relative paths here. Stuck with an unexpected absolute path, shake thinks the file isn't needed even thought the relative path **is** needed.

How To Collect Stats

To collect fsatrace lint errors as reported below, you need to modify Shake to warn instead of error on fsatrace lint errors. Have a look at this commit of shake (from this branch) for how to do that. Then you'll want to do a clean build with linting enabled and save the output to a file e.g.:

$ ./boot && ./configure
$ (./hadrian/build.sh -j --lint-fsatrace 2>&1) > build.out

Then you can use breakdown.hs to extract and compare lint error counts between builds:

$ runhaskell ~/breakdown.hs build_01.out build_02.out

You can also pass the same file twice to just get the counts of one run.

Status

This uses a stage0 compiler with support for -include-cpp-deps (see comment below)

Remaining lint errors by file extension

Extension	Missing Need - Error Count
.h	84956
.hs-boot	108
.so	15
.hs	3
.gz	1
.hs-incl	1

Extension	Late Need - Error Count
.h	4012
.so	8
.hs-incl	1

I've tried to break these errors down into some specific cases as follows. Each point roughly corresponds to a single issue (though this is just an educated guess). Have a look at this build output and grep for "@@@" to find relevant errors.

• (~ 83.5k errors) #16868 /include//*.h dependencies used in later stage of ghc copiler (see comment).
• (~ 4k errors) _build/generated/*.h used/late need for many .o file rules.
• (108 errors) #16815 .hs-boot used but not depended upon in rule for dependencies.mk. This is from dependency generation using ghc -M.
  • other .hs/.h files used but not depended upon in rule for dependencies.mk.
    • (2 errors) includes/CodeGen.Platform.hs used but not depended upon in rule for dependencies.mk
    • (1 errors) libraries/filepath/System/FilePath/Internal.hs used but not depended upon in rule for _build/stage1/libraries/filepath/.dependencies.mk
• (~ 72 errors) #16983 (closed) in rule for _build/generated/platformConstants. I think this file is a bit of a special case.
• (12 errors) libffi.so needed after being used when linking various ways of rts .so.
• (9 errors) libffi.so used but not depended upon in rule for linking some binaries.
• (2 errors) _build/stage0/compiler/build/primop-commutable.hs-incl late need in rule for _build/stage0/compiler/build/PrimOp.o
• (1 error) libffi-tarballs/libffi-3.99999+git20171002+77e130c.tar.gz used but not depended upon in rule for ../stage1/libffi/build/Makefile.in ../stage1/libffi/build/configure

Things already finished:

• (~ 1.8k errors) CPP dependencies !845 (merged)

changed weight to 5

added Tbug Trac import hadrian labels

assigned to @trac-davide

changed the description

changed the description

I've added a shake PR https://github.com/ndmitchell/shake/pull/656 (this related to point 2 above).

mentioned in issue #16399 (closed)

marked this issue as related to #16399 (closed)

added well-typed priorities label

What is the state of this, @davide?

We went back and forth on the patch to Shake a few times, and I've now merged it. I've also followed up with additional lint improvements. I intend to release these as a new Shake version tomorrow. After that, it should be possible to trace out of tree builds too.

The lack of compatibility with --shared isn't something I'm aware of, and at first glance, I don't see how they interact. An example using Shake HEAD would be great.

Great, thank you, @ndmitchell! I'm very much looking forward to playing around with --shared myself.

assigned to @DavidEichmann

@ndmitchell (and @bgamari), the ultimate goal is more accurate dependencies in Hadrian. This is needed so that caching and partial rebuilds work correctly.

At the moment many of our rules looks something like:

patterns &%> \ outFiles -> do
    need deps
    ...

First, please excuse my naivety wrt build systems. Originally I had the romanticized impression that:

• patterns would match all files that the Action produces.
• Similarly, outFiles exactly equals all files that the Action produces.
• deps would exactly list all dependant files.

I quickly learned this is not so! Instead, this is a more typical usage:

patternsPartial &%> \ outFiles -> do
    need depsPart
    ...
    theRest <- liftIO lookForProducedFiles
    produces theRest

where

1. patternsPartial matches only a subset of the files that the Action produces
2. depsPart lists only a subset of the files used by the Action
3. theRest lists files such that the union of outFiles and theRest should be exactly the set of files that the Action produces. In practice this is prone to error and likely not the case.

Despite these missing dependencies/outputs a clean build with Hadrian succeeds. If it dosen't, a programmer updates the dependencies just enough for the build to succeed again. While this results in a Hadrian that succeeds at a clean build, partial rebuilds and caching don't work reliably.

So... fsatrace linting to the rescue? I've been struggling with trying to fix the new linting errors we get from hadrian. I'm thinking about how these errors should be fixed. I originally assumed it would require a good bit of manual needing and some extra logic to anticipate dependencies. After a fair amount of fiddling, this seems like an unrealistic/unmaintainable workflow. Our external commands (e.g. configure/make) have non-trivial behaviour and are occasionally subject to change, so anticipating dependencies is prone to error.

Instead I suggest we use the AutoDeps functionality which does all the needing for us based on the results of the fsatrace. I think we'll need to expand on this functionality, perhaps to also automate calling of produces. I've also quickly run into an error where a command both produces a rules output file then reads it, causing AutoDeps to need it and resulting in a recursive rule error. So some work needs to be done here. One con here is it would also require fsatrace to run a Hadrian build, so we'd likely have to add an fsatrace submodule to the ghc repo.

If you have any advice or thoughts, @ndmitchell, that would be greatly appreciated.

@DavidEichmann it would be great if patterns listed all the files, and the dependencies were all listed, but it's going to be a lot of work to get there. Some notes:

• The lack of dependencies is a bit crap. We should fix that.
• The fact that patterns doesn't cover everything and we use produces isn't a big deal.
• The make and configure rules are going to be the biggest pain. How hard is it to just wildly over-approximate? Have you seen getDirectoryFiles so we can glob our way to success?

Using AutoDeps only works if the files were already built, which in practice means source files or things guaranteed produced by something earlier. My reservation about AutoDeps is it means GHC requires fsatrace to build, which is a big hammer and a potential problem with bootstrapping.

Could you give concrete examples of problems you are facing? e.g. which specific rule depends on what that seems unexpected and hard to calculate in advance?

I've tried to get a bigger picture of the task ahead. I locally patched shake to warn instead of error on fsatrace linting errors, so that I can collect all linting errors at once. I get the following breakdowns by file extension. There is a total of 448480 lint errors:

Files used without being `need`ed

file extension	error count
.h	85607
.hi	83541
.dyn_hi	69669
.p_hi	63769
	25429
.cache	11785
.hi-boot	1412
.p_hi-boot	723
.dyn_hi-boot	723
.so	387
.hsc	204
.hs-boot	108
.Plo	80
.hs-incl	60
.hs	49
.in	31
.a	21
.o	20
.c	10
.x	9
.lo	9
.ac	7
.m4	6
.awk	5
.cabal	4
.3	4
.status	2
.pc	2
.map	2
.info	2
.guess	2
.S	2
.sub	1
.sh	1
.s	1
.lai	1
.la	1
.host	1
.gz	1
.0	1

Files `need`ed after being used

file extension	error count
.hi	34533
	25404
.dyn_hi	18991
.p_hi	18914
.h	4006
.hi-boot	1412
.p_hi-boot	723
.dyn_hi-boot	723
.hs-incl	60
.hs	12

I suspect most of this is easily fixed. I'll characterize this a bit further and slow get this number down. Then we can focus on the more complicated cases.

@DavidEichmann Interesting list. I guess most of these are various invocations of the Cabal library, e.g. for copying packages...

I accidentally lumped in "need after use" errors. I updated my last comment to split into 2 cases.

I'm currently trying to fix one source of missing needs: when ghc builds some object files, it uses *.hi files located in the package database (e.g. stage0/lib/../lib/x86_64-linux-ghc-8.6.1/transformers-0.5.5.0/Control/Applicative/Backwards.hi). We already have some code to need dependant libraries, but this doesn't include the *.hi files. To derive the file paths my plan in to:

• Find the directory containing the *.hi files.
  • This can be done with a new builder similar to GhcPkg Dependencies that extracts the "import-dirs" filed instead for the "depends" field from ghc-pkg.
• Find the list of modules for the package and convert to relative paths.
  • See related funciton Oracles.ModuleFiles.hsObjects which gets a list of *.o files of a package.

@DavidEichmann I'm actually unsure about the hi files in the package database. As far as I understand, Make doesn't produce these files, so in the Make tree there is only one hi file per module. However, in Hadrian we currently have two hi files: one in the package build directory and one in the package database.

Can we get rid of duplicate hi files altogether?

@snowleopard, good question! In Hadrian we output all package build artifacts into a build directory e.g. stage0/libraries/transformers/build/, then copy everything using cabal copy closer to the package database at e.g. stage0/lib/x86_64-linux-ghc-8.6.1/transformers-0.5.5.0. Then the package is registered with the new copied directory. Make, on the other hand, skips this copy step and simply registers the package using an absolute path to the build directory. It sounds like an appealing simplification to remove the copy step in Hadrian and simply place build artifacts directly into the package database. I'll have a search myself, but is there a good reason Hadrian does this copy step (@alp or @bgamari)? Perhaps there are some extra files in the build dir that we don't want?

If I'm not mistaken, @angerman wrote that part. Moritz, any objection to mimicking Make here?

Reimplementing/short circuiting the copy step in hadrian sounds fine to me, provided that this does not put more files into the target folder than what cabal copy would have done. That is, I doubt we want to end up with an install that looks different to what cabal would have produced and subsequently end up with hard to track down bugs where you depend on files in the install location, that cabal would never have put there.

marked this issue as related to #16621

mentioned in issue #16304

changed title from Hadrian: support shake's --lint-fsatrace feature to Hadrian: support shake's --lint-fsatrace feature and fix lint errors

marked this issue as related to #16653 (closed)

Related MR: !317 (merged)

changed the description

@DavidEichmann, Thanks for the summary! A few things:

• It seems to me like hs-boot files being omittted from -M output is a bug. We should open an independent ticket for this.
• The other thing piece of the story that is still missing is what the potential pay-off for caching is. I recall you did some simple measurements but I have forgotten the outcome. Let's write this down.
• I assume that this log was built using a boot compile without !845 (merged)?

This build log and lint error stats are from a built with !845 (merged) and a Stage0 compiler that supports -include-cpp-deps (though I had to disabled the version gate in hadrian to ensure -include-cpp-deps was used). I'll updated the description. I'll admit the stats are a bit out of date. I'll run it again on the latest master and report the results here.

Ok the latest run is as follows:

Using stage0 ghc from my branch bc540753 (I had to use alp's nix shell to build this) and a recent master (again a branch from my repo) with some small modifications to shake 966be087d.

Remaining lint errors by file extension

Extension	Missing Need - Error Count
.h	84956
.hs-boot	108
.so	15
.hs	3
.gz	1
.hs-incl	1

Extension	Late Need - Error Count
.h	4012
.so	8
.hs-incl	1

changed the description

marked this issue as related to #16815

changed the description

mentioned in issue #16852

It may be an easier place to continue with the quick flavour. With some luck this will avoid many lint errors, and narrow our current focus.

mentioned in issue #16868

marked this issue as related to #16868

changed the description

mentioned in issue #16865 (closed)

added Pnormal label

marked this issue as related to #16926

mentioned in issue #16926

changed the description

mentioned in issue #16983 (closed)

changed the description

marked the checklist item (~ 72 errors) #16983 (closed) in rule for _build/generated/platformConstants. I think this file is a bit of a special case. as completed