Refactoring calls and returns #134
Conversation
| @@ -48,17 +48,15 @@ struct StackFrame<M: Memory> { | |||
|
|
|||
| /// `next_block` and `next_stmt` describe the next statement/terminator to execute (the "program counter"). | |||
| /// `next_block` identifies the basic block, | |||
| next_block: BbName, | |||
| /// next_block is None after calling a function without giving a basic block to return to. UB is raised after the function returns. | |||
| next_block: Option<BbName>, | |||
There was a problem hiding this comment.
I don't think we want to allow this to be None. Why shouldn't we have UB immediately any time this would be set to None?
There was a problem hiding this comment.
In the syntax for Terminator::Call, both the return place and the next block are optional (separately). This is set to None in any Call (or CallIntrinsic) without a next block, and that is sensible when calling a non-returning function. Returning anyway will then be UB, of course.
The conceptual idea with the field being Option would be that a stack frame may keep existing even when it will never execute another statement or terminator.
There was a problem hiding this comment.
a stack frame may keep existing even when it will never execute another statement or terminator.
I don't think that is something we want though.
Thinking about become, I would expect the old stack frame to be popped and then the new one to be pushed.
There was a problem hiding this comment.
Oh yes, for become it would be. But a regular call to fn() -> !?
There was a problem hiding this comment.
Sure, what about that? We can already represent them. Just make a call that has neither a return block nor a return place.
There was a problem hiding this comment.
A call to such a function is exactly the situation where (with this patch) next_block is set to None, as in "the caller will not be asked to execute another step, so it doesn't need to define what that step would be". Before this patch, the callee's caller_return_info.next_block would instead be set to None, so that if and when it tries to return, that will be UB.
With this patch, that return would technically be fine, but the next step in that thread is UB. This partially unifies the behaviour with returning from the bottom frame, which is not UB even if the function doesn't have a return local, although I would also like to change that towards requiring a return local to return at all.
There was a problem hiding this comment.
A call to such a function is exactly the situation where (with this patch) next_block is set to None, as in "the caller will not be asked to execute another step, so it doesn't need to define what that step would be". Before this patch, the callee's caller_return_info.next_block would instead be set to None, so that if and when it tries to return, that will be UB.
Okay. I just don't think that's a good change though -- the current behavior seems better to me. I'd like updating the "current basic block" in the caller's frame to happen upon return, not during the Call. It helps with diagnostics, and it IMO better represents the intended semantics: the jump happens on Return.
With this patch, that return would technically be fine, but the next step in that thread is UB. This partially unifies the behaviour with returning from the bottom frame, which is not UB even if the function doesn't have a return local, although I would also like to change that towards requiring a return local to return at all.
Not requiring a return local is mostly a convenience so that the main function can be called without allocating space for a return local, and to make writing some tests simpler. I agree it's kind of ugly though. I would be on-board with making return locals mandatory (or tying them up with whether a return block is present), assuming this doesn't cause undue amounts of pain.
There was a problem hiding this comment.
Okay. I just don't think that's a good change though -- the current behavior seems better to me. I'd like updating the "current basic block" in the caller's frame to happen upon return, not during the Call. It helps with diagnostics, and it IMO better represents the intended semantics: the jump happens on Return.
You have convinced me of that now. The rationale of making this change was more that the Return in one function shouldn't need to know anything about the caller's structure or stack frame. I previously considered Call to be something that is evaluated and then its done, and the entirety of the callee happens afterwards.
However, together with what I've learned from rust-lang/unsafe-code-guidelines#417, I'm now thinking that maybe Call should actually be a two-step thing: Pre-call, it prepares the return place (e.g. either allocate new place, or just pass the actual destination) and evaluates the arguments to form the callee's StackFrame. Post-call, if the return place was a temporary, copy to the actual destination and deallocate, and finally update next_block.
In other words, while a stack frame is not at the top of the stack, it has a Call "on hold" that will be finished after the function above it evaluates a Return. I'll have to see how it would end up looking in practice.
There was a problem hiding this comment.
Yes, that is a way to think about this. The CallerReturnInfo is basically the environment of a caller-defined closure that will be run after the callee returns to run the post-call actions. We don't actually want this to be a closure since having the data explicit will make it a lot easier to formally reason about this specification. (With such reasoning, equality might come in, and a struct like CallerReturnInfo has a much clearer notion of equality than a closure.)
| self.mutate_cur_frame(|frame| { | ||
| frame.next_block = next_block; | ||
| frame.next_stmt = Int::ZERO; | ||
| }); |
There was a problem hiding this comment.
I'm not sure I conceptually agree with this -- I quite deliberately had the caller's stack frame point to the Call operation while the call is active. This is needed, for example, to get reasonable backtraces: we want to know where in the caller we were when the call was made, not where we will be when the call returns.
There was a problem hiding this comment.
Indeed, for backtraces the call origin would need to be stored. A reasonable place might be the callee's CallerReturnInfo (probably renamed to just CallerInfo or something) . My understanding is still that this information is only needed for diagnostic reasons. It cannot affect the execution, right?
There was a problem hiding this comment.
It cannot affect the execution, right?
I think it cannot, yes.
But this diagnostics issue to me indicates that it's the wrong design choice to update the next_block before the call.
| // Therefore the thread must terminate now. | ||
| assert_eq!(Int::ZERO, self.thread_manager.active_thread().stack.len()); | ||
|
|
||
| return self.thread_manager.terminate_active_thread(); |
There was a problem hiding this comment.
Ah, this is a good catch, currently we are failing to free the locals of the bottom frame.
We should probably have some tests that check for memory leaks...
Changes in this PR:
These changes together mean the return from a function doesn't actually care about its caller's stack frame, which will help with further changes I have planned.
One downside is that this loses the information of which call-statement in the caller got us here, since it's possible for multiple call-statements to have the same next_block, but it could be tracked separately if needed for e.g. diagnostics.