[RFC] Add Exn module by nojb · Pull Request #2137 · ocaml/ocaml

nojb · 2018-11-06T10:23:27Z

This PR introduces modules Exn and Stack_trace, corresponding to the built-in type exn, taking advantage at the same time to reorganize the Printexc API (which would eventually be deprecated).

The differences between Printexc and Exn/Stack_trace are roughly the following:

A type alias Exn.t for exn is added;
Functions are renamed/reorganized, as per the table below.

`Printexc`	`Exn` / `Stack_trace`
`print`	removed
`catch`	removed
`print_backtrace`	removed
`get_backtrace`	removed
`record_backtrace`	`Exn.set_tracing`
`backtrace_status`	`Backtrace.tracing`
`register_printer`	`Exn.register_printer`
`raw_backtrace`	`Stack_trace.t`
`get_raw_backtrace`	`Exn.last_trace`
`print_raw_backtrace`	removed
`raw_backtrace_to_string`	`Stack_trace.to_string`
`raise_with_backtrace`	`Exn.raise_with_trace`
`get_callstack`	`Stack_trace.current`
`backtrace_slot`	removed
`backtrace_slots`	`Stack_trace.slots`
`Slot.t`	`Stack_trace.Frame.t`
`Slot.is_raise`	`Stack_trace.Frame.is_raise`
`Slot.is_inline`	`Stack_trace.Frame.is_inline`
`Slot.format`	`Stack_trace.Frame.format`
`Slot.location`	`Stack_trace.Frame.location`
`location`	`Stack_trace.Frame.location`
`raw_backtrace_slot`	`Stack_trace.Frame.Raw.t`
`raw_backtrace_length`	`Stack_trace.length`
`get_raw_backtrace_slot`	`Stack_trace.Frame.Raw.get`
`convert_raw_backtrace_slot`	`Stack_trace.Frame.of_raw`
`get_raw_backtrace_next_slot`	`Stack_trace.Frame.Raw.next`
`exn_slot_id`	`Exn.id`
`exn_slot_name`	`Exn.name`

Lastly, the type Printexc.t introduced in Add paths for built-in types #1876 is removed.

nojb · 2018-11-06T10:25:57Z

Not sure about print_if_exn. Personally I never use this function. Maybe it should be moved to Fun?

gasche · 2018-11-06T10:34:52Z

It's nice to take the time for an API redesign for backtraces (cc @jhjourdan): the previous design was highly constrained by compatibility concerns.

I would have expected to see raise, reraise and (re)raise_with_backtrace also exposed in the new module (cc @bobot). If you are in the mood for giving examples in ocamldoc format, I think that two examples could be useful (and I think writing the examples down could help finalize the API choices):

catching an exception, printing the backtrace in a warning/alarm and raising a simpler exception
catching an exception, saving the backtrace, doing some resource handling, and reraising the same exception with the same backtrace.

Finally, if we can bikeshed, I don't like record_backtrace as a function name. From the name I expect this function to return or store a backtrace in some way. Maybe set_backtrace_recording and recording_status would make more sense? Or record_backtraces?

nojb · 2018-11-06T10:42:53Z

I would have expected to see raise, reraise and (re)raise_with_backtrace also exposed in the new module (cc @bobot).

Good point, I will add them.

If you are in the mood for giving examples in ocamldoc format, I think that two examples could be useful (and I think writing the examples down could help finalize the API choices):

catching an exception, printing the backtrace in a warning/alarm and raising a simpler exception

catching an exception, saving the backtrace, doing some resource handling, and reraising the same exception with the same backtrace.

Yes, I will add these. Thanks for the suggestion.

Finally, if we can bikeshed, I don't like record_backtrace as a function name. From the name I expect this function to return or store a backtrace in some way. Maybe set_backtrace_recording and recording_status would make more sense? Or record_backtraces?

I agree that the names are not good, but couldn't think of obviously better alternatives right away.
We should think of a name that works well for the pair record_backtrace, backtrace_status. Maybe Backtrace.{get,set}_recording ?

gasche · 2018-11-06T10:47:52Z

get_recording is not clear to me (I think of a recording as an object that I will get), I think "recording status" (or: configuration, setting, etc.) is the real concept here.

nojb · 2018-11-06T10:51:31Z

get_recording is not clear to me (I think of a recording as an object that I will get), I think "recording status" (or: configuration, setting, etc.) is the real concept here.

OK, what about Backtrace.{get,set}_recording_status ?

gasche · 2018-11-06T10:59:00Z

Yes, I think that would be nice.

alainfrisch · 2018-11-06T10:59:05Z

enable_recording: bool -> unit, recording_enabled: unit -> bool?

gasche · 2018-11-06T11:08:27Z

This would also be fine with me, although I find @nojb's proposal more systematic (it is easy to guess one name from the other).

stdlib/exn.mli

dbuenzli · 2018-11-06T11:26:15Z

stdlib/exn.mli

+(** Returns [true] if exception backtraces are currently recorded, [false] if
+    not. *)
+
+module Backtrace : sig


TBH I find this design quite messy can't we maybe review the organisation and prune a bit the terminology ?

Do we really need both call stack and backtrace in the vocabulary ? Isn't an "exception backtrace" simply an exception with a call stack ? Wouldn't it make sense to maybe have these Backtraces in their own toplevel module as Callstack ? I mean Exn.current_callstack really doesn't feel satisfactory to me.

If that is done then maybe rather than recording backtraces we can simply trace exceptions and maybe have Exn.{tracing,set_tracing}.

I agree current_callstack is odd, but where to put it?

Note that while there is only one type here ("backtraces/call stacks") there are two notions of "current": "current backtrace" means the backtrace/call stack of the "last" raise; "current callstack" means the backtrace/call stack at the call site. So in any case we will need some terminology to differentiate these two notions.

As long as backtraces/call stacks remain closely related to exceptions, it doesn't seem strange to me to keep them in a submodule of Exn (the issue of current_callstack notwithstanding). But if we expect them to have a life of their own (are we?) maybe putting them in their own top-level module could be considered.

Note that while there is only one type here ("backtraces/call stacks") there are two notions of "current": "current backtrace" means the backtrace/call stack of the "last" raise; "current callstack" means the backtrace/call stack at the call site. So in any case we will need some terminology to differentiate these two notions.

Well Exn.current_trace () or even Exn.last_trace () and Callstack.current ().

As long as backtraces/call stacks remain closely related to exceptions,

Daniel has a point that while backtraces are related to exceptions, call stacks are an independent concept. Currently there is only one primitive type raw_backtrace, but we could have a raw_callstack type (with the same underlying representation), or a less opinionated name shared between the two, such as "stack trace" or "stack fragment".

(I don't have time to devote to these API questions before mid-November, so unfortunately I won't participate in making proposals of my own. Good luck!)

nojb · 2018-11-06T12:09:58Z

Added raise, reraise and raise_notrace as suggested by @gasche, and
Removed print_current_backtrace, string_of_current_backtrace, print_if_exn, Exn.print as suggested by @dbuenzli

dbuenzli · 2018-11-06T13:13:50Z

Added raise, reraise and raise_notrace as suggested by @gasche, and

So we have raise_notrace but raise_with_backtrace. Could we maybe choose between trace and backtrace ?

I would rather try to go with the former, then the we can call these things "exception traces" and the whole process "exception tracing" which in turn might allow to get rid of the notion of recording (same would work with backtrace though "exception backtracing" feels a little bit more odd).

nojb · 2018-11-06T13:29:45Z

I went ahead and added a separate Callstack module. It does feel like a cleaner design. I also switched from backtrace terminology to just trace or exception trace when needed, so as suggested by @dbuenzli, we have Exn.tracing: unit -> bool, Exn.set_tracing: bool -> unit and Exn.last_trace: unit -> Callstack.t.

dbuenzli · 2018-11-06T13:39:35Z

stdlib/callstack.mli

+    @since 4.08 *)
+
+type t
+(** The abstract type [t] stores a callstack.


There's a mix of "callstack" and "call stack" in the text, one should be chosen. Maybe this suggests it should rather be Call_stack; stack already appears separately in Stack_overflow.

Personally, I find Call_stack a bit ugly. Maybe use callstack everywhere? Another possibility is to use Backtrace instead of Callstack as module name.

Backtrace is less clear than "call stack" or "stack trace" to me. I don't mind if you want to call it Callstack and if you use "call stack" in the text but at least do it consistently.

That said if people never use "callstack" as a single word, conventions would say that Call_stack is the right identifier.

You could also call it Stack_trace which would, terminlogy-wise, blend better with Stack_overflow exception.

In fact maybe "stack trace" is a better description of the value than "call stack". If I have a value for a call stack I would expect to be able to actually manipulate the call stack with it, which is not the case.

OK, let's go with Stack_trace. I pushed a commit with the renaming.

hannesm · 2018-11-06T13:47:28Z

I'm not entirely convinced of the name Exn - is there a reason to not name it Exception?

dbuenzli · 2018-11-06T14:14:45Z

I'm not entirely convinced of the name Exn - is there a reason to not name it Exception?

It's the name of the type in Pervasives.

nojb · 2018-11-08T20:11:04Z

I filled out the implementation, updated the manual, and rewrote the implementation of Printexc to call the functions from the new modules.

…race

stdlib/stack_trace.mli

dbuenzli · 2018-11-14T21:46:14Z

stdlib/stack_trace.mli

+    { filename: string;
+      line_number: int;
+      start_char: int;
+      end_char: int }


I wouldn't expose the record fields but rather move it to its own module with a creating function and accessors.

Indeed. Also, I would recommend putting the closing curly brace on its own line (as you have in other places), since it makes it quicker to add another record field.

Do we want to expose a "create" function, or just accessors?

I think it would definitively be useful be able to create values or redefine fields e.g. to remap filenames in a given context. I would also rename {start,end}_char to {start,end}_byte since it's likely a better characterization of what it is but that may not be consistent with the rest of the compiler apis.

stdlib/exn.mli

stdlib/stack_trace.mli

mshinwell · 2018-11-15T10:52:20Z

stdlib/stack_trace.mli

+
+val length: t -> int
+(** [length st] returns the number of (non-inlined) slots in the stack trace
+    [st]. *)


Why would this only return the number of non-inlined frames? Seems a bit odd.

As far as I can tell, this reflects the runtime representation of stack traces, so it is more efficient than "flattening" the stack traces fully. I wonder if this cost is worth the complication in the API.

mshinwell · 2018-11-15T10:54:27Z

stdlib/stack_trace.mli

+
+val slots: t -> Slot.t array option
+(** Returns the slots of a stack trace, or [None] if none of them contain useful
+    information.


The presence of this option type seems a bit strange. Maybe it would be better to always return the slots, and then have accessor functions that demonstrate what properties they have?

For one thing, the frames (slots) may not be obtainable if no debug info is available.

Do we want to distinguish between no debug info available and an empty array though ?

I don't see why one would want to.

OK, I threw away the option and and now return an empty array instead of None.

nojb · 2018-11-16T15:37:27Z

I am thinking of getting rid of the notion of "raw" stack trace frames (type Printexc.raw_backtrace_slot or Stack_trace.Frame.Raw.t) and just exposing the "friendly" type (type Printexc.backtrace_slot or Stack_trace.Frame.t).

What do you think?

gasche · 2018-11-17T13:42:15Z

Some use cases for callstack slots are performance-sensitive. For example, @jhjourdan uses them in the statistical profiler (you want to collect them for a reasonably high portion of your allocations while keeping a reasonably low overhead), and I would expect that they are traversed in reporting/statistics/aggregation logic which may also traverse non-small amount of data and have to be fast.

I haven't followed the recent discussions (slowly coming back to the real world^W^Wpull requests), and don't have a full picture of the API (maybe having a low-level notion of trace, but no low-level notion of slot, could be enough), but we designed the original API exposure with the idea that easy things should be easy and advanced/low-level things possible, and I liked that design choice.
(On the other hand, it certainly poses some tensions, for example in term of API evolution with evolving runtime features; inlined slots came only after, and disrupted some ideas we had about linear slot access. cc @let-def)

let-def · 2018-11-19T10:45:51Z

I think @diml also has some performance sensitive use cases for callstacks.
I would prefer to keep the low-level API available, making it explicit that the compatibility might be hard to preserve, rather than removing them.

Raw traces have to be kept as they are the ones that can be reinstalled with %raise_with_backtrace and afaict, the low-level API does not add much complications (if we ignore backward compatibility) while it is useful to implement the high-level API internally.

That being said, I would like the normal API to be as simple as possible, the length function for instance could stop distinguishing between regular and inlined functions. (... be should the stack trace type reflect the "physical" state of the stack or the "logical" one, that includes the inlined functions?).

let-def · 2018-11-19T10:47:56Z

... I might also add a richer version of stack traces which includes the names and values of locals in the future.

ghost · 2018-11-19T11:28:44Z

I did some experiment at some point, but right now we have nothing in production that is performance sensitive and relies on callstacks.

Currently, the only operations you can do on a raw_backtrace_slot is convert it and get the next entry. This allows to iterate over a raw backtrace until a certain point without allocating backtrace_slot values for the whole backtrace. It'd be nice to preserve this feature and it seems like it could be done without the raw_backtrace_slot type.

gasche · 2025-06-06T14:56:30Z

I heard feedback recently (during the MirageOS retreat) that the exception-backtrace API is hard to use, so I still think that there would be user interest in having an Exn module. If someone else than @nojb is willing to give this a try, it could end up being a very helpful change -- but it certainly will be somewhat frustrating in the middle.

dbuenzli reviewed Nov 6, 2018

View reviewed changes

nojb force-pushed the exn branch from 963a2b9 to 2ab410f Compare November 8, 2018 15:34

nojb force-pushed the exn branch 2 times, most recently from 3f46a59 to 1bc0233 Compare November 9, 2018 15:07

nojb added 11 commits November 13, 2018 20:51

Add Exn module

76e4ca8

Add raise, reraise, raise_notrace

3d61b1f

Rename record_{backtrace,status} => Backtrace.{set,get}_recording_status

2ffd2b0

Add docstring

0aed769

Remove print_if_exn, print_current_backtrace, string_of_current_backt…

8995d0a

…race

Doc

81dedef

Add Callstack module

fededc4

Doc

4038134

Rename Callstack to Stack_trace

48f8a07

Add implementation

c73f9f8

Implement Printexc in terms of Exn and Stack_trace

de7f579

nojb added 2 commits November 13, 2018 20:52

Avoid inlined wrapper around external

6fbaf7c

check-typo fixes

b13fb16

nojb force-pushed the exn branch from 1bc0233 to b13fb16 Compare November 13, 2018 19:52

gadmm mentioned this pull request Nov 14, 2018

protect: treat as an error the case where two exceptions are being raised #2118

Merged

dbuenzli reviewed Nov 14, 2018

View reviewed changes

mshinwell reviewed Nov 15, 2018

View reviewed changes

stdlib/stack_trace.mli Outdated Show resolved Hide resolved

mshinwell reviewed Nov 15, 2018

View reviewed changes

nojb added 5 commits November 15, 2018 11:34

Rename Stack_trace.{Slot => Frame, slots => frames}

86cb831

Reorder the interface of Exn, add doc headings

9654427

Add doc for raise functions

5177658

Typo

648cc2a

Use optional argument for Stack_trace.current

af963b1

nojb force-pushed the exn branch from af963b1 to 648cc2a Compare November 15, 2018 11:53

caml_get_current_callstack: add optional argument variant

2b57d3d

nojb force-pushed the exn branch from feb7180 to 2b57d3d Compare November 16, 2018 14:12

Stack_trace.frames: return an empty array instead of None

bee3009

gasche added the caml-weekly-news label Dec 3, 2018

nojb mentioned this pull request Feb 13, 2019

Deprecate Obj.set_tag #1725

Merged

nojb closed this Jan 8, 2020

dbuenzli mentioned this pull request Mar 1, 2022

OCaml 5.0 & unhandled exceptions #11074

Open

dbuenzli mentioned this pull request Mar 15, 2025

add Thread.set_extended_uncaught_exception_handler #13870

Closed

gasche mentioned this pull request Jun 6, 2025

Add Fun.todo #13101

Closed

johnyob mentioned this pull request Dec 23, 2025

feat: [@@unstable] features #14430

Closed

Conversation

nojb commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Nov 6, 2018

Uh oh!

gasche commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Nov 6, 2018

Uh oh!

gasche commented Nov 6, 2018

Uh oh!

nojb commented Nov 6, 2018

Uh oh!

gasche commented Nov 6, 2018

Uh oh!

alainfrisch commented Nov 6, 2018

Uh oh!

gasche commented Nov 6, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Nov 6, 2018

Uh oh!

dbuenzli commented Nov 6, 2018

Uh oh!

nojb commented Nov 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannesm commented Nov 6, 2018

Uh oh!

dbuenzli commented Nov 6, 2018

Uh oh!

nojb commented Nov 8, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mshinwell Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Nov 6, 2018 •

edited

Loading

gasche commented Nov 6, 2018 •

edited

Loading

nojb Nov 6, 2018 •

edited

Loading

mshinwell Nov 15, 2018 •

edited

Loading

gasche commented Nov 17, 2018 •

edited

Loading