-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
struct Rav1dTaskContext::scratch
: Make into zerocopy
-based "unions"
#969
Conversation
4503763
to
92627e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of overarching points:
- Do we need to lazy initialize things? Can we just initialize them where we need to?
- I'm still thinking about this, but these initializations as a different variant are quite expensive given the size of these structs and arrays (up to 250 KB). It might be better to do a
zerocopy
cast to different variants instead. In essence, a safeunion
where the variants are safely transmutable to each other. Then the backing scratch buffer becomes just a[u8; N]
aligned to whatever alignment we need.
1b32838
to
a4a26fa
Compare
@kkysen I've reworked this to use zerocopy to make the scratch data into safe "unions", with most of the structs containing appropriately-sized byte buffers that are then cast to the needed data type using |
6855a15
to
e02d0cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks really good now. I just had a few comments.
src/internal.rs
Outdated
pub struct InterIntraEdgePal; | ||
#[derive(Clone, Copy, FromZeroes, FromBytes, AsBytes)] | ||
#[repr(C, align(32))] | ||
pub struct ScratchEdgeBuf([u8; 257 * 2 + 30]); // 257 Pixel elements + 30 padding bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to return the 30 padding bytes? Provenance-wise, I think that's safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "return the 30 padding bytes"? Return the full size of the buffer, including padding, from buf_mut
? I don't think doing that would be a good idea, the buffer had a specific length for a reason and we shouldn't be changing the size of it. There's also code using this data that assumes a length of 257.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we start with a [u8; 257 * 2 + 30]
. Say we then return a &[u16; 257]
for bpc16, and then asm accesses 30 bytes after that. That violates stacked borrows since it expands provenance. So to do things correctly with no UB, we need to keep that 30 bytes there the whole time. Is it worth it? I'm not sure, but it technically is UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the original code was written to be a buffer of 257 elements without any padding at the end, I think it's reasonably safe to assume that the asm won't read beyond that. If it is, the behavior isn't going to be any different than it was in the original C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the original code was written to be a buffer of 257 elements without any padding at the end, I think it's reasonably safe to assume that the asm won't read beyond that
I'm pretty sure the padding is there since asm is using SIMD instructions that over-read.
If it is, the behavior isn't going to be any different than it was in the original C.
C has different UB rules than Rust, and doing it in Rust with all raw pointers is also different than creating that intermediate slice.
Rav1dTaskContext::scratch
: Make into enumsstruct Rav1dTaskContext::scratch
: Make into zerocopy
-based "enums"
struct Rav1dTaskContext::scratch
: Make into zerocopy
-based "enums"struct Rav1dTaskContext::scratch
: Make into zerocopy
-based "unions"
24d4c69
to
2d44315
Compare
2d44315
to
f048bb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that 30 padding bytes is technically UB, but it's okay to merge. I think we should take a further look at it at some point.
Makes the various scratch
union
s into safe "union"s by usingzerocopy
.