Change implementation to support (and use) dyn Future for erased BoxScope

[steffahn]

No description provided.

[dureuill]

Hello,

sorry for the delay.

• I rebased on Introduce some macros for soundness #11
• I added some of the missing SAFETY comments

[dureuill]

What are the preconditions on this for fields to be sound? It looks to me like there are none, as just creating the pointers to the fields should always be valid, even if this is dangling or null.

I tried looking at the preconditions for addr_of_mut, but they're very unclear to me. The docs simultaneously states that " This means that addr_of_mut!(*ptr) is defined behavior even if ptr is null, dangling, or misaligned" and that "addr_of_mut!((*ptr).field) still requires the projection to field to be in-bounds, using the same rules as [offset].".

But the docs for offset stats that "Both the starting and resulting pointer must be either in bounds or one byte past the end of the same allocated object.", and "allocated object" is meaningless to me when talking of a nullptr.

Could you clarify the preconditions here?

[dureuill]

Hmmm, reading the docs very precisely seems to imply:

• addr_of_mut!(*(invalid)) is not, by itself undefined behaviour
• but addr_of_mut!(*(invalid).field) is undefined behaviour

Is that your interpretation as well? If yes then the precondition is that this is a valid mut pointer to RawScope<T, F>, what to you think?

[steffahn]

I think it needs to point to an allocation that can hold a full RawScope value. If I remember correctly, the relevant thing to look at is for pointers to be "dereferenceable", since the addr_of_mut invocation contains a pointer dereference. Ah, there we have it, I just said "allocation", too... just as meaningful as "allocated object" xD - as far as I know, null is not permitted here; I'm not sure how exactly dangling pointers would count; there are two different cases in my mind, the "use-after-free" one (i.e. either stack memory that went out of scope, or heap memory that was freed) and "the pointer was always dangling" (e.g. the ptr::dangling kind of method).

Even if the real requirements are weaker, it doesn't hurt to require more in the SAFETY comment. Notably, what is not a requirement is that self points to an initialized RawScope value; and I believe it wouldn't need to be aligned, too; though we only care about the uninitialized case being valid, anyway, the way we're using this fields function.

[dureuill]

Thanks for the prompt response, I'll try to update based on this, so basically my understanding is that this should be pointing to an allocated Scope that is not necessarily initialized.

This gives me a bit of pause though, due to our use of F: ?Sized.
In the case of F = dyn Future, how are we actually computing the offset of active_fut on an uninitialized RawScope? I believe the offset would depend on the alignment of F, which in the !Sized case cannot be known unless reading it from the vtable, requiring an initialized F, right?

Of course, it doesn't actually happen in our case, as we're computing the field either from the maybe-initialized but fully typed version, or from the erased but fully-initialized version.

[steffahn]

Ah, and besides allowing unintialized access, of course we're also using that addr_of_mut doesn't do any actual accesses behind the pointer, so in particular this fields method doesn't conflict with existing borrows of the state field.

I think the relevant documentation is the notion of "dereferenceable" mentioned here that

the memory range of the given size starting at the pointer must all be within the bounds of a single allocated object

(Notably, other requirements for "valid" pointers should be irrelevant, because we need neither validity for reads nor writes.)

So the takeaway would be that indeed NULL should not be allowed, and all our use-cases are fine as the memory region in question is inside of a single allocated heap-allocated memory section of the Box.

---

Also, the fields method of course also has safety-relevant post-conditions, e.g. that the pointers it returns point into the respective fields of *self, which is crucial to know for the code that uses them then. Really, fields is a rather leaky abstraction, designed just to avoid some code-duplication for the addr_of_mut calls. So it doesn't seem to bad if the SAFETY comments are kept quite concise, and it doesn't hurt if the SAFETY comments might not be fully comprehensive to all readers as one could always look back at its implementation.

[steffahn]

Oh, I hadn't read your reply yet (my last reply above was written without yours showing up in my browser).

This gives me a bit of pause though, due to our use of F: ?Sized.
In the case of F = dyn Future, how are we actually computing the offset of active_fut on an uninitialized RawScope? I believe the offset would depend on the alignment of F, which in the !Sized case cannot be known unless reading it from the vtable, requiring an initialized F, right?

That's indeed a very interesting train of thought. I believe you're fully correct that the fields method would do some computation involving alignment read from the vtable.

However, it's not a problem at all. Vtables are always valid, even for raw pointers. No requirement of the F value itself being initialized exists. This is also why something like ptr::null cannot work for T: !Sized. A valid vtable is already created ... well creation happens statically ... but let's say included/referenced in the pointer metadata, as soon as we're casting from a known Sized type. So the cast of *mut RawScope<T, F> to *mut RawScope<T, dyn Future<...>> does result in a correct vtable that contains alignment information from the original type F, regardless of whether or not the *mut RawScope<T, F> pointer pointed to anything valid. (This kind of cast even works for null pointers.)

[dureuill]

OK, I'm not sure how that works, but evidently it works (and miri is happy)

https://gist.github.com/rust-play/129b3db72149d293da76662907510f06

Oh, I hadn't read your reply yet (my last reply above was written without yours showing up in my browser).

ahaha same for me, looks like github is having trouble updating the replies dynamically

[steffahn]

I'm not sure how that works, but evidently it works

Of course, in the abstract my previous comment answered that, but I'm curious about the assembly nonetheless (with optimizations on and #[inline(never)] on the fields method):

playground::Foo<dyn playground::A>::fields:
	movq	16(%rdx), %rax
	addq	%rsi, %rax
	movq	%rsi, (%rdi)
	movq	%rax, 8(%rdi)
	movq	%rdx, 16(%rdi)
	retq

my best-guess commentary would be

playground::Foo<dyn playground::A>::fields:
        ; %rdi contains return-pointer that the FooFields return value should be written to
        ; %rsi contains the address-part of the *mut Foo<dyn A>
        ; %rdx contains the vtable-pointer part of the *mut Foo<dyn A>
	movq	16(%rdx), %rax ; read alignment from the vtable into %rax
	                       ; vtable starts with drop_in_place implementation, then size, then alignment,
                               ; so offset 16 is the third entry
	addq	%rsi, %rax ; add address of the whole Foo<dyn A>, this results in the address of field b
	movq	%rsi, (%rdi) ; move address of the whole Foo<dyn A> (equal to address of field a)
                             ; to first word of return value, the field `a` of `FooFields` 
	movq	%rax, 8(%rdi) ; move (calculated) address of field b to second word of return value
	movq	%rdx, 16(%rdi) ; move vtable pointer to third word of return value, these latter two together
                               ; make up the fat pointer `b` field of type `*mut dyn A` in `FooFields<dyn A>` return value
	retq

[dureuill]

Alright, I pushed some new documentation that details the safety preconditions and how we're fulfilling them, that I hope will give us confidence that what we're doing is sound.

I'll merge this in to #11, merge #11, rebase #9, merge #9, and #7 and #8 will be closed at last 👍