Sebastian Graf authored 5 years ago and Marge Bot committed 5 years ago

D3673 experienced reduce/reduce conflicts when trying to use
opt_instance for associated data families.

That was probably because the author tried to use it for
Haskell98-syntax without also applying it to GADT-syntax, which actually
leads to a reduce/reduce conflict. Consider the following state:

```
data . T = T
data . T where T :: T
```

The parser must decide at this point whether or not to reduce an empty
`opt_instance`. But doing so would also commit to either
Haskell98 or GADT syntax! Good thing we also accept an optional
"instance" for GADT syntax, so the `opt_instance` is there in both
productions and there's no reduce/reduce conflict anymore.

Also no need to inline `opt_instance`, how it used to be.

Takenobu Tani authored 5 years ago and Marge Bot committed 5 years ago

This commit adds -haddock option to Hadrian-based build system.

To enable :doc command on GHCi, core libraries must be compiled
with -haddock option.
Especially, the `-haddock` option is essential for a release build.

Assuming current GitLab CI condition (.gitlab-ci.yml),
I add -haddock option to the default flavour only.

This has already been done for Make-based build system.
Please see #16415.

Kari Pahula authored 5 years ago and Marge Bot committed 5 years ago

Add a new optional failure handling for upsweep which continues
the compilation on other modules if any of them has errors.

Artem Pyanykh authored 5 years ago and Marge Bot committed 5 years ago

Currently, if you change these ^ flavour parameters, rebuilding is not
triggered, since `programContext` doesn't set up a dependency on
those values.

Exposing these values via an oracle does set the dependency and
properly triggers a rebuild of binaries.

Several attempts to factor out these actions ended up in cyclic
dependency here or there. I'm not absolutely happy with this variant
either, but at least it works.

====

Issue repro:

In UserSettings.hs:
```
dbgDynamic = defaultFlavour { name = "dbg-dynamic"
                            , dynamicGhcPrograms = pure True,
                            ... }

dbgStatic = defaultFlavour { name = "dbg-static"
                           , dynamicGhcPrograms = pure False
                           ... }
```

Then in console:

```
$ hadrian/build.sh -j --flavour=dbg-dynamic
... does the build
$ hadrian/build.sh -j --flavour=dbg-static
... does nothing, considers binaries up to date
```

adam authored 5 years ago and Marge Bot committed 5 years ago

Metric Increase:
    haddock.base
    T4029

Takenobu Tani authored 5 years ago and Marge Bot committed 5 years ago

This commit only fixes links and markdown syntax.

[skip ci]

[ci skip]

The 'cp' field really is only used when type==posTypeFresh so it's more
space efficient to have it in the nextPos union.

This finally moves the newly generalised heap traversal code from the
retainer profiler into it's own file.

A lot of these includes are presumably leftovers from when the retainer
profiler still did it's own heap profiling.

Turns out some genius disabled warnings for RetainerProfile.c in the build
system. That would have been good to know about five silent type mismatch
crashes ago.. :)

Currently it is necessary for user code to expend at least one extra bit in
the closure header just to know whether visit() should return true or
false, to indicate if children should be traversed.

The generic traversal code already has this information in the visited bit
so simply pass it to the visit callback.

There's simply no need anymore for this whole business. Instead of
individually traversing roots in retainRoot() we just push them all onto
the stack and traverse everything in one go.

This feature was not really used anyways. There is an
`ASSERT(isEmptyWorkStack(ts))` at the top of retainRoot() which means there
really can't ever have been any chunks at the toplevel.

The only place where this was probably used is in traversePushStack but
only way back when we were still using explicit recursion on the
C callstack.

Since the code was changed to use an explicit traversal-stack these
stack-chunks can never escape one call to traversePushStack anymore.  See
commit 5f1d949a ("Remove explicit recursion in retainer profiling")

STATIC_INLINE already does what the code wanted here, no need to duplicate
the functionality here.

These invariants don't seem to make any sense in the current code.  The
text talks about c_child_r as if it were an StgClosure, for which RSET()
would make sense, but it's a retainer aka 'CostCentreStack*'.

A lot of comments and strings are still talking about old names, fix
that.

Keeping track of the maximum stack seems like a good idea in all
configurations. The associated ASSERTs only materialize in debug mode but
having the statistic is nice.

To make the debug code less prone to bitrotting I introduce a function
'debug()' which doesn't actually print by default and is #define'd away
only when the standard DEBUG define is off.

This gets all remaining functions in-line with the new 'traverse' prefix
and module name.

Commit dbef766c ("Profiling cleanup.") made this debug code obsolete by
removing the 'cost' function without a replacement. As best I can tell the
retainer profiler used to do some heap census too and this debug code was
mainly concerned with that.

In the old code when DEBUG_RETAINER was set, FIRST_APPROACH is
implied. However ProfHeap.c now depends on printRetainerSetShort which is
only available with SECOND_APPROACH. This is because with FIRST_APPROACH
retainerProfile() will free all retainer sets before returning so by the
time ProfHeap calls dumpCensus the retainer set pointers are segfaulty.

Since all of this debugging code obviously hasn't been compiled in ages
anyways I'm taking the liberty of just removing it.

Remember guys: Dead code is a liability not an asset :)

Instead of breaking out of the switch-in-while construct using `return` this
uses `goto out` which makes it possible to share a lot of the out-variable
assignment code in all the cases.

I also replaced the nasty `while(true)` business by the real loop
condition: `while(*c == NULL)`. All `break` calls inside the switch aready
have either a check for NULL or an assignment of `c` to NULL so this should
not change any behaviour.

Using `goto out` also allowed me to remove another minor wart: In the
MVAR_*/WEAK cases the popOff() call used to happen before reading the
stackElement. This looked like a use-after-free hazard to me as the stack
is allocated in blocks and depletion of a block could mean it getting freed
and possibly overwritten by zero or garbage, depending on the block
allocator's behaviour.

This essentially turns the heap traversal code into a visitor. You add a
bunch of roots to the work-stack and then the callback you give to
traverseWorkStack() will be called with every reachable closure at least
once.