-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
provide guarantees about whether memory goes in the coroutine frame or stack frame #1194
Labels
Milestone
Comments
andrewrk
added
bug
Observed behavior contradicts documented or intended behavior
proposal
This issue suggests modifications. If it also has the "accepted" label then it is planned.
labels
Jul 4, 2018
andrewrk
added a commit
that referenced
this issue
Jul 7, 2018
* add std.atomic.QueueMpsc.isEmpty * make std.debug.global_allocator thread-safe * std.event.Loop: now you have to choose between - initSingleThreaded - initMultiThreaded * std.event.Loop multiplexes coroutines onto kernel threads * Remove std.event.Loop.stop. Instead the event loop run() function returns once there are no pending coroutines. * fix crash in ir.cpp for calling methods under some conditions * small progress self-hosted compiler, analyzing top level declarations * Introduce std.event.Lock for synchronizing coroutines * introduce std.event.Locked(T) for data that only 1 coroutine should modify at once. * make the self hosted compiler use multi threaded event loop * make std.heap.DirectAllocator thread-safe See #174 TODO: * call sched_getaffinity instead of hard coding thread pool size 4 * support for Windows and MacOS * #1194 * #1197
Note that this is especially important, because here's a modified version of the test that fails in ReleaseFast mode, even with the const std = @import("std");
const builtin = @import("builtin");
const assert = std.debug.assert;
var ptr: *u8 = undefined;
test "where does the coroutine memory go" {
var da = std.heap.DirectAllocator.init();
defer da.deinit();
const p = try async<&da.allocator> simpleAsyncFn();
_ = @noInlineCall(blowUpStack, 5);
assert(ptr.* == 0xee);
}
async fn simpleAsyncFn() void {
var x: u8 = 0xee;
ptr = &x;
suspend;
var a = &x;
}
fn blowUpStack(count: usize) usize {
if (count == 0) return 0;
asm volatile ("nop");
return @noInlineCall(blowUpStack, count - 1) + @noInlineCall(blowUpStack, count - 1);
} |
Here's a workaround to make it pass in ReleaseFast mode for now: const std = @import("std");
const builtin = @import("builtin");
const assert = std.debug.assert;
var ptr: *u8 = undefined;
test "where does the coroutine memory go" {
var da = std.heap.DirectAllocator.init();
defer da.deinit();
const p = try async<&da.allocator> simpleAsyncFn();
_ = @noInlineCall(blowUpStack, 5);
assert(ptr.* == 0xee);
}
async fn simpleAsyncFn() void {
// Comment this out to make the test fail
suspend |p| {
resume p;
}
var x: u8 = 0xee;
ptr = &x;
suspend;
}
fn blowUpStack(count: usize) usize {
if (count == 0) return 0;
asm volatile ("nop");
return @noInlineCall(blowUpStack, count - 1) + @noInlineCall(blowUpStack, count - 1);
} |
andrewrk
added a commit
that referenced
this issue
Jul 10, 2018
andrewrk
added a commit
that referenced
this issue
Jul 12, 2018
Closed
8 tasks
andrewrk
added a commit
that referenced
this issue
Mar 15, 2019
Before, allocator implementations had to provide `allocFn`, `reallocFn`, and `freeFn`. Now, they must provide only `reallocFn` and `shrinkFn`. Reallocating from a zero length slice is allocation, and shrinking to a zero length slice is freeing. When the new memory size is less than or equal to the previous allocation size, `reallocFn` now has the option to return `error.OutOfMemory` to indicate that the allocator would not be able to take advantage of the new size. For more details see #1306. This commit closes #1306. This commit paves the way to solving #2009. This commit also introduces a memory leak to all coroutines. There is an issue where a coroutine calls the function and it frees its own stack frame, but then the return value of `shrinkFn` is a slice, which is implemented as an sret struct. Writing to the return pointer causes invalid memory write. We could work around it by having a global helper function which has a void return type and calling that instead. But instead this hack will suffice until I rework coroutines to be non-allocating. Basically coroutines are not supported right now until they are reworked as in #1194.
Done with the merge of #3033 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This test fails:
In this example, even though a pointer to a variable
x
on the coroutine frame escapes, LLVM does not spillx
into the coroutine frame, because it is not referenced after a suspend point. You can see that if you reference the variable after the suspend point then the test passes becausex
is spilled into the coroutine frame.It is important that we support local variables that do not spill, because sometimes we need coroutine code to execute even after the frame has been destroyed. In this situation the unspilled variables are accessible while the spilled variables are not.
There is one more relevant piece of information here, which is that LLVM coroutines provide one more utility that zig does not expose directly to the user. This is that you can access the coroutine "promise" value from the handle. (In zig the handle is of type
promise
). Point here being that we could have async functions define a type which is guaranteed to be inside the coroutine frame, and is accessible via the promise handle.So here's one proposal:
Note that this makes the global capture unnecessary as the assert could be rewritten:
This proposal also makes it possible to write generators:
With this proposal, variables would work the same way they do now, and the first test case would still fail in the same way. However there would be an explicit feature to use when you want to guarantee that memory is inside the coroutine frame.
The text was updated successfully, but these errors were encountered: