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

Backtraces truncated with 4.02 #6556

Closed
vicuna opened this issue Sep 11, 2014 · 19 comments
Closed

Backtraces truncated with 4.02 #6556

vicuna opened this issue Sep 11, 2014 · 19 comments
Assignees

Comments

@vicuna
Copy link

vicuna commented Sep 11, 2014

Original bug ID: 6556
Reporter: @diml
Assigned to: @gasche
Status: resolved (set by @diml on 2018-05-28T14:03:29Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.0
Category: standard library
Monitored by: @Drup @gasche @ygrek @hcarty @yakobowski

Bug description

In the following code, uncommenting the [let e = e in] produces a smaller backtrace:

let f2 () = raise Exit
let f1 f2 = f2 ()
let g f1 f2 =
try f1 f2
with e ->
(* let e = e in *)
raise e

let () = try g f1 f2 with _e -> Printf.eprintf "%s\n%!" (Printexc.get_backtrace ())

With [let e = e in] commented:

Raised at file "toto.ml", line 1, characters 18-22
Called from file "toto.ml", line 4, characters 6-11
Re-raised at file "toto.ml", line 7, characters 10-11
Called from file "toto.ml", line 9, characters 13-20

with [let e = e in] uncommented:

Raised at file "toto.ml", line 7, characters 10-11
Called from file "toto.ml", line 9, characters 13-20

Looking at the -dlambda it's easy to understand why: without the [let e = e in] the raise is turned into a reraise.

I remember there was a discussion about this, so it might have been said that this behavior was acceptable but I have a feeling it's going to make a lot of people unhappy.

@vicuna
Copy link
Author

vicuna commented Sep 11, 2014

Comment author: @mshinwell

As a further example, Core_kernel has a "result" type, with a [try_with] function thus:

let try_with f =
try Ok (f ())
with exn -> Error exn

With 4.02 any caller of [Result.try_with] that wishes to propagate the exception by raising it, preserving the backtrace of [exn] from [f] through to [try_with], has to remember to use %reraise.

@vicuna
Copy link
Author

vicuna commented Sep 11, 2014

Comment author: @mshinwell

(This might be an argument for putting backtraces in exception values themselves, which would probably also solve the lack of precision in retrieval of backtraces mentioned in mantis #6554.)

@vicuna
Copy link
Author

vicuna commented Sep 11, 2014

Comment author: @mshinwell

Another case that is apparently not considered a re-raise:

match expr with
| v -> v
| exception exn -> raise exn

@vicuna
Copy link
Author

vicuna commented Dec 3, 2014

Comment author: @lpw25

Something that I've wanted in the past was to extend the notion of a reraise to include raising a different exception. For example,

try
  foo ()
with Not_found -> raise (Error "Not found")

could result in a backtrace like

Raised at file "foo.ml", line 1, characters 18-22
Called from file "foo.ml", line 4, characters 6-11
Re-raised as Error _ at file "foo.ml", line 7, characters 10-11
Called from file "foo.ml", line 9, characters 13-20

This would then allow us to treat any raise that is syntactically within a with clause as a reraise.

This would fix the reported example, as well as make it easier to debug programs like the compiler, which wrap their exceptions multiple times as they escape.

@vicuna
Copy link
Author

vicuna commented Dec 3, 2014

Comment author: @alainfrisch

A first step would already be to export an explicit "reraise" function in Pervasives. The current heuristic used by the compiler to turn "raise" into "reraise" is mostly implemented to preserve backward-compatibility, for simple cases, but it seems fair to me to ask users to be explicit about a raise being actually a "reraise" (meaning that the stacktrace should be preserved, if possible).

@vicuna
Copy link
Author

vicuna commented Dec 3, 2014

Comment author: @lpw25

it seems fair to me to ask users to be explicit about a raise being actually a "reraise" (meaning that the stacktrace should be preserved, if possible).

Either way, I would like to be able to reraise a different exception and have it printed correctly in the backtrace.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @alainfrisch

Either way, I would like to be able to reraise a different exception and have it printed correctly in the backtrace.

Currently, "reraise" is considered as a simple "raise" if the exception is not physically equal to the previous one (see caml_stash_backtrace in byterun/backtrace.c and asmrun/backtrace.c). It would be easy to drop this test, with an increased risk of creating wrong traces (i.e. append to a previous, unrelated trace).

This would not be enough to satisfy your request, because you also want to record all exception names in the trace. It should not be overly difficult, when the new exception isn't equal to the previous one, to record the name in the trace (in stash_backtrace). This would change the internal representation of traces, but luckily, the concrete representation is noy exposed in Printexc.

In your example:

 try
      foo ()
    with Not_found -> raise (Error "Not found")

--->

  Raised at file "foo.ml", line 1, characters 18-22
  Called from file "foo.ml", line 4, characters 6-11
  Re-raised as Error _ at file "foo.ml", line 7, characters 10-11
  Called from file "foo.ml", line 9, characters 13-20

wouldn't it be more relevant to keep the old exception (Not_found) in the trace?

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @gasche

wouldn't it be more relevant to keep the old exception (Not_found) in the trace?

There are many reasons for wrapping a failure into another exception which carries more information (some have been discussed in the coq-related PR; another concrete use-case I have is a parser that catches exceptions coming from semantic actions and rewrap them in a richer exception containing the location of the production).

I think the suggestions to

  • allow to explicitly require a reraise
  • in that case, append to the backtrace even when the exception differs

are both good ideas.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @alainfrisch

I was not arguing against the feature, but in Leo's example, the displayed trace shows "Error", which is the name of the uncaught exception, so the information is redundant. What is lost in the reraise is the name of the original exception (Not_found); my point is that it seems more useful to keep that name in the trace.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @gasche

Sorry, I misunderstood indeed. We could display the name of the exception being raised on all "R(er-)?aised" lines -- preferably at the end to avoid disturbing tooling that parses raise locations, or even on its own line before.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @lpw25

wouldn't it be more relevant to keep the old exception (Not_found) in the trace?

Oh yes, I see what you mean. I guess I was thinking that the original exception would be displayed in the "Uncaught exception" message, but obviously that's incorrect.

How about something like:

Uncaught exception Error _

Raised as Not_found at ...
Called from ....
Rerasied as Error _ at ...
Called from ...

with the "as Not_found" only appearing if there has been a changing reraise.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @lpw25

Related to some of the discussion of exceptions in Coq, I wonder whether it would also be a good idea to let users insert additional information into backtraces.

Even if the information was just strings it might improve debugging in the presence of complicated control flow (e.g. Lwt and Async). For example

try
spawn_thread ()
with exn ->
Printexc.attachf "Spawned from thread %s" thread_name;
reraise exn

to give:

Raised as Not_found at ...
Called from ....
Spawned from thread foo
Called from ...

Perhaps it is not worth the effort, but it might be useful.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @alainfrisch

be a good idea to let users insert additional information into backtraces.

Would that subsume your proposal of keeping names of previous exceptions? This would require users to manually insert the old exception name as one of these extra information, but at least users that don't need this feature (but want to re-raise a different exception and keep the same trace) wouldn't have to pay the extra tiny cost of it.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @lpw25

Would that subsume your proposal of keeping names of previous exceptions?

I meant in addition to it. Reraising a different exception is the most common case, so it would be good to support it as easily as possible. I just thought that in more complex cases the additional power of arbitrary strings might be useful.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @lpw25

but at least users that don't need this feature (but want to re-raise a different exception and keep the same trace) wouldn't have to pay the extra tiny cost of it.

Since we already statically detect when syntactically the same exact exception value is used, this cost could easily be optimised away by the code for compiling the "%reraise" primitive.

@vicuna
Copy link
Author

vicuna commented Dec 4, 2014

Comment author: @diml

Even if the information was just strings it might improve debugging in the presence of complicated control flow (e.g. Lwt and Async). For example

I don't think that would help much for Lwt or Async. The main problem with backtraces and Lwt/Async is that the backtrace is global and will always be destroyed by another thread.

@vicuna
Copy link
Author

vicuna commented Jun 28, 2017

Comment author: @lpw25

Another case that is apparently not considered a re-raise:

match expr with
| v -> v
| exception exn -> raise exn

Note that this was fixed some time before 4.04.0

@vicuna
Copy link
Author

vicuna commented May 28, 2018

Comment author: @diml

I think this is fixed in 4.07 (since #1567).

@vicuna vicuna closed this as completed May 28, 2018
@vicuna
Copy link
Author

vicuna commented May 29, 2018

Comment author: @trefis

I don't think so, #1567 seems mostly unrelated to what is discussed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants