-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
issue #42 moving join and yield into stdlib #185
Conversation
This is great. Sorry for the slow uptake; got side tracked the past week and this fell through the cracks. Can you file a contributor form with the foundation? It's just a blanket declaration of IP originator-ship on anything you contribute. Only has to be done once. |
I'm glad you're happy with the patch! I've just submitted a CA to Mozilla. |
Hi, We've received your committer agreement, thanks. A few points: First, it's best to make a separate branch and rebase that branch forwards while it's waiting for me to integrate. As presented, this change requires me to cherry-pick just the commit in question, which is not ideal for history-keeping, and gets cumbersome with multiple commits on a branch. Second, the change doesn't, unfortunately, work. It's close but there are a couple bugs. Aside from a new test being added since your patch (which needs updating to handle it), there's a bigger issue that the yield and join upcalls have ... quite intimate knowledge of the call stack that's calling them. If you try running 'make check' on linux with valgrind, you'll see the tests generate memory faults. What's happening is that the upcalls eventually call task->yield(), which is very specific about having to know how many args are in its caller's frame, and assumes that its caller is a cross-stack call (rust-to-C). Since you're violating both of those assumptions by indirecting through builtin_yield and builtin_join, it doesn't work. I've tried moving the code in question out of rust_upcall and into rust_builtin ("upcall" as a term we try to reserve for calls-the-compiler-synthesizes) and it .. works slightly better. But I still get valgrind errors. Not sure what else is going wrong. I'll be around over the next few days if you want to diagnose this further, figure out what's up. Sorry, yielding (safely descheduling a task mid-call) is the most delicate code-path in the runtime. It's devilishly hard to get right. |
Hey graydon, Sorry for the very slow response, it's been a hectic season. Given these issues, please let's reject my pull request. If I can revisit this, and fix the bugs, I'll submit a new request in the spring. [Marking this request closed.] |
Various pointer-related things
Fix broken example in previous PR
Add dummy fast math implementation
This is a draft pull request towards solving #19. ## Changes - Added wrappers for `transmute_unchecked()` - Annotated these wrappers with contracts - Wrote harnesses that verify these wrappers Note: the reason we write wrappers for `transmute_unchecked()` and we annotate those wrappers is that function contracts do not appear to be currently supported for compiler intrinsics (as discussed in [rust-lang#3345](model-checking/kani#3345)). Also, rather than using a single wrapper for `transmute_unchecked()`, we write several with different constraints on the input (since leaving the function parameters completely generic severely restricts what we can do in the contracts, e.g., testing for equality). This is not intended to be a complete solution for verifying `transmute_unchecked()`, but instead a proof of concept to see how aligned this is with the expected solution. Any feedback would be greatly appreciated -- thank you! By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses. --------- Co-authored-by: AlexLB99 <a6leblan@uwaterloo.ca>
Hi graydon,
Let me know what you think.
Best,
Graham