feat: stack-based allocator for wdk crate #611
feat: stack-based allocator for wdk crate #611leon-xd wants to merge 19 commits intomicrosoft:mainfrom
wdk crate #611Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new fixed-size, stack-friendly formatting buffer (WdkFormatBuffer) to support heap-free formatting in driver contexts, and integrates it into the kernel print!/println! path by replacing the previous DbgPrintBufWriter implementation.
Changes:
- Added
crates/wdk/src/fmt.rsimplementingWdkFormatBuffer<const T: usize = 512>withfmt::Write, plusas_str()/as_cstr()helpers and tests. - Updated
crates/wdk/src/print.rs(WDM/KMDF path) to format intoWdkFormatBufferand callDbgPrintdirectly; removed the old buffering/flushing module. - Adjusted the KMDF sample driver to add an explicit type annotation for the
WdfDriverCreateoutput handle pointer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| examples/sample-kmdf-driver/src/lib.rs | Tweaks the WdfDriverCreate out-handle variable typing. |
| crates/wdk/src/print.rs | Switches kernel printing to WdkFormatBuffer + direct DbgPrint. |
| crates/wdk/src/lib.rs | Adds the new fmt module and re-exports WdkFormatBuffer. |
| crates/wdk/src/fmt.rs | New formatting buffer type, CStr/str views, and unit tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
=======================================
Coverage 77.41% 77.41%
=======================================
Files 24 24
Lines 4853 4853
Branches 4853 4853
=======================================
Hits 3757 3757
Misses 979 979
Partials 117 117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wdk crate
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8050685 to
74a4e88
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@wmmc88 Thought about this for a bit and realized I should just prevent the breaking change if possible. So added a new type, |
wdk crate wdk crate
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mod fmt; | ||
|
|
||
| #[cfg(any( | ||
| driver_model__driver_type = "WDM", | ||
| driver_model__driver_type = "KMDF", | ||
| driver_model__driver_type = "UMDF" | ||
| ))] | ||
| pub use fmt::{WdkFlushableFormatBuffer, WdkFormatBuffer}; |
There was a problem hiding this comment.
This module is declared as mod fmt; (private), so external users cannot access the wdk::fmt::... path described in the PR (and earlier discussion). If the intended public API is wdk::fmt::WdkFormatBuffer, make this pub mod fmt (or re-export a pub mod fmt { pub use super::fmt::*; }). Otherwise, update the PR description/docs to reflect the actual public path (wdk::WdkFormatBuffer).
There was a problem hiding this comment.
I think I'm comfortable just exporting it as wdk::WdkFormatBuffer and wdk::WdkFlushableFormatBufer. not sure if anyone else has any opinions
There was a problem hiding this comment.
i agree with copilot. should reduce the duplication and its typically convention to do stuff like wdk::FormatBuffer
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Leon Durrenberger <leon.durrenberger@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl<F: FnMut(&WdkFormatBuffer<N>), const N: usize> fmt::Write for WdkFlushableFormatBuffer<F, N> { | ||
| fn write_str(&mut self, s: &str) -> fmt::Result { | ||
| let capacity = N - 1; | ||
| let mut remaining = &s.as_bytes()[..]; | ||
|
|
||
| // Fill what fits, flush, continue with the rest. | ||
| while remaining.len() > capacity - self.format_buffer.used { | ||
| let space = capacity - self.format_buffer.used; | ||
| self.format_buffer.buffer[self.format_buffer.used..self.format_buffer.used + space] | ||
| .copy_from_slice(&remaining[..space]); | ||
| self.format_buffer.used = capacity; | ||
|
|
||
| (self.flush_fn)(&self.format_buffer); | ||
| self.format_buffer.reset(); | ||
|
|
||
| remaining = &remaining[space..]; | ||
| } | ||
|
|
||
| // Remaining bytes fit in the buffer. | ||
| self.format_buffer.buffer | ||
| [self.format_buffer.used..self.format_buffer.used + remaining.len()] | ||
| .copy_from_slice(remaining); | ||
| self.format_buffer.used += remaining.len(); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
WdkFlushableFormatBuffer::write_str splits the incoming &str by raw bytes when chunking, which can cut through a multi-byte UTF-8 codepoint. That makes WdkFormatBuffer::as_str() potentially return Err(Utf8Error) for a flushed chunk even though the original formatted output was valid UTF-8. Either document this explicitly (recommended, since as_str is fallible) or change the chunking logic to split on UTF-8 boundaries when the flush consumer is expected to treat chunks as &str.
There was a problem hiding this comment.
@leon-xd Copilot is right. We should end at a clean char boundary not a byte boundary. as_str returning an error is not the only problem. A bigger problem would be DbgPrint emitting garbled output if the last char gets cut in the middle.
We should also add a test for this.
There was a problem hiding this comment.
yeah, i think this should be chunking off of graphemes
There was a problem hiding this comment.
Ideally it should be graphemes not chars, but that will require pulling in the unicode_segmentation crate and might increase binary size a bit as well.
| impl<F: FnMut(&WdkFormatBuffer<N>), const N: usize> fmt::Write for WdkFlushableFormatBuffer<F, N> { | ||
| fn write_str(&mut self, s: &str) -> fmt::Result { | ||
| let capacity = N - 1; | ||
| let mut remaining = &s.as_bytes()[..]; | ||
|
|
||
| // Fill what fits, flush, continue with the rest. | ||
| while remaining.len() > capacity - self.format_buffer.used { | ||
| let space = capacity - self.format_buffer.used; | ||
| self.format_buffer.buffer[self.format_buffer.used..self.format_buffer.used + space] | ||
| .copy_from_slice(&remaining[..space]); | ||
| self.format_buffer.used = capacity; | ||
|
|
||
| (self.flush_fn)(&self.format_buffer); | ||
| self.format_buffer.reset(); | ||
|
|
||
| remaining = &remaining[space..]; | ||
| } | ||
|
|
||
| // Remaining bytes fit in the buffer. | ||
| self.format_buffer.buffer | ||
| [self.format_buffer.used..self.format_buffer.used + remaining.len()] | ||
| .copy_from_slice(remaining); | ||
| self.format_buffer.used += remaining.len(); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
@leon-xd Copilot is right. We should end at a clean char boundary not a byte boundary. as_str returning an error is not the only problem. A bigger problem would be DbgPrint emitting garbled output if the last char gets cut in the middle.
We should also add a test for this.
crates/wdk/src/fmt.rs
Outdated
|
|
||
| /// Resets the buffer to its initial empty state. | ||
| pub fn reset(&mut self) { | ||
| self.buffer = [0; N]; |
There was a problem hiding this comment.
open question: should this even bother to zero out the buffer since the buffer is private?
There was a problem hiding this comment.
after a flush the format buffer calls reset. i'd like to keep it if possible?
There was a problem hiding this comment.
i see -- nothing gets beyond used so it doesn't really matter what's in the rest of the buffer. removed.
| /// | ||
| /// The buffer always contains a NUL terminator because `write_str` | ||
| /// reserves the last byte. | ||
| pub const fn as_cstr(&self) -> &CStr { |
There was a problem hiding this comment.
what happens when we have internal null since that's valid in rust string?
There was a problem hiding this comment.
We could skip any embedded nulls in the incoming string when writing to the buffer. Or replace them with ?s.
There was a problem hiding this comment.
it writes until the first null, I believe this is documented
| impl<F: FnMut(&WdkFormatBuffer<N>), const N: usize> fmt::Write for WdkFlushableFormatBuffer<F, N> { | ||
| fn write_str(&mut self, s: &str) -> fmt::Result { | ||
| let capacity = N - 1; | ||
| let mut remaining = &s.as_bytes()[..]; | ||
|
|
||
| // Fill what fits, flush, continue with the rest. | ||
| while remaining.len() > capacity - self.format_buffer.used { | ||
| let space = capacity - self.format_buffer.used; | ||
| self.format_buffer.buffer[self.format_buffer.used..self.format_buffer.used + space] | ||
| .copy_from_slice(&remaining[..space]); | ||
| self.format_buffer.used = capacity; | ||
|
|
||
| (self.flush_fn)(&self.format_buffer); | ||
| self.format_buffer.reset(); | ||
|
|
||
| remaining = &remaining[space..]; | ||
| } | ||
|
|
||
| // Remaining bytes fit in the buffer. | ||
| self.format_buffer.buffer | ||
| [self.format_buffer.used..self.format_buffer.used + remaining.len()] | ||
| .copy_from_slice(remaining); | ||
| self.format_buffer.used += remaining.len(); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
yeah, i think this should be chunking off of graphemes
crates/wdk/src/fmt.rs
Outdated
| /// # Errors | ||
| /// Returns an error if the written bytes are not valid UTF-8. | ||
| pub fn as_str(&self) -> Result<&str, Utf8Error> { | ||
| core::str::from_utf8(&self.buffer[..self.used]) |
There was a problem hiding this comment.
this is guaranteed to always succeed
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub const fn new() -> Self { | ||
| const { | ||
| assert!( | ||
| N >= 2, | ||
| "N must be at least 2 (one byte of content plus the NUL terminator)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
The PR description mentions support/tests for zero-sized buffers and as_cstr() returning an error when a terminator is missing (e.g., N == 0), but the implementation now enforces N >= 2 at compile time and as_cstr() panics on the error path. Please update the PR description (or relax the N >= 2 constraint / make as_cstr fallible) so the documented behavior matches the code.
| /// Interior NUL bytes in the formatted output will cause the string to be | ||
| /// truncated at the first NUL. |
There was a problem hiding this comment.
_print's doc comment says interior NUL bytes will truncate output at the first NUL, but the UMDF implementation explicitly splits on the first NUL, prints the prefix, then recursively prints the remaining content (i.e., it strips NULs rather than truncating). Please update this doc comment to reflect the UMDF behavior (or make the implementation match the truncation behavior).
| /// Interior NUL bytes in the formatted output will cause the string to be | |
| /// truncated at the first NUL. | |
| /// Interior NUL bytes in the formatted output are handled differently depending | |
| /// on the driver model: | |
| /// - In WDM/KMDF builds, each chunk is passed to `DbgPrint` as a C string, so the | |
| /// output is effectively truncated at the first NUL byte in each chunk. | |
| /// - In UMDF builds, the implementation splits on NUL bytes and logs each | |
| /// NUL-separated segment in sequence, effectively stripping the NUL bytes from | |
| /// the output. |
| #[cfg(any( | ||
| driver_model__driver_type = "WDM", | ||
| driver_model__driver_type = "KMDF", | ||
| driver_model__driver_type = "UMDF" | ||
| ))] | ||
| mod fmt; | ||
|
|
||
| #[cfg(any( | ||
| driver_model__driver_type = "WDM", | ||
| driver_model__driver_type = "KMDF", | ||
| driver_model__driver_type = "UMDF" | ||
| ))] | ||
| pub use fmt::{WdkFlushableFormatBuffer, WdkFormatBuffer}; |
There was a problem hiding this comment.
do these need to be gated?
Adds and integrates
wdk::fmt::WdkFormatBuffer.Summary
crates/wdk/src/fmt.rswith a fixed-size, stack-friendly formatting buffer (WdkFormatBuffer<T>) implementingfmt::Write.core::ffi::CStrand&stralongside tests to ensure correctness.WdkFormatBufferfromwdk::fmtvialib.rsfor WDM/KMDF/UMDF driver models.print.rskernel path to useWdkFormatBufferinstead ofDbgPrintBufWriter; removed that module and its tests.Details
WdkFormatBuffer<T = 512>stores a zero-initialized[u8; T]and tracksusedbytes.Defaultdelegates tonew()for convenience.as_str()returns a UTF-8 view over written bytes only.as_cstr()ensures a trailing NUL when the buffer is filled; returnsFromBytesUntilNulErroron missing terminator (e.g.,T == 0).fmt::Writeimplementation clamps writes to capacity and signals truncation viafmt::Error.print.rskernel path now formats intoWdkFormatBufferand callsDbgPrintdirectly using the resultingCStr. The previousdbg_print_buf_writermodule and its flush logic/tests were removed. This results in different behavior when handling overflowing buffers.as_str()/as_cstr()usageRationale
wdk::fmt(andpub use fmt::WdkFormatBuffer) removes dead-code warnings and makes the type available to consumers.Testing
Notes / Follow-ups
fmt::writeerrors and silently return onas_cstrfailure. A future improvement could be a chunked/flush API onWdkFormatBufferto mimic prior semantics.