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

Bug in new Salsa pretty-printed stack traces: wrong stack traces #25

Closed
NHDaly opened this issue Jul 4, 2020 · 2 comments · Fixed by #26
Closed

Bug in new Salsa pretty-printed stack traces: wrong stack traces #25

NHDaly opened this issue Jul 4, 2020 · 2 comments · Fixed by #26
Labels
bug Something isn't working error handling

Comments

@NHDaly
Copy link
Collaborator

NHDaly commented Jul 4, 2020

The salsa pretty stack trace is currently off-by-one, skipping the first function called and duplicating the last one:

julia> using Salsa

julia> @derived f(rt, x) = g(rt, x)
f (generic function with 1 method)

julia> @derived g(rt, x) = h(rt, x)
g (generic function with 1 method)

julia> @derived h(rt, _) = error("OH NO")
h (generic function with 1 method)

julia> f(Runtime(), 1)
ERROR: SalsaWrappedException: Error encountered while executing Salsa derived function:
OH NO

------ Salsa Trace -----------------
[1] @derived f(::Runtime, 1::Any)
[2] @derived g(::Runtime, 1::Any)
[3] @derived h(::Runtime, 1::Any)
------------------------------------

Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] %%__user_h(::Salsa._TracingRuntime{Salsa.EmptyContext,Salsa._DefaultSalsaStorage.DefaultStorage}, ::Int64) at ./REPL[16]:1
 [3] invoke_user_function(::Salsa._TracingRuntime{Salsa.EmptyContext,Salsa._DefaultSalsaStorage.DefaultStorage}, ::Salsa.DerivedKey{typeof(h),Tuple{Any}}, ::Int64) at /Users/daly/julia-dev/Salsa/src/Salsa.jl:612
...

Also, I feel like we're maybe printing the stack backwards? I'm not sure what's best - it kind of makes sense to print top-down since that's like the order of the demand, but usually stack traces are printed the other way. I kind of like though that you can just scroll to the middle and read both the pretty stack trace for the current frame and the julia stack frame of the failure, so that's kind of nice?

@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 4, 2020

CC: @comnik for your opinion on the printing order...

Regarding the best order to print them in, it's not hard to change, we just need to pick. Some reasonable options:

  • Currently (demand-first):
    ------ Salsa Trace -----------------
    [1] @derived f(::Runtime, 1::Any)
    [2] @derived g(::Runtime, 1::Any)
    [3] @derived h(::Runtime, 1::Any)
    ------------------------------------
  • Same order reversed numbering:
    ------ Salsa Trace -----------------
    [3] @derived f(::Runtime, 1::Any)
    [2] @derived g(::Runtime, 1::Any)
    [1] @derived h(::Runtime, 1::Any)
    ------------------------------------
  • Inside out (standard stacktrace order):
    ------ Salsa Trace -----------------
    [1] @derived h(::Runtime, 1::Any)
    [2] @derived g(::Runtime, 1::Any)
    [3] @derived f(::Runtime, 1::Any)
    ------------------------------------

@NHDaly NHDaly closed this as completed in #26 Jul 5, 2020
@comnik
Copy link
Contributor

comnik commented Jul 5, 2020

@rai-nhdaly Let's run with the order we have for a bit and figure out how it works in practice?

@NHDaly NHDaly assigned NHDaly and unassigned NHDaly Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working error handling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants