search

edef1c / libfringe

a Rust library implementing safe, lightweight context switches, without relying on kernel services
https://edef1c.github.io/libfringe
Apache License 2.0
512 stars 31 forks source link

Clean up the stack implementations #54

Closed Amanieu closed 8 years ago

Amanieu commented 8 years ago

Changes:

whitequark commented 8 years ago

First, can you please separate the "clean ups" into distinct commits?

The valgrind stuff is moved out of the generators and into the stack implementations.

Uh, how about no. Now anyone implementing a stack externally will now have to 1) implement the Valgrind interface 2) implement interfaces of any other debugging tools we may be interested in the future 3) predicate on the architecture-specific availability of Valgrind themselves. This significantly decreases ergonomics (and also it's not a cleanup, it creates duplication).

SliceStack will now automatically align a slice on construction. However this means that its fields are now private.

Again, how about no. This makes it completely useless for its original purpose, that is using a static mut for a stack, since you cannot construct it. You could as well just remove SliceStack entirely now.

Amanieu commented 8 years ago

Again, how about no. This makes it completely useless for its original purpose, that is using a static mut for a stack, since you cannot construct it. You could as well just remove SliceStack entirely now.

Can't you just use a static mut [u8; N] and create a SliceStack from that?

Uh, how about no. Now anyone implementing a stack externally will now have to 1) implement the Valgrind interface 2) implement interfaces of any other debugging tools we may be interested in the future 3) predicate on the architecture-specific availability of Valgrind themselves. This significantly decreases ergonomics (and also it's not a cleanup, it creates duplication).

The motivation here was to associate the debug info with the stacks rather than with the generators. Consider the case where a stack is reused for multiple generators, this would cause the stack to be registered and de-registered multiple times.

whitequark commented 8 years ago

The motivation here was to associate the debug info with the stacks rather than with the generators. Consider the case where a stack is reused for multiple generators, this would cause the stack to be registered and de-registered multiple times.

I don't see how this is a problem. You're sacrificing ergonomics in a misguided attempt to improve efficiency of a feature only useful for debugging in the first place. (And we should also gate the valgrind stuff behind #[cfg(build = "debug")].)

Can't you just use a static mut [u8; N] and create a SliceStack from that?

Hm, I tried that and encountered some sort of problem, but I don't recall it now. Disregard that comment then.

That said, if SliceStack::new didn't mess with valgrind then it could be a const fn.

Amanieu commented 8 years ago

I reverted the valgrind changes.

whitequark commented 8 years ago

LGTM. SliceStack's new can be made a const fn later if necessary, or the field exposed anyway, or something to that degree. As it is it is definitely nicer than what I had, so your change there is a net win.

whitequark commented 8 years ago

There's no real point in making Stack unsafe but since people persistently point that out I suppose there is no real harm either.

Amanieu commented 8 years ago

I don't think SliceStack::new can be made a const fn since it needs to inspect the address of the slice to ensure that it is properly aligned.

Also, publicly exposing the field would allow safety violations (in particular, the alignment restriction won't be enforced).

whitequark commented 8 years ago

I don't think SliceStack::new can be made a const fn since it needs to inspect the address of the slice to ensure that it is properly aligned.

Right.

Also, publicly exposing the field would allow safety violations (in particular, the alignment restriction won't be enforced).

Using SliceStack is already unsafe since it provides no guard page; exposing the field could be seen as inelegant design since it lets you violate what's supposed to be an invariant, but it does not add any more unsafety.

Amanieu commented 8 years ago

I moved the tests into tests/stack.rs.