I'm writing a coroutine library and I'd like to declare it ready for general use soon.
However, there are bits of unsafe code in it. I've gone through it multiple times, written down arguments why I think it to be safe and tried to be generally conservative about the assumptions. But I would appreciate another pair of eyes to have a look, to check if I'm missing something or assume something that is not true.
Of course, review or suggestions about the safe code is also welcome, but I think the unsafe is the one with most impact.
It lives on github. I'll be glad to hear any comments, either here or as issues on the repository.
Are there some specific places/interactions that you’re worried about? Or any assumptions you make? If you set the table for reviewers, it may garner more responses. Some waypoints/focus points for a review would be helpful.
Well, there're 6 instances of unsafe blocks in the code in total (not counting comments and benchmarks). I'm a bit worried about all of them, but all except of the long one in Coroutine::wait are probably trivial.
One assumption I make is that types X<'a> and X<'b> have the same representation, which I think is the case but haven't found the promise anywhere.
But I'm more worried about the things I might have overlooked or didn't think about than the ones I did look into ‒ and I can't really make a table for that.
Hmm. That's interesting idea. The future's poll has &mut self available, so it can, in theory, call Coroutine::wait again on it. But the poll is called from the core, which should usually be outside of a coroutine (I'll have to think if I can smuggle it inside a coroutine context). It'll panic outside of coroutine context. But I'll have to think and try out what can or can't be done with this and how serious it is.
On the other hand, even if it was called reentrantly, I don't see how this could lead to UB. I think it is legal to have multiple *mut pointers to the same thing, isn't it? I'm pretty sure being able to put the same future multiple times (wrapped in this coroutine task) into the core will do interesting things, but in itself it should be well defined interesting things.
I don't think there's anything else „arbitrary“ the user could do ‒ the only difference from plugging the future into the core directly is the &mut self instead of self access to it.
To make it clear, I don't know if there are safety problems with this code, I've just seen "unsafe + callbacks + reentrancy = UB" pattern several time, for example, in the lazy_cell case: https://github.com/indiv0/lazycell/pull/74
Sure, it's appreciated. It is one of the things I haven't thought about and it deserves some thinking and experimenting what can go wrong. The rest is just the thinking out loud .
Yes, but this argument is on a bit different level than what I want. Maybe I should clarify.
The fact they are erased today doesn't mean they will be erased in the tomorrow version of rustc. That is what I mean by promise. Can I be sure this'll always work, or does it work „just by accident“?
The other part is, can the optimiser assume something based on the fact X<'a> and X<'b> are different types (or, are they really different types?)? Can it assume *mut X<'a> never points to the same place as another *mut X<'b>? (again, can, not does).
In other words, I think this works today OK, but is it guaranteed by something that'll keep working?
This is not an accident. Lifetimes have always been “sold” as compile-time only with no impact on codegen. I don’t know if it’s guaranteed anywhere but I’m almost certain this will continue to be the case.
Variance/subtyping, when applicable, says that X<'a> and X<'b> can be interchanged (again, when variance allows for it). So I think they’re different types (unless 'a == 'b) but there’s a possible subtyping relationship that muddies the water in terms of what “actual” type is present in some piece of code.
Raw pointers don’t have any aliasing guarantees on their own so the compiler cannot assume anything in that respect, lifetimes aside even.
I suspect Rust needs a formal memory model that will define these rules/guarantees authoritatively (as well as what unsafe code can assume, which is where these lifetime shenanigans usually come into play). My own opinion is that it’s extremely unlikely that lifetimes will ever influence codegen or anything in trans.
What if somebody wants to specializemyfunc<'a> with some faster version for myfunc<'static>? (because of it can use some fn memoizing_caluclate(&'static [u32;4])->... that squirrels away some values in addition to calculating)
Specialization is pre-codegen/trans. I don’t see this being much different from, say, transmuting some type to 'static and then calling a method that accepts only 'static lifetime - this is unsafe but it doesn’t effect the representation/layout or codegen.
This is why specialization has been considered unsound for a long time. That said, @nikomatsakis recently found an interesting solution: Baby Steps
TL;DR: Right now, lifetimes cannot influence trans/codegen, people really want to keep it that way, that means specializing for 'static probably isn't gonna work.
Why not just trat lifetime parameters just like a funny type parameters? It's obvious that type choice affect the code, so why can't lifetime choice affect the code? if ('static) { cache; use } else { use; } ...
I guess because if that were the case, the compiler would need to process one instantiation of the generic code per choice of lifetime parameter, instead of erasing the lifetimes after borrow checking as it does right now. This would in turn hurt compile times and code size. But that is only a guess.
Furthermore, Rust will then get the full nightmare of interaction between subtyping (lifetimes form a hierarchy, unlike the normal types) and generics, like with C++ templates. My humble opinion is we don't want to go in there.
.