More precise and efficient handling of backtraces by bobot · Pull Request #293 · ocaml/ocaml

bobot · 2015-11-16T00:47:21Z

This merge-request doesn't propose yet any codes for integration. The goal is to understand if the proposed directions are worth investigating.

Keeping correct backtraces

The merge-request in #246 shows that there are cases were the backtraces used for reraise or just Printexc.print_backtrace have been lost by intermediary raise:

between the start of the exception handler and the reraise,
before the exception handler by the use of an exception in a when guard.

The tests for these cases are added in the first commit with the buggy backtraces as reference.

Thanks to the Printexc.raw_backtrace type, it is possible to fix these backtraces, in a very inefficient way , by using Printexc.get_raw_backtrace and adding a function caml_reraise_raw_backtrace that make the given backtrace and exception the current ones (it sets caml_backtrace_buffer and caml_backtrace_last_exn):

save the backtrace at the beginning of the exception handler and use it for reraise,
save the backtrace at the beginning of the computation of when guards and restore it at the end.

The third, fourth and sixth commits implement these inefficient fix.
The references of the tests are fixed

Do we agree that there is no other case where the backtrace is wrong, and that the backtraces returned with these fixes are the correct one?

Keeping the backtrace creation local

Currently the current backtrace is a unique global state without precise semantics, which makes its optimization hard. Since the only points where the current backtrace is clear are at the start of
exception handlers we propose to restrict the creation of backtraces (Printexc.print_backtrace, get_backtrace, get_raw_backtrace) inside exception handler and that they refer to these backtrace. The only exception I found is in Core where a function factorized code and which is used only in exception handler.

the seventh commit adds a warning that checks that the functions are used only inside exception handlers. A project can already turn this warning as an error to check if there is such occurrences.
the fifth commit adds a backtrace binding for exception handler (rec like record backtrace).

try ...
with exn when guard rec bt -> ... bt ...

The function Printexc.print_backtrace, get_backtrace, get_raw_backtrace have not yet been made to refer to the backtrace at the start of the exception handler but it is easy to do so once the
warning becomes a real error.

Compute backtrace as late as possible

This part have not been implemented and is the hardest to implement. We are looking for feedback.

Since the backtrace is computed always at raise site when -g is specified and the recording of backtrace activated, it is often computed without behind ever used. That's why raise_notrace have
been added for not computing backtrace. However it is more natural to specify if you want the backtrace at the exception handler site. We propose that if the backtrace is not binded in an exception handler (no rec bt, Printexc.print_backtrace, get_backtrace,
get_raw_backtrace) the backtrace is not computed.

For doing that we think that the way the stack is handled during exceptions must be modified in a way similar but a lot simpler and less powerful than resumable exception and algebraic effect:

raise doesn't pop the stack, but executes the matching of the exception and executes guards of exception handlers directly on the current stack,
at the start of an exception handler the backtrace is computed if needed and the stack is poped,
the execution of a guard could access spilled values but since the stack has not been poped the values are not relative to the stack pointer. So the loading of spilled register must be modified in the guards or the values copied at the current stack pointer.
during the computation of the backtrace of an exception raised inside a guard the portion of stack kept must be jumped,
the raise of Stack_overflow must be handled specially by computing directly the backtrace and poping the stack.

The advantages are:

No need for raise_notrace because if you don't use the backtrace it is not computed.
code compiled with -g and with backtrace recording activated are a lot more efficient, if the code doesn't bind any backtrace it is as fast as if the backtraces are not recorded.

Drawbacks:

the compilation of code compiled without -g must be also modified
the stack is bigger when computing guards of exception handlers and it is not natural. However such guards are often small.

I discussed this proposition with @def-lkb and @chambart at the last OUPS. We are convinced that it is possible and interesting to implement. Does the core developers agree, have some remarks or oppositions? Does OCaml developers bind backtraces outside exception handlers?

The reference given are the current results not the ideal results. The tests show cases where the backtrace we want to use during reraise have been lost by intermediary raise.

Previously this raise was transformed into a reraise although the backtrace used can't be the one of the first try. Remove this transformation by simplifying the code. ```ocaml try e1 with exn -> try e2 with _ -> raise exn ```

It reraises an exception just after copying the given backtrace to the backtrace buffer. The primitive only does the copying part, the compiler adds the reraise.

The heuristic that transform raise into reraise automatically save the raw_backtrace using caml_get_exception_raw_backtrace at the start of the handler and use caml_reraise_raw_backtrace just before the reraise. WARNING: If an exception is used inside a guard of a try_with the backtrace is still wrong.

Add the construction [rec id] in exception handler that should bind the backtrace of the point of programm where the exception have been raised. The type of the backtrace is `Printexc.raw_backtrace`. Currently it is implemented by adding a `let id = Printexc.get_raw_backtrace in` so if a raise is done during a previous when clause the backtrace is still the wrong one. ```ocaml try exp with exn when exp rec id -> exp ```

In a very inefficient way. Save the backtrace before guards of `try with` using `get_raw_backtrace` and restore it using `reraise_raw_backtrace` (that doesn't reraise by itself).

Add a warning for use of these functions outside exception handler: - print_backtrace - get_backtrace - get_raw_backtrace It is not the application that is warned, but just the use of the identifier.

gasche · 2015-11-18T16:13:18Z

We discussed it at the developer meeting today. The various advanced aspect of the proposal are too fresh to be included in the next release, but your proposal of a reraise <raw-trace> <exn> primitive/function was relatively well-received. Could you propose a patch for this in the following weeks, so that we can discuss it in time for the 4.03 feature freeze in a month?

let-def · 2016-07-08T10:41:56Z

testsuite/tests/backtrace/backtrace2.ml

  with Error "a" -> print_string "a"; print_newline(); 0
     | Error "b" as exn -> print_string "b"; print_newline(); raise exn
     | Error "c" -> raise (Error "c")
+     (** [Error "d"] not catched *)


let-def · 2016-07-08T10:52:49Z

Is it worth the pain of executing the when guard on top of the stack?

Executing pattern matching only should be enough to catch reasonable use cases.
Allowing arbitrary user code to be executed in this context seems hard to achieve (but I don't know the details) for unclear benefits. The same apply to lazy values being forced in a try-handler.

This of course is a premature concern. I mention it because putting the rec t before the when guard in the grammar would highlight that recording happen before guard execution, and give us more freedom to chose an implementation later.

let-def · 2016-07-08T10:53:19Z

The current proposal specializes the grammar rules for try-cases.
As a result, backtrace recording is not possible in match _ with exception _ -> _ (which is not supported at all, reraising strategy fails in it even without rec t).
I would suggest always allowing the backtrace binding and rejecting incorrect cases later.

let-def · 2016-07-08T11:15:58Z

The rec t is encoded as a tuple pattern:
try x with exn rec bt -> () is equivalent to try x with (Some bt, exn) -> ().

A proper representation is probably preferable :).

bobot · 2016-07-08T14:46:53Z

Is it worth the pain of executing the when guard on top of the stack?

There is two opposites objectives:

Providing the right backtraces
Being efficient: don't copy raw_backtrace and don't allocate unnecessarily

Executing on top of the stack allows to have both, and it could be more efficient than currently since instead of computing backtrace at raise it could be done only when needed.

However other tradeoff can be found. For example we could keep on the side the caml_backtrace_buffer and replace it by NULL. That will cost an allocation if the when clause use an exception. We could keep a cache of buffers, ...

I mention it because putting the rec t before the when guard in the grammar would highlight that recording happen before guard execution, and give us more freedom to chose an implementation later.

Funnily I preferred to put it after so that it is clear that the backtrace variable is not binded in the when clause, so that we have more freedom 😸 .

A proper representation is probably preferable :).

@lpw25 proposed the syntax try x with (exn,bt) -> (). which bind bt before the when clause, but we can still forbid its use in the when clause.

lpw25 · 2016-07-08T17:47:37Z

Yes I like | Foo, bt -> .... Although I might try to make the parsing such that | (Foo, bt) -> ... was disallowed since to me the parentheses imply that it is a single-value.

I also agree with @let-def that supporting when conditions before deciding whether to take the backtrace is not worth the trouble. It seems difficult to implement the right semantics and unlikely to be used that much in practice. So I would go with | Foo, bt when ... -> ... as the syntax.

(I might even be tempted to just ban when clauses on exceptions if I was going to use the efficient early matching implementation, based on the reasoning "You can't write that because it cannot be implemented efficiently").

damiendoligez · 2016-07-19T14:52:51Z

Yes I like | Foo, bt -> .... Although I might try to make the parsing such that | (Foo, bt) -> ... was disallowed since to me the parentheses imply that it is a single-value.

This is too much overloading for pair syntax, especially if you start adding funky restrictions on parentheses. What about | Foo with bt -> ... or some other keyword ?

mshinwell · 2016-12-08T11:30:47Z

Here are some details from a Mantis issue (#7262) that I think can be reasonably incorporated into this GPR. In the example below the second and subsequent exception raises are treated as re-raises, which leads to the second and subsequent backtraces containing stale backtraces.

external foo : unit -> unit = "stub_foo"

let bar b = foo b

let _ =
  Printexc.record_backtrace true;
  for i = 0 to 3 do
    match bar () with
    | _ -> ()
    | exception Not_found -> Printf.printf "%s------\n%!" (Printexc.get_backtrace ())
  done

together with the C stub:

#include <caml/fail.h>
value stub_foo(value arg)
{
        caml_raise_not_found();
}

Fixing caml_raise_not_found to clear the backtrace buffer only seems like a partial solution to the problem.

xavierleroy · 2019-10-14T17:37:27Z

Three years later, where are we with these issues and proposed improvements to backtraces?

Simplifies addition of new operations and safe with existing code.

* Revert "Use polymorphic compare for [Arch.equal_*] (ocaml#293)" This reverts commit a4cd7b7. * Simplify the case analysis when the arguments are not equal and still have warning 4 on. * Update equal_specific_operation with recently added operations

gasche · 2023-03-10T12:20:04Z

We should turn this PR into an RFC to discuss the design we want. I agree that this area is in dire need of well-designed improvements. Do we have a volunteer to write an RFC? Otherwise, should we just close?

damiendoligez · 2023-07-12T13:34:57Z

Archiving this PR, but an RFC is still welcome.

bobot added 7 commits November 14, 2015 10:56

Add tests for wrong backtrace with reraise

abe7753

The reference given are the current results not the ideal results. The tests show cases where the backtrace we want to use during reraise have been lost by intermediary raise.

Simplify the code of reraise exception heuristic

3f21271

Previously this raise was transformed into a reraise although the backtrace used can't be the one of the first try. Remove this transformation by simplifying the code. ```ocaml try e1 with exn -> try e2 with _ -> raise exn ```

Add caml_reraise_raw_backtrace

4432762

It reraises an exception just after copying the given backtrace to the backtrace buffer. The primitive only does the copying part, the compiler adds the reraise.

Restore backtrace after guard execution

2884ba3

In a very inefficient way. Save the backtrace before guards of `try with` using `get_raw_backtrace` and restore it using `reraise_raw_backtrace` (that doesn't reraise by itself).

Warning for backtrace outside exn handler

ca46813

Add a warning for use of these functions outside exception handler: - print_backtrace - get_backtrace - get_raw_backtrace It is not the application that is warned, but just the use of the identifier.

This was referenced Dec 21, 2015

Proper backtraces upon compiler failure #358

Closed

Use reraise_raw_backtrace primitive in the compiler #374

Merged

Add reraise_raw_backtrace primitive #378

Merged

damiendoligez added this to the 4.04-or-later milestone Jan 22, 2016

mshinwell changed the title ~~[WIP] More precise and efficient handling of backtraces~~ More precise and efficient handling of backtraces Jun 10, 2016

mshinwell added the work-in-progress label Jun 10, 2016

let-def reviewed Jul 8, 2016
View reviewed changes

damiendoligez removed this from the 4.04 milestone Aug 2, 2016

damiendoligez added this to the long-term milestone Sep 27, 2017

This was referenced Mar 14, 2019

Backtrace-friendly finalizers #7163

Closed

backtraces are concatenated when flambda turned on #7262

Closed

lthls pushed a commit to lthls/ocaml that referenced this pull request Feb 23, 2021

Add machine readable inlining reports (ocaml#293)

48c237c

poechsel pushed a commit to poechsel/ocaml that referenced this pull request Mar 1, 2021

Add machine readable inlining reports (ocaml#293)

71ff110

poechsel pushed a commit to poechsel/ocaml that referenced this pull request Mar 1, 2021

Add machine readable inlining reports (ocaml#293)

98e0304

chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Jan 4, 2022

Use polymorphic compare for [Arch.equal_*] (ocaml#293)

a4cd7b7

Simplifies addition of new operations and safe with existing code.

damiendoligez closed this Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More precise and efficient handling of backtraces#293

More precise and efficient handling of backtraces#293
bobot wants to merge 7 commits intoocaml:trunkfrom
bobot:explicit_backtrace_binding

bobot commented Nov 16, 2015

Uh oh!

gasche commented Nov 18, 2015

Uh oh!

let-def Jul 8, 2016

Uh oh!

let-def commented Jul 8, 2016 •

edited

Loading

Uh oh!

let-def commented Jul 8, 2016 •

edited

Loading

Uh oh!

let-def commented Jul 8, 2016

Uh oh!

bobot commented Jul 8, 2016 •

edited

Loading

Uh oh!

lpw25 commented Jul 8, 2016

Uh oh!

damiendoligez commented Jul 19, 2016

Uh oh!

mshinwell commented Dec 8, 2016

Uh oh!

xavierleroy commented Oct 14, 2019

Uh oh!

gasche commented Mar 10, 2023

Uh oh!

damiendoligez commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

bobot commented Nov 16, 2015

Keeping correct backtraces

Keeping the backtrace creation local

Compute backtrace as late as possible

Uh oh!

gasche commented Nov 18, 2015

Uh oh!

let-def Jul 8, 2016

Choose a reason for hiding this comment

Uh oh!

let-def commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

let-def commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

let-def commented Jul 8, 2016

Uh oh!

bobot commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpw25 commented Jul 8, 2016

Uh oh!

damiendoligez commented Jul 19, 2016

Uh oh!

mshinwell commented Dec 8, 2016

Uh oh!

xavierleroy commented Oct 14, 2019

Uh oh!

gasche commented Mar 10, 2023

Uh oh!

damiendoligez commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

let-def commented Jul 8, 2016 •

edited

Loading

let-def commented Jul 8, 2016 •

edited

Loading

bobot commented Jul 8, 2016 •

edited

Loading