Skip to content

Comments

Use raise_with_backtrace instead of raise#1

Open
Willenbrink wants to merge 2 commits intocraigfe:mainfrom
Willenbrink:main
Open

Use raise_with_backtrace instead of raise#1
Willenbrink wants to merge 2 commits intocraigfe:mainfrom
Willenbrink:main

Conversation

@Willenbrink
Copy link

Hey,

when an exception is raised within a continuation the backtrace is lost as we catch the exception and simply re-raise it. This PR exchanges Stdlib.raise by Printexc.raise_with_backtrace so this information is not lost.
I'm not sure exactly how large the performance impact is but it should be insignificant. In addition, I assume it is even cheaper if backtraces are currently not recorded or the exception was raised with Stdlib.raise_notrace.

@favonia
Copy link

favonia commented May 2, 2022

It seems the backtrace is not lost during the re-raising, at least in newer OCaml. I wonder if you have a concrete example showing the loss? (I made this mistake before...)

@Willenbrink
Copy link
Author

I think this is unrelated as I can reproduce the problem on your branch. The problem I fixed is shown by this program (found in test/passing/main.ml on this branch):

open Stdlib.EffectHandlers.Deep

exception%effect E: unit

let () =
  Printexc.record_backtrace true;
  let raiser () = raise Not_found in
  match raiser () with
  | e -> e
  | [%effect? E, k] ->
    continue k ()

Without my patch this prints:

Fatal error: exception Not_found    
Raised at Ppx_effects_runtime.raise in file "src/ppx_effects_runtime.ml", line 4, characters 12-24
Called from Dune__exe__Main in file "test/passing/main.ml", line 8, characters 2-74

With the patch it results in:

Fatal error: exception Not_found    
Raised at Dune__exe__Main.raiser in file "test/passing/main.ml", line 7, characters 18-33
Re-raised at Ppx_effects_runtime.raise in file "src/ppx_effects_runtime.ml", line 6, characters 2-36
Called from Dune__exe__Main in file "test/passing/main.ml", line 8, characters 2-74

As you can see the original raise is not present in the first backtrace but is in the second one. I did notice that my patch only fixes this for match with but not for try with. Looking at the code I can not find an obvious fix for this although it is somehow related to this block as we never call exnc with the correct raise there.

Where did it come from? Is that a change in 5.0.0 beta1 -> 5.0.0?
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.

2 participants