Reduce uses of uninitialized arrays #3261
Conversation
kornelski
force-pushed
the
init-me-maybe
branch
3 times, most recently
from
3aecd94
to
39380df
Compare
|
CI fails due to #3263 |
|
Any comments for this PR? Is the direction good? Can it be merged despite the CI issue? |
|
Lots of fixes should land since they are good. I hope we'll fix the dav1d situation so we can have back the tests :/ |
kornelski
force-pushed
the
init-me-maybe
branch
from
39380df
to
36da899
Compare
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
src/asm/shared/transform/inverse.rs
Outdated
There was a problem hiding this comment.
If all the changes in this PR were like this file, I would approve it in a heartbeat.
This is the pattern we should be following to avoid undefined behavior and double initialization.
There was a problem hiding this comment.
I don't understand the comment. Does that mean other files than inverse.rs are not acceptable? What do I need to change?
There was a problem hiding this comment.
BTW, I've wanted to use a pattern of:
let initialized = function(&mut maybe_uninit);but that required reliably tracking the length inside the function, which would regress performance. This lead me to try some loop optimizations like #3262 but I found it wasn't always possible to have guaranteed length for free. Plus there's A LOT of uninitialized to deal with.
There are more functions using which I haven't converted yet, because I'm not actually sure they fully initialize their data!
There was a problem hiding this comment.
Firstly, thank you for detailing the current sites of potential undefined behavior. Everything that maintains the current intent can be merged. Any changes that replace uninitialized() with zero-fill should be dropped and can be addressed in a follow-up PR.
BTW, I've wanted to use a pattern of:
let initialized = function(&mut maybe_uninit);
I have recently tackled this issue in a toy project:
https://github.com/barrbrain/sudoku/blob/main/src/lib.rs#L27-L40
Some care is required with ownership semantics when crossing the unsafe boundary. This pattern is well defined:
https://github.com/barrbrain/sudoku/blob/main/src/lib.rs#L355-L356
I will see if we can add some infrastructure to ease converting the current code to well-defined.
There was a problem hiding this comment.
Once MaybeUninit::uninit_array hits stable I guess a lot would be easier.
Probably we could change some Vec in Box<[]> to make more evident that the size is fixed meanwhile.
(also, thank you again for taking care of this :))
There was a problem hiding this comment.
The only cases of zero-fill (I assume you mean from_fn(|| 0)) are in benchmarks, outside of the benchmarked section. I assume they're not important, since they're neither in the library, nor performance-critical. So I've just changed them for simplicity and determinism.
I'm trying to remove UB of
Aligned::uninitialized()— #1572