Clean up the stack implementations #54

Amanieu · 2016-09-17T04:21:47Z

Changes:

~~The valgrind stuff is moved out of the generators and into the stack implementations.~~
Stack is now an unsafe trait since it has a contract that must be upheld by an implementation.
SliceStack will now automatically align a slice on construction. However this means that its fields are now private.
Various cleanups and moving code around.

whitequark · 2016-09-17T11:26:25Z

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 · 2016-09-17T15:33:23Z

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 · 2016-09-17T16:05:08Z

src/stack/slice_stack.rs

+    ///
+    /// This function will automatically align the slice to make it suitable for
+    /// use as a stack. However this function may panic if the slice is smaller
+    /// than `STACK_ALIGNMENT`.


Where does it panic in release builds?

This may panic even in release builds: &mut slice[offset..(offset + adjusted_len)]

Worth a comment, I think.

whitequark · 2016-09-17T16:09:56Z

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 · 2016-09-17T17:46:36Z

I reverted the valgrind changes.

whitequark · 2016-09-17T18:26:38Z

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 · 2016-09-17T18:28:21Z

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 · 2016-09-17T18:30:02Z

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 · 2016-09-17T18:33:35Z

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.

whitequark · 2016-09-29T09:00:09Z

src/stack/slice_stack.rs

+        let adjusted_len = (slice.len() - offset) & !(::STACK_ALIGNMENT - 1);
+
+        // This may panic if offset is larger than the size of the slice
+        SliceStack(&mut slice[offset..(offset + adjusted_len)])


Please group these two panics into one explicit one, and write a test for both conditions.

The only panic is the explicit one. adjusted_len is guaranteed to be less than slice.len() - offset.

So the comment is wrong now?

Oops, forgot about that one. Fixed.

And the test?

Test added.

whitequark · 2016-09-29T10:00:04Z

src/stack/slice_stack.rs

+    }
+}
+
+#[cfg(test)]


These tests don't poke into any implementation details as far as I see, so shouldn't they go to tests/?

Well I feel that the precise behavior of whether it panics or not is a bit of an implementation detail. The only thing the interface guarantees is that it won't panic if the slice is larger than STACK_ALIGNMENT, and it may panic if it is smaller.

That's a weird contract...

Amanieu · 2016-09-29T17:30:50Z

I moved the tests into tests/stack.rs.

close #54

whitequark reviewed Sep 17, 2016

View reviewed changes

Amanieu force-pushed the stack branch from aab6fb3 to 5459f29 Compare September 17, 2016 17:46

Amanieu force-pushed the stack branch 3 times, most recently from 3b75d61 to 0b6f2b7 Compare September 17, 2016 18:31

Amanieu force-pushed the stack branch from 0b6f2b7 to 44b0e8e Compare September 17, 2016 18:36

Amanieu mentioned this pull request Sep 18, 2016

Stack should be an unsafe trait #39

Open

edef1c force-pushed the master branch from dd6cf23 to 5608d2d Compare September 22, 2016 17:41

Amanieu force-pushed the stack branch 2 times, most recently from 32dd60b to 4f4040b Compare September 29, 2016 08:31

whitequark reviewed Sep 29, 2016

View reviewed changes

Amanieu force-pushed the stack branch 2 times, most recently from 3a0682e to 3fb86e0 Compare September 29, 2016 09:58

whitequark reviewed Sep 29, 2016

View reviewed changes

Amanieu force-pushed the stack branch from 3fb86e0 to d6ff860 Compare September 29, 2016 10:17

Clean up the stack implementations

4bcbd4a

Amanieu force-pushed the stack branch from d6ff860 to 4bcbd4a Compare September 29, 2016 17:30

Amanieu changed the base branch from master to next-major September 29, 2016 17:35

Amanieu changed the base branch from next-major to master September 29, 2016 17:39

edef1c closed this in 400adf3 Oct 1, 2016

edef1c pushed a commit that referenced this pull request Feb 25, 2017

Clean up the stack implementations

fc4cdbf

close #54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up the stack implementations #54

Clean up the stack implementations #54

Amanieu commented Sep 17, 2016 •

edited

whitequark commented Sep 17, 2016

Amanieu commented Sep 17, 2016

whitequark Sep 17, 2016

Amanieu Sep 17, 2016

whitequark Sep 17, 2016

whitequark commented Sep 17, 2016

Amanieu commented Sep 17, 2016

whitequark commented Sep 17, 2016

whitequark commented Sep 17, 2016

Amanieu commented Sep 17, 2016

whitequark commented Sep 17, 2016

whitequark Sep 29, 2016

Amanieu Sep 29, 2016

whitequark Sep 29, 2016

Amanieu Sep 29, 2016

whitequark Sep 29, 2016

Amanieu Sep 29, 2016

whitequark Sep 29, 2016

Amanieu Sep 29, 2016

whitequark Sep 29, 2016

Amanieu commented Sep 29, 2016

Clean up the stack implementations #54

Clean up the stack implementations #54

Conversation

Amanieu commented Sep 17, 2016 • edited

whitequark commented Sep 17, 2016

Amanieu commented Sep 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented Sep 17, 2016

Amanieu commented Sep 17, 2016

whitequark commented Sep 17, 2016

whitequark commented Sep 17, 2016

Amanieu commented Sep 17, 2016

whitequark commented Sep 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Sep 29, 2016

Amanieu commented Sep 17, 2016 •

edited