Make use of GHC call-stack simulation for the bounds-checked partial functions #184
Conversation
`(!)` etc. are of course only safer than their `unsafe` pendants in the sense that errors are raised as proper exceptions, but when used in sizable applications that by itself does not make the errors easy to find. GHC-8 has added support for simulated call stack which can actually give some information on the use site of such errors. In this commit, I added the necessary constraint to the partial functions which perform bounds checks.
|
It would be good to make sure that this doesn’t interact poorly with ghc
fusion or optimization’s.
Likewise : with cabal new build profiled builds or debug builds are cached
in a pretty nice way.
That said : this could Be great: let’s make sure that it doesn’t have any
cost :)
…On Tue, Sep 26, 2017 at 7:44 PM Justus Sagemüller ***@***.***> wrote:
Until now, Vector's (!), head etc., like other partial functions in
Haskell, simply crashed the program without giving useful information as to
where the problem originated – unless compiling with -fprof, which gives
stack traces for everything but is horrible for performance.
GHC-8 has added the HasCallStack
<https://hackage.haskell.org/package/base-4.9.1.0/docs/GHC-Stack.html#t:HasCallStack>
constraint, which allows such functions to give meaningful error diagnosis
without requiring global profiling. I enabled this on this library's
checked-partial functions.
------------------------------
You can view, comment on, or merge this pull request online at:
#184
Commit Summary
- Enable GHC call-stack simulation in "safe" partial functions.
- Also support call-stack simulation in concrete boxed/unboxed
interfaces.
File Changes
- *M* Data/Vector.hs
<https://github.com/haskell/vector/pull/184/files#diff-0> (22)
- *M* Data/Vector/Fusion/Bundle/Monadic.hs
<https://github.com/haskell/vector/pull/184/files#diff-1> (9)
- *M* Data/Vector/Generic.hs
<https://github.com/haskell/vector/pull/184/files#diff-2> (24)
- *M* Data/Vector/Generic/Mutable.hs
<https://github.com/haskell/vector/pull/184/files#diff-3> (4)
- *M* Data/Vector/Internal/Check.hs
<https://github.com/haskell/vector/pull/184/files#diff-4> (17)
- *M* Data/Vector/Unboxed.hs
<https://github.com/haskell/vector/pull/184/files#diff-5> (24)
- *A* include/stacktracetools.h
<https://github.com/haskell/vector/pull/184/files#diff-6> (6)
- *M* include/vector.h
<https://github.com/haskell/vector/pull/184/files#diff-7> (14)
- *M* vector.cabal
<https://github.com/haskell/vector/pull/184/files#diff-8> (1)
Patch Links:
- https://github.com/haskell/vector/pull/184.patch
- https://github.com/haskell/vector/pull/184.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#184>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAQwucpipr1Gp2duS7_pl6uEvzTOto_ks5smYxagaJpZM4PlBu3>
.
|
| import GHC.Stack (HasCallStack) | ||
| #define CHECK(f) (withFrozenCallStack Ck.f __FILE__ __LINE__) | ||
| #else | ||
| #define HasCallStack (Eq ()) |
There was a problem hiding this comment.
That Eq () fallback constraint was just a rough hack, to replace HasCallStack with something as simple as possible on GHC versions that don't support it yet. It turns out even that super-simple hack doesn't work just like that, it would require tossing in FlexibleContexts:
Data/Vector/Internal/Check.hs:122:15:
Non type-variable argument in the constraint: Eq ()
(Use FlexibleContexts to permit this)
In the type signature for ‘checkIndex’:
checkIndex :: (Eq ()) =>
String -> Int -> Checks -> String -> Int -> Int -> a -> a
Any ideas how this could be properly done, short of cluttering every signature with an #if MIN_VERSION_base?
There was a problem hiding this comment.
I would do what the call-stack library does:
#if MIN_VERSION_base(4,9,0)
import GHC.Stack (HasCallStack)
#elif MIN_VERSION_base(4,8,1)
type HasCallStack = (?callStack :: GHC.CallStack)
#else
import GHC.Exts (Constraint)
type HasCallStack = (() :: Constraint)
#endifThis requires base-4.5 (GHC 7.4) or later, but those are precisely the version bounds that vector has. Moreover, this shouldn't require the use of FlexibleContexts.
There was a problem hiding this comment.
I don't suppose it would be ok for vector to actually depend on call-stack?
|
Sure, I agree. My idea was to only add |
|
I suspect that as far as performance is concerned the callstacks generally shouldn't be a problem so long as the function is inlined. In this case I would expect that the callstack allocation will be floated in to the failure branch if one is necessary at all. I would look at the simplified core (i.e. |
|
FWIW, I rebased @leftaroundabout's patches on top of the current master here: https://github.com/feuerbach/vector/tree/feature/ghc-callstack Let me know if there's anything else I can do to help this get merged. |
|
Ooo. Nice! I’ll take a look soon. Everything is a tad slow what with the
plague, but this piques my interest.
…On Wed, Apr 15, 2020 at 2:23 PM Roman Cheplyaka ***@***.***> wrote:
FWIW, I rebased @leftaroundabout <https://github.com/leftaroundabout>'s
patches on top of the current master here:
https://github.com/feuerbach/vector/tree/feature/ghc-callstack
Let me know if there's anything else I can do to help this get merged.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#184 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABBQSF6SYOUKMIN5S5RGDRMX3RZANCNFSM4D4UDO3Q>
.
|
|
Thanks @cartazio. I've also added some missing HasCallStack annotations on my branch. Annotations are still missing in the Fusion/* modules (Data/Vector/Fusion/Stream/Monadic.hs, Data/Vector/Fusion/Bundle.hs, Data/Vector/Fusion/Bundle/Monadic.hs); I'm not sure whether they should be there or not. |
|
@feuerbach The idea looks good to me, but I do not like |
|
On other hand repeating same thing is asking for problems when someone will forget to edit one of files or make a mistake. Still it doesn't solve duplication completely. There's |
|
My opinion on this PR is that the direction of relying on HasCallStack is the right way to go, however adding more CPP is not the right approach to get to the goal. I've just submitted a PR that reduces CPP usage in favor of HasCallStack and uses My sincere apologies for this PR to be dragged around for 4 years. I do think that this PR was an improvement, and should have gotten more attention. |
|
Closing since #397 is merged |
Until now, Vector's
(!),headetc., like other partial functions in Haskell, simply crashed the program without giving useful information as to where the problem originated – unless compiling with-fprof, which gives stack traces for everything but is horrible for performance.GHC-8 has added the
HasCallStackconstraint, which allows such functions to give meaningful error diagnosis without requiring global profiling. I enabled this on this library's checked-partial functions.