Use IP based CallStack in error and undefined

Authored by gridaphobe.

Description

Use IP based CallStack in error and undefined

This patch modifies error, undefined, and assertError to use
implicit call-stacks to provide better error messages to users.

There are a few knock-on effects:

  • GHC.Classes.IP is now wired-in so it can be used in the wired-in types for error and undefined.
  • TysPrim.tyVarList has been replaced with a new function TysPrim.mkTemplateTyVars. tyVarList made it easy to introduce subtle bugs when you need tyvars of different kinds. The naive

    ` tv1 = head $ tyVarList kind1 tv2 = head $ tyVarList kind2 `

    would result in tv1 and tv2 sharing a Unique, thus substitutions would be applied incorrectly, treating tv1 and tv2 as the same tyvar. mkTemplateTyVars avoids this pitfall by taking a list of kinds and producing a single tyvar of each kind.
  • The types GHC.SrcLoc.SrcLoc and GHC.Stack.CallStack now live in ghc-prim.
  • The type GHC.Exception.ErrorCall has a new constructor ErrorCallWithLocation that takes two Strings instead of one, the 2nd one being arbitrary metadata about the error (but usually the call-stack). A bi-directional pattern synonym ErrorCall continues to provide the old API.

Updates Cabal, array, and haddock submodules.

Reviewers: nh2, goldfire, simonpj, hvr, rwbarton, austin, bgamari

Reviewed By: simonpj

Subscribers: rwbarton, rodlogic, goldfire, maoe, simonmar, carter,
liyang, bgamari, thomie

Differential Revision: https://phabricator.haskell.org/D861

GHC Trac Issues: Trac #5273

Details

Group Auditors
Restricted Owners Package
Restricted Owners Package
Committed
bgamariSep 2 2015, 6:21 AM
Reviewer
simonpj
Differential Revision
D861: Use IP based CallStack in error and undefined
Parents
rGHCad26c54b86a8: Testsuite: refactoring only
Branches
Unknown
Tags
Unknown
Build Status
Buildable 5459
Build 5855: GHC Continuous Integration (amd64/Linux)
nomeata raised a concern with this commit.Sep 4 2015, 9:34 AM
nomeata added a subscriber: nomeata.

This commit triggered performance warnings, as you can see at https://perf.haskell.org/ghc/#revision/6740d70d95cb81cea3859ff847afc61ec439db4f

In particular, it increases code size noticeably (roughly +30kB). Is that expected and acceptable?

A blind guess: Given a small function like foldl1, which is inlineable and calls error, maybe the actual CallStack value will also be inlined with it? I should build the latest HEAD and have a look at the unfoldings in the interface... but it looks like I have to blow away my build again, so probably not now.

A blind guess: Given a small function like foldl1, which is inlineable and calls error, maybe the actual CallStack value will also be inlined with it? I should build the latest HEAD and have a look at the unfoldings in the interface... but it looks like I have to blow away my build again, so probably not now.

This does not seem to be the case...

We were discussing this a bit yesterday in #ghc.

A small increase would be expected (many of the benchmarks include a call to error via an imported function), but 30k seems too large. It's possible that a bundled change in Cabal (which defers stripping libraries until the install-phase) is related, but it's not clear.

Please let me know if/how I can help!

@nomeata I spent some more time looking into this today and have learned the following.

  • The increase in binary sizes (at least for the atom benchmark) is coming from a handful of modules in ghc-prim and base
  • GHC/Types.o has increased by about 5K. It's now the home of the SrcLoc and CallStack types, which together define 7-8 record selectors, so I think this may not be too surprising.
  • Control/Applicative.o, Data/Either.o, Data/Foldable.o, and a few others in base have increased in by 2-5K each. Every one of these modules contains at least one CallStack in the Core.
  • The other modules I've looked at that have not increased in size do not have a CallStack, so it seems pretty clear that the increase *is* due to the CallStack.
  • Finally, I think you're correct that the CallStacks are being inlined sometimes, because I found a CallStack referencing GHC.Base in Data.Foldable.

So perhaps all we need to do is tell GHC to not inline CallStack values? Do you happen to know how I can do that? :)

nomeata accepted this commit.Sep 5 2015, 1:01 AM

Nice analysis, thanks. From what I understand, you did not find anything obviously and blatantly wrong, only minor possible ways for tweaks.

I’m not quite sure what the best way is to prevent the sharing. Maybe the type checker, which produces the CallStack values, can be told to add the (core equivalent) of a NOINLINE pragma to them?

BTW, is there a good reason why CallStack is a data and not a newtype?

Hmm, I changed my mind: This needs more finetuning. I opened Trac #10844 for this.

BTW, is there a good reason why CallStack is a data and not a newtype?

There is not a particularly good reason, no. It would probably make sense to switch it to a newtype, I don't think there would be any compatibility concerns should we want to add extra fields to the CallStack in the future.