Add the Allowed type to libtock_platform.

[jrvanwhy]

Allowed is the type that will be returned by Platform::allow() in the successful case. It represents a memory buffer shared with the kernel. It uses raw pointers and volatile writes to perform read/write accesses to the memory to avoid encountering undefined behavior.

This is the first step towards fixing #129 and #143.

[jrvanwhy]

The Travis failure is due to #225 and is unrelated to this PR; please review this PR when you get a chance.

[hudson-ayers]

This PR contains code that it is very important to get right, as mistakes could lead to very subtle and hard to trace bugs, so I tried to review it closely. In general, everything seems right to me. However, the comments about dropping Allowed values after replacing them seemed a little misleading to me.

In general, it almost feels like it might be better to disallow placing anything with a Drop implementation in an Allowed type, as it is impossible to know whether drop() will be called or not. But perhaps I am missing something.

[hudson-ayers]

I tested this by modifying replace(), and it does seem to be generally true.

#[test]
fn replace() {
    let dropped1 = core::cell::Cell::new(false);
    let dropped2 = core::cell::Cell::new(false);
    let dropped3 = core::cell::Cell::new(false);
    let mut buffer = DropCheck {
        flag: Some(&dropped1),
        value: 1,
    };
    let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
    assert_eq!(kernel_ptr.value(), 1);
    assert_eq!(dropped1.get(), false);

    // Simulate the kernel replacing the value in buffer. We don't drop the
    // existing value.
    kernel_ptr.set(DropCheck {
        flag: Some(&dropped2),
        value: 2,
    });
    let returned = allowed.replace(DropCheck {
        flag: Some(&dropped3),
        value: 3,
    });
    assert_eq!(returned.value, 2);
    assert_eq!(kernel_ptr.value(), 3);
    assert_eq!(dropped1.get(), false);
    drop(returned); // <-- Added by me
    assert_eq!(dropped1.get(), false); // <-- added by me
    assert_eq!(dropped2.get(), true); // <-- modified by me
    assert_eq!(dropped3.get(), false);
}

However, this behavior is definitely somewhat confusing, as the caller of replace cannot know whether the destructor will be called for the item originally inserted from the userspace side, or for the item inserted by the kernel.

It feels like it would be a mistake for any userspace implementation to rely on drop being called for values that have been Allowed, but I read this comment as suggesting that doing so is possible. Perhaps it should be reworded? Alternatively, perhaps I am misunderstanding?

[jrvanwhy]

In general, it almost feels like it might be better to disallow placing anything with a Drop implementation in an Allowed type

In principle, I agree. Putting a Drop type into a shared buffer seems like a footgun to me. However, the only way to disallow Drop is to require Copy, which seems unnecessarily restrictive. The behavior I implemented was to allow non-Copy types, but make sure that Allowed does not drop values. It may make sense to document that on Allowed directly rather than implicitly through member documentation.

Another thing we could do is to put a Copy bound on set, but still allow replace and take to be used with Drop types.

... or I could remove support for Drop types entirely.

What do you think?

[hudson-ayers]

In principle, I agree. Putting a Drop type into a shared buffer seems like a footgun to me. However, the only way to disallow Drop is to require Copy, which seems unnecessarily restrictive. The behavior I implemented was to allow non-Copy types, but make sure that Allowed does not drop values. It may make sense to document that on Allowed directly rather than implicitly through member documentation.

Another thing we could do is to put a Copy bound on set, but still allow replace and take to be used with Drop types.

Hmm...I agree that requiring Copy seems too restrictive -- slices are not Copy (rust-lang/rust#30483), and it definitely feels like we should support allowing slices. I also feel that having to use a different API for types that implement Drop is confusing in itself - while it would indicate to users that they should be careful, there would be lots of false positives for things like slices which have no Drop implementation but are not Copy. It is too bad something like rust-lang/rfcs#1148 is not possible.

... or I could remove support for Drop types entirely.

What do you think?

After thinking about this some more, I think that it is better to just leave things as-is but to document the potential failure of drops directly on Allowed, and make it clear in the documentation that Allowing a Drop type should only be done after careful consideration.

If there were a way to disallow Drop without requiring Copy I might feel differently :/

[jrvanwhy]

Hmm...I agree that requiring Copy seems too restrictive -- slices are not Copy (rust-lang/rust#30483),

That only applies to dynamically-sized slices. Arrays are Copy if the type they contain is Copy. Also, I will be introducing an AllowedSlice type soon that would be better for working with arrays.

[hudson-ayers]

Hmm...I agree that requiring Copy seems too restrictive -- slices are not Copy (rust-lang/rust#30483),

That only applies to dynamically-sized slices. Arrays are Copy if the type they contain is Copy. Also, I will be introducing an AllowedSlice type soon that would be better for working with arrays.

I was imagining that it would be common to share a portion of a userspace array with the Kernel (I had in mind a packet buffer in userspace, but only the portion of the packet that the app actually populates with data gets Allowed), which I believe would require slices. However if there is a separate AllowedSlice type that mitigates my concerns.

Outside of arrays/slices, I am not sure whether disallowing non-Copy types will lead to more headaches than removing the Drop footgun will save. Currently it does not seem that any libtock-rs driver allows() any Drop types or any non-Copy types, so experience is not a useful teacher. I am inclined to defer to your judgement on this :)

[jrvanwhy]

Because Allowed uses unsafe code, I decided to take the conservative route and add a Copy bound.

[hudson-ayers]

Looks good!

[jrvanwhy]

I believe this PR is ready to merge.

[hudson-ayers]

bors r+

[bors]

Build succeeded:

• continuous-integration/travis-ci/push