Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert XException to ErrorCall #1461

Merged
merged 1 commit into from
Jul 29, 2020
Merged

Convert XException to ErrorCall #1461

merged 1 commit into from
Jul 29, 2020

Conversation

christiaanb
Copy link
Member

To make it easier to report XException when pack would normally hide them.

@leonschoorl
Copy link
Member

We could also use HasCallStack and friends to get the location of the xToErrorCtx
In addition to or instead of the string

*Main> h  undefined 3
*** Exception: a is X
CallStack (from HasCallStack):
  xToErrorCtx, called at xerror.hs:6:4 in main:Main
X: undefined
CallStack (from HasCallStack):
  errorX, called at src/Clash/XException.hs:876:13 in clash-prelude-1.3.0-inplace:Clash.XException
  undefined, called at <interactive>:1:4 in interactive:Ghci2
diff --git a/clash-prelude/src/Clash/XException.hs b/clash-prelude/src/Clash/XException.hs
index 7c702e3f..b5566331 100644
--- a/clash-prelude/src/Clash/XException.hs
+++ b/clash-prelude/src/Clash/XException.hs
@@ -125,9 +125,9 @@ errorX msg = throw (XException ("X: " ++ msg ++ "\n" ++ prettyCallStack callStac
 -- > f (xToErrorCtx "a is X" -> !a) (xToErrorCtx "b is X" -> !b) = ...
 --
 -- __NB:__ Fully synthesisable, so doesn't have to be removed before synthesis
-xToErrorCtx :: String -> a -> a
+xToErrorCtx :: HasCallStack => String -> a -> a
 xToErrorCtx ctx a = unsafeDupablePerformIO
-  (catch (evaluate a >> return a) (\(XException msg) -> throw (ErrorCall (ctx <> "\n" <> msg))))
+  (catch (evaluate a >> return a) (\(XException msg) -> throw (ErrorCall (ctx <> "\n" <> prettyCallStack callStack <> "\n" <> msg))))
 {-# NOINLINE xToErrorCtx #-}

@christiaanb
Copy link
Member Author

I doesn't have to be added to the primitive though

@leonschoorl
Copy link
Member

Right, we can keep the primitive as it is and create a wrapper that passed the rendered callstack as a string

@leonschoorl
Copy link
Member

The callstack can also be used to trace further back before h:

f, g, h :: HasCallStack => Bool -> BitVector 8 -> BitVector 8
f = g
g = h
h (xToErrorCtx "a is X" -> a) (xToErrorCtx "b is X" -> b) = slice d7 d0 (pack a ++# b)
*Main> f undefined 3
*** Exception: a is X
CallStack (from HasCallStack):
  xToErrorCtx, called at xerror.hs:8:4 in main:Main
  h, called at xerror.hs:7:5 in main:Main
  g, called at xerror.hs:6:5 in main:Main
  f, called at <interactive>:12:1 in interactive:Ghci1
X: undefined
CallStack (from HasCallStack):
  errorX, called at src/Clash/XException.hs:876:13 in clash-prelude-1.3.0-inplace:Clash.XException
  undefined, called at <interactive>:12:3 in interactive:Ghci1

Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, have you tested it? Could be good to add a couple of tests testing the synthesizability / whether it actually displays usable error messages.

-- > {-# LANGUAGE ViewPatterns, BangPatterns #-}
-- > f (xToErrorCtx "a is X" -> !a) (xToErrorCtx "b is X" -> !b) = ...
--
-- __NB:__ Fully synthesisable, so doesn't have to be removed before synthesis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

synthesizable 🇺🇸

@christiaanb
Copy link
Member Author

@leonschoorl @martijnbastiaan could you review again to see whether it looks good now?

@@ -364,6 +364,9 @@ coreToTerm primMap unlocs = term
go "Clash.Magic.noDeDup" args
| [_aTy,f] <- args
= C.Tick C.NoDeDup <$> term f
go "Clash.XException.xToErrorCtx" args -- xToError :: forall a. String -> a -> a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the callstack argument to the comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the correct primitive name

clash-prelude/src/Clash/XException.hs Show resolved Hide resolved
@christiaanb
Copy link
Member Author

@leonschoorl I think I've addressed all your points now

-- errorX, called at ...
-- <BLANKLINE>
xToError :: HasCallStack => a -> a
xToError = xToErrorCtx (prettyCallStack (popCallStack callStack))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you popCallStack?
Doesn't that remove the removes the call that will tell me whether it was the a or the b argument to h that contained the XException?

clash-ghc/src-ghc/Clash/GHC/GHC2Core.hs Show resolved Hide resolved
To make it easier to report XException when `pack` would normally
hide them.
@christiaanb christiaanb merged commit 8ee9152 into master Jul 29, 2020
@christiaanb christiaanb deleted the x_to_error branch July 29, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants