OxCaml Stack Switching - #376
Conversation
Signed-off-by: Josiah White <me@josiahwhite.io>
|
@TheNumbat this will need to be updated for oxcaml/oxcaml#6440 |
| } | ||
| | Ptwrite of | ||
| { location : Location.t | ||
| ; data : string |
There was a problem hiding this comment.
Why is data of type string? The ptwrite instruction can only hold a 32-bit/64-bit payload, and judging by perf_ptwrite_event_re in src/perf_decode.ml it appears perf script renders it as a number.
| } | ||
| end | ||
|
|
||
| module Fiber = struct |
There was a problem hiding this comment.
This could really use a doc-comment explaining things like:
- What does a
Fiber.trepresent? - Why do we need special handling for them?
- etc.
| ; symbolizer : Symbolizer.t | ||
| ; ocaml_exception_info : Ocaml_exception_info.t or_null | ||
| ; exception_handlers : Frame.t Vec.t | ||
| ; effect_handlers : (Frame.t * exn_depth:int) Vec.t |
There was a problem hiding this comment.
Doc-comments explaining these fields would be great.
| match Vec.last t.exception_handlers with | ||
| | This dst_frame -> | ||
| Vec.pop_back_unit_exn t.exception_handlers; | ||
| if not (Symbol.equal dst_frame.location.symbol dst.symbol) |
There was a problem hiding this comment.
Why did you weaken this from an assert to a log? I'd prefer to keep this strict, getting this wrong is generally evidence of a correctness issue.
| | #(maybe_frame, ~distance, ..) -> | ||
| (* We are probably raising into an exception handler much further up the stack that we never saw the entrance into. *) | ||
| let distance = | ||
| (* - Add 1 to the distance for the [_phantom_frame] we are injecting. |
There was a problem hiding this comment.
Could you please restore the comment indentation to what it was previously? It looks like is an issue in most of the existing comments you moved around.
| end | ||
|
|
||
| (* The [location], [parent], and [kind] fields are actually **immutable** except for on [Sentinel.t] instances. *) | ||
| (* The [location], [parent], and [kind] fields are actually **immutable** except for on [Sentinel.t] instances. |
There was a problem hiding this comment.
parent being immutable save for on Sentinel.ts (when parent is Null) was vital for correctness, because the invariant that all Frame.t chains in a trace-segment end with the same Sentinel.t is what grants you the ability to do an O(1) update to place a new parent frame at the top of stack and know that it applies to all the frames in the trace-segment. You don't get this guarantee with non-sentinel frames. Are you sure this is not broken?
| -> time:Time_ns.Span.t | ||
| -> unit | ||
|
|
||
| type flow |
There was a problem hiding this comment.
Minor nit: module Flow : sig type t end would be preferable.
5 participants
Adds special handling for OCaml 5 effect primitives.
In OxCaml, if the
ptwriteinstruction is supported, the runtime will emit events that record the fiber ID upon perform and resume. Magic trace can use this ID to save and restore the fiber's callstack and exception handling state. We now also generate flow events connecting the perform and resume pair. Support forptwriteevents comes from #328.If
ptwriteevents are not present, we still try to track state for the current fiber, which means programs that perform and resume with a stack discipline will work correctly. Otherwise, we still use the most recent state, so callstacks may be incorrect, but the trace still mostly works (e.g. themigratetest hits unknown return warnings, but still produces something coherent).Adds several sample programs that exercise effects and exceptions. They all give the expected structure with
ptwriteevents and something coherent without.There are currently a bunch of hardcoded offsets to detect the effect operations - I'll add these to the compiler output (alongside the exn handler info) and read them in another pr.