Detect and abort tracing if "early return" occurs. #1513
Conversation
|
Looks good to me. |
ykrt/src/mt.rs
Outdated
| }; | ||
| }); | ||
| } | ||
|
|
||
| fn expect_stop_tracing(mt: &Arc<MT>, loc: &Location) { | ||
| let TransitionControlPoint::StopTracing = mt.transition_control_point(loc) else { | ||
| let TransitionControlPoint::StopTracing = | ||
| mt.transition_control_point(loc, 0 as *mut c_void) |
There was a problem hiding this comment.
Clippy will probably say to use ptr::null_mut().
There was a problem hiding this comment.
There are no warnings from Clippy. Note this is a c_void.
There was a problem hiding this comment.
It should work since null_mut<T>() parametrised. Fewer magic numbers, right?
| #[allow(dead_code)] | ||
| GrowsToHigherAddress, | ||
| /// The CPU stack grows "upwards" i.e. growth changes it to a higher address. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Is this second variant dead?
There was a problem hiding this comment.
Right now, yes. But having it there helps explain the other one IMHO.
|
Do I understand correctly that this will superceed #1481? Because we would no longer require the user to do manual annotations for early returns? |
Yes and yes. |
|
Force pushed the |
|
Surprised the casts were needed. Doesn't it just coerce? |
If we start tracing a loop and the function-with-a-control-point returns whilst tracing is ongoing, we are in dangerous territory. For quite a while we assumed that "dangerous territory" meant (gongs of doom!) "UB". Actually, we just hadn't created very good examples: when I did, it turns out that an `unwrap` in trace_builder fails if early return happens. We *could*, therefore, detect early returns there, but that means that we've done a lot of work before realising this. This commit instead has `mt.rs` detect early returns, and abort tracing, as soon as we reasonably can. [Note, however, that if a user annotates `return`s from the function, they could stop things even earlier. Is this worth it? It is quite complicated to explain and, as we'll see in a bit, it's not a very common case] All that said, it turns out early returns aren't that common: you have to start tracing on the same iteration that you "early return" from. This is definitely possible -- indeed the two C tests in the PR do that, one "normally" and one when side-tracing -- but it seems unlikely that it will happen very often. So although we could force the user to help us detect this even sooner, I'm not sure it's worth burdening them or us with that responsibility right now: the cases where it would save serious resources are borderline pathological. Indeed, the only such case I can think of is: if you start tracing from the "root" control point call frame, and then return to the main program (i.e. you will never go back to the interpreter), then tracing is never turned off. One day we can fix this, but I don't think we will encounter it very soon.
|
You're right. Force pushed the simplification. |
|
Thanks! |
If we start tracing a loop and the function-with-a-control-point returns whilst tracing is ongoing, we are in dangerous territory. For quite a while we assumed that "dangerous territory" meant (gongs of doom!) "UB". Actually, we just hadn't created very good examples: when I did, it turns out that an
unwrapin trace_builder fails if early return happens. We could, therefore, detect early returns there, but that means that we've done a lot of work before realising this.This commit instead has
mt.rsdetect early returns, and abort tracing, as soon as we reasonably can. [Note, however, that if a user annotatesreturns from the function, they could stop things even earlier. Is this worth it? It is quite complicated to explain and, as we'll see in a bit, it's not a very common case]All that said, it turns out early returns aren't that common: you have to start tracing on the same iteration that you "early return" from. This is definitely possible -- indeed the two C tests in the PR do that, one "normally" and one when side-tracing -- but it seems unlikely that it will happen very often. So although we could force the user to help us detect this even sooner, I'm not sure it's worth burdening them or us with that responsibility right now: the cases where it would save serious resources are borderline pathological. Indeed, the only such case I can think of is: if you start tracing from the "root" control point call frame, and then return to the main program (i.e. you will never go back to the interpreter), then tracing is never turned off. One day we can fix this, but I don't think we will encounter it very soon.