Repository navigation
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.
There was a problem hiding this comment.
I don't see any particular reason, fixed
| } | ||
| 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.
There was a problem hiding this comment.
I think this was just for debugging, switched to failwith. However, I think both this failwith and the exception handler not found in the current callstack case can be hit if we start tracing between the handler and the exception, right? (Same applies to switching to a previously-unknown fiber and then raising into a handler installed on that fiber.)
| | #(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?
There was a problem hiding this comment.
I believe it's still true that all Frame.t chains in a trace segment end with the same sentinel. Suspending a callstack by packaging it into a Fiber.t removes the callstack from the trace segment, and resuming it splices it back into the correct path to the current sentinel. Indeed reparent can easily violate this invariant, so I at least renamed it and expanded the comment
There was a problem hiding this comment.
The sentinel being the common ancestor of Frame.t chains is the reason it's the only Frame.t whose parent can be modified safely; for any other Frame.t, we have no guarantee that this reparenting will apply to every Frame.t chain in the Trace_segment. Here's a case where this breaks things:
1 Call perform0 -> caml_perform
2 Jump caml_perform+0xb5 -> caml_apply3 # perform hook, effect_handlers empty
3 Jump caml_apply3 -> effc
4 Call effc -> continue
5 Call continue -> caml_resume
6 Jump caml_resume+0xda -> fun # resume hook
Output of write_trace:
0ns ENTER caml_apply3
0ns ENTER [zero or more unknowable frames]
1ns ENTER perform0
1ns ENTER caml_perform
2ns EXIT caml_perform
write_trace RAISED: "got unexpected result" ((expected (From_perf perform0)) (got (From_perf continue)))
If you modify the parent of anything besides the sentinel, when writing out the trace the series of frames we enter via Calls is no longer guaranteed to match the series of frames we exit via Returns.
There was a problem hiding this comment.
Ah ok, I must not have realized older trace segments still referenced the same physical frames and read them during trace writing. It now creates a fresh copy of the callstack with the desired parent.
| -> 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.
77b92d7 to
b9f6592
Compare
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.