feat(fdt): add FdtNode::events and Fdt::events for single-pass traversal by calef · Pull Request #108 · google/dtoolkit

calef · 2026-11-08T03:09:21Z

Adds single-pass traversal of the structure block.

FdtNode::events yields the events of a node's subtree, from its BeginNode to its matching EndNode.
Fdt::events is a shorthand for root().events().
The iterator tracks the depth in O(1), and each BeginNode and EndNode carries the depth of its node, counted from the node the iterator started at (0 for that node). The depth also catches an FDT_END inside a node, an FDT_END_NODE before the first node and an FDT_PROP outside any node: Fdt::new already rejects all three, and on an unchecked FDT the iterator panics like the other methods.
The DTB builders used by the tests live in tests/common/mod.rs.
Under Miri the large trees use 256 nodes and the recursive comparison uses 32.
The fuzz parse target walks events() too.

Visiting every node through `children()` takes time quadratic in the depth of the tree, because finding the next sibling of a node walks the node's entire subtree. This can't be fixed inside `children()` without extra memory. This adds `Fdt::events`, which returns an iterator over the structure block in its flat order: `FdtEvent::BeginNode` (with the node's name and its offset relative to `dt_struct`), `FdtEvent::Property`, and `FdtEvent::EndNode`. `FDT_NOP` tokens are skipped. A full traversal is a single linear pass and doesn't allocate. Fixes google#102

codecov · 2026-11-08T20:04:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.91%. Comparing base (7662cd1) to head (4730681).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   93.61%   93.91%   +0.30%     
==========================================
  Files          26       27       +1     
  Lines        3428     3503      +75     
==========================================
+ Hits         3209     3290      +81     
+ Misses        219      213       -6

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

m4tx

This looks good to me overall; I've added a few comments about some minor issues.

This is definitely a step in a good direction and as mentioned in the comment, I think it would be logical to replace some of the existing code (such as FdtNode::children()) with a wrapper on top of events(). We have some duplicated parsing logic and it would be good to unify it, especially considering this is arguably the most critical part of the library.

Regarding the discussion points raised:

BeginNode carries the name and the offset, not an FdtNode

I think it's okay; changing this would either require removing parent_address_space from FdtNode (which would trigger a bigger refactor around the Node trait and the standard module), or quietly supplying an invalid value of said field into an FdtNode instance, which isn't very Rusty (or developer-friendly, for that matter).

I think it would be reasonable to provide a higher-level iterator that would return FdtNodes in depth-first order. This would obviously require storing parent_address_spaces for the entire ancestor chain; to keep Fdt API alloc-free, we could solve this by using a fixed sized array by assuming some maximum depth for practical device trees (e.g. 64, configurable via a const generic). This is definitely a topic for another PR, though.

The enum isn't #[non_exhaustive]

The enum itself probably won't get more variants; the question is whether individual variants should be marked #[non_exhaustive]. I think it's not very likely for them to be changed, and at this stage of the project it's still fine to make some API breakages, so I'd say let's leave this as is.

I didn't add a benchmark, since the repo doesn't have a benchmark setup; I can add one if you'd like.

I trust that this is much faster on very deep device trees than children(). If you'd like to add some benchmarks, I'll be happy to see them in a separate PR. I had some plans to add some benchmarking to the library anyway, so it will be nice to start with something.

I guess the next steps would be to (in no particular order and in separate PRs):

Replace existing bespoke parsing logic in the fdt module with events(), wherever it makes sense (FdtNode::children() is definitely a good candidate)
Add .descendants() method to FdtNode that would be a higher-level wrapper over events() which would return FdtNodes directly at the cost of some extra memory stored. (to be confirmed: whether there will be users for this, and whether the fixed array approach makes sense)
Fix #103 with this.

Feel free to tackle any of this if you're interested; I'll create GH issues to track these.

m4tx · 2026-11-08T20:19:06Z

+/// Visiting a 100,000-deep chain through `children()` takes minutes, while
+/// `events()` takes milliseconds. Miri is much slower, so it uses a smaller
+/// tree.
+const LARGE_TREE_NODES: usize = if cfg!(miri) { 1_000 } else { 100_000 };


Can we reduce this even further for Miri? The test runtime in CI has increased from ~2 minutes to about 10. The test still will run and I'm doubtful we have any meaningful chance of hitting an undefined behaviour just by increasing the number of nodes.

Thank you, done. Under Miri the two large-tree tests now use 256 nodes (7ae5b0f), and the recursive comparison uses 32 nodes (9e9b4ac). The native run keeps 100,000 nodes, so the linear-time check is unchanged. If this is still too slow, the per-node comparison in events_match_recursive_traversal could be skipped under Miri.

m4tx · 2026-11-08T20:30:36Z

+    /// assert_eq!(depth, 0);
+    /// ```
+    #[must_use]
+    pub fn events(self) -> FdtEventIter<'a> {


Would it make sense for this method to be defined on FdtNode instead? This way, it would be fairly easy to implement other methods (e.g. children()) on top of it, reducing code duplication in a frequently used part of the parser.

The only downside I see is that we'd need to keep track of the current depth in order for this to work, but this is O(1) additional memory, so I think it's fine. Another bonus when tracking depth is that we could detect invalidly placed FdtToken::End.

Good idea, done in e709f30. events() is now defined on FdtNode and yields the events of that node and its descendants, and Fdt::events stays as a shorthand for root().events(). The iterator counts open nodes (O(1) memory), so an FDT_END inside a node is now detected. Fdt::new already rejects it, and on an unchecked FDT it panics like the other methods. Tests cover both.

m4tx · 2026-11-09T02:12:38Z

+/// Builds a version 17 FDT blob from a structure block and a strings block.
+fn build_dtb(structure: &[u8], strings: &[u8]) -> Vec<u8> {
+    // The header is followed by an empty memory reservation block (just the
+    // terminator), then by the structure block and the strings block.
+    let header_size = 40;
+    let off_mem_rsvmap = header_size;
+    let off_dt_struct = off_mem_rsvmap + 16;
+    let off_dt_strings = off_dt_struct + structure.len();
+    let totalsize = off_dt_strings + strings.len();
+
+    let mut dtb = Vec::with_capacity(totalsize);
+    for value in [
+        0xd00d_feed,
+        totalsize,
+        off_dt_struct,
+        off_dt_strings,
+        off_mem_rsvmap,
+        17,
+        16,
+        0,
+        strings.len(),
+        structure.len(),
+    ] {
+        dtb.extend_from_slice(
+            &u32::try_from(value)
+                .expect("DTB header fields should fit in u32")
+                .to_be_bytes(),
+        );
+    }
+    dtb.extend_from_slice(&[0; 16]);
+    dtb.extend_from_slice(structure);
+    dtb.extend_from_slice(strings);
+    dtb
+}
+
+const FDT_BEGIN_NODE: u32 = 0x1;
+const FDT_END_NODE: u32 = 0x2;
+const FDT_PROP: u32 = 0x3;
+const FDT_NOP: u32 = 0x4;
+const FDT_END: u32 = 0x9;
+
+fn push_token(structure: &mut Vec<u8>, token: u32) {
+    structure.extend_from_slice(&token.to_be_bytes());
+}
+
+fn push_begin_node(structure: &mut Vec<u8>, name: &str) {
+    push_token(structure, FDT_BEGIN_NODE);
+    structure.extend_from_slice(name.as_bytes());
+    structure.push(0);
+    structure.resize(structure.len().next_multiple_of(4), 0);
+}


Can we move this part to a new file in tests/?

It's nice to have some helper functions to build test DTBs - even better, they do not share any code with the actual library that we're testing. However, the file is already pretty long, so I'd suggest focusing on the actual test code here, and low-level-code-to-build-test-assets somewhere else.

Done in 8515ef7. The DTB-building helpers now live in tests/common/mod.rs, which does not use any code of the library, so tests/fdt.rs holds only the tests.

The helpers that write test DTBs byte by byte are not tests themselves, and tests/fdt.rs is already long. They now live in tests/common/mod.rs, which does not use any code of the library, so that tests/fdt.rs holds only the tests.

`events()` is now defined on `FdtNode`, and yields the events of that node and its descendants: it starts with the node's `BeginNode` and ends with its matching `EndNode`. This lets other traversals, such as `children()`, be built on top of it later. `Fdt::events` stays as a shorthand for `fdt.root().events()`. To know where the node ends, the iterator counts the nodes that have started and not yet ended. This is O(1) memory, and it also detects an `FDT_END` token placed inside a node, which the previous version took as the end of the tree. As with the other methods on an FDT built with `Fdt::new_unchecked`, such an FDT causes a panic; `Fdt::new` already rejects it.

The two large-tree tests took most of the Miri run with 1,000 nodes. Miri looks for undefined behaviour, which does not depend on the number of nodes, since every node goes through the same code; a few hundred nodes is still far beyond the depth and width of real device trees. The native run keeps 100,000 nodes, which is what shows that the traversal is linear.

…heck Comparing `events()` with a recursive traversal through `children()` takes time quadratic in the depth of the chain, and with 100 nodes it took about a minute and a half under Miri. 32 nodes still compare the two traversals on a deep and a wide tree.

The parse target checks that no operation on a tree accepted by `Fdt::new` panics. `events()` reads the structure block through its own path, so the target now walks it as well.

calef · 2026-11-09T20:13:13Z

Thanks for the review. Pushed four follow-up commits:

events() now lives on FdtNode and yields the events of that node's subtree, from its BeginNode to its matching EndNode; Fdt::events stays as a shorthand for root().events(). The iterator tracks the depth (O(1)), which also catches an FDT_END inside a node: Fdt::new already rejects it, and on an unchecked FDT it panics like the other methods. Tests cover both.
The DTB builders moved to tests/common/mod.rs.
Under Miri the large trees use 256 nodes and the recursive comparison 32. On my machine the events tests are about 85 s of the ~7 min Miri run of tests/fdt.rs. If you would like it lower still, the per-node node.events() comparison in events_match_recursive_traversal is now the largest piece and could be skipped under Miri.
The fuzz parse target walks events() too.

I have updated the description to match. Happy to take the children()-on-events() refactor and #103 as separate PRs.

m4tx

Thanks for the updates; this looks much better now. I left a few more comments; when these are addressed, I think we'll be able to merge this.

m4tx · 2026-11-10T12:41:01Z

+    pub(crate) fdt: Fdt<'a>,
+    /// The offset of the next token to read, relative to the start of the FDT,
+    /// or `None` once the node the iterator started at has ended.
+    pub(crate) offset: Option<usize>,


This is sort of a microopt here, but could we make this a bit more Rusty and use a similar approach to InnerChildIter, i.e. make this an enum with variants Running and Stopped, and change this offset to store usize directly? Not only this will be more idiomatic IMO, but most importantly, we'd be able to save size_of<usize> bytes on this struct, as the compiler would be able to store the Stopped variant as a niche value of fdt. See here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=f9397b52534fc1b7e2a5ea42b0e30f72

That would probably require splitting this into a private enum and a public wrapper over it to avoid leaking the implementation details (again, similarly to InnerChildIter), but I think it's worth it.

Done in 4a9bbb5. FdtEventIter now wraps a private InnerEventIter with Running { fdt, offset, depth } and Stopped, as FdtChildIter does with InnerChildIter. size_of::<FdtEventIter>() goes from 40 to 32 bytes on a 64-bit target, with Stopped in the niche of the slice reference.

m4tx · 2026-11-10T12:51:14Z

+                    // The iterator starts at an `FDT_BEGIN_NODE` token and
+                    // stops once the depth is back to zero, so the depth is
+                    // never zero here.
+                    self.depth -= 1;


What if the Fdt is constructed with new_unchecked and the first token is EndNode? I believe we'd underflow the depth which would cause the parser to misbehave (on release builds).

I think we need to use checked_sub here and panic!("Fdt should be valid") if this underflows.

Good catch, thank you. Done in 1a300d0: the EndNode arm now uses checked_sub and panics with the usual message. Without it a release build wrapped the depth and kept reading past the node; Fdt::new rejects that shape (BadToken(FDT_BEGIN_NODE)), so only the unchecked constructors reached it. end_node_first_is_rejected and events_panic_on_end_node_first cover both paths.

m4tx · 2026-11-10T12:52:34Z

+        /// It can be passed to [`Fdt::node_at_offset`] to get the
+        /// corresponding [`FdtNode`].
+        offset: usize,
+    },


If we need to keep track of depth now anyway, maybe it's worth adding it to BeginNode and EndNode?

Done in 4730681. BeginNode and EndNode now carry depth, counted from the node the iterator started at: 0 for that node, so Fdt::events gives the depth below the root and node.events() the depth below node. A node's BeginNode and EndNode carry the same value. The size of FdtEvent is unchanged (the Property variant sets it), the Fdt::events example no longer counts by hand, the FdtNode::events example shows the depth starting at 0 for /a/b, and the tests check the field against a recursive traversal.

m4tx · 2026-11-10T13:13:43Z

+                        offset: offset - self.fdt.header().off_dt_struct() as usize,
+                    });
+                }
+                FdtToken::Prop => {


Similarly to the EndNode case, maybe we should also check inside the Prop case whether depth == 0 and panic if so? This makes the parser fully robust to any adversary content I believe (even if new_unchecked is used).

Agreed. Done in b4edc0a: the Prop arm asserts that a node is open. Before this, an unchecked tree whose structure block started with FDT_PROP yielded the property and then the root with no panic in either profile. property_before_root_is_rejected and events_panic_on_property_before_root cover Fdt::new and new_unchecked. With this and the EndNode check, every sequence the iterator yields is well nested: it begins with a BeginNode or panics, and it stops when that node ends.

An FDT built with `Fdt::new_unchecked` can have an `FDT_END_NODE` token before any `FDT_BEGIN_NODE`. The iterator then subtracted one from a depth of zero: a debug build panicked with an overflow message, and a release build wrapped the depth to `usize::MAX` and kept reading past the node it was asked about. The subtraction is now checked and panics with the same message as the other checks on an invalid FDT. `Fdt::new` already rejects such a tree.

An FDT built with `Fdt::new_unchecked` can have an `FDT_PROP` token before the first `FDT_BEGIN_NODE`. The iterator yielded it as a property that belongs to no node, then went on to the root without a panic. Every event now belongs to a node: a property at depth zero panics with the same message as the other checks on an invalid FDT, as `children()` already does for a misplaced property. `Fdt::new` already rejects such a tree.

The iterator kept the offset as an `Option<usize>`, with `None` once the node it started at had ended. It now wraps a private `InnerEventIter` with a `Running` variant, which holds the FDT, a plain offset and the depth, and a `Stopped` variant, as `FdtChildIter` wraps `InnerChildIter`. The compiler stores `Stopped` in the niche of the FDT's slice reference, so the iterator is one `usize` smaller: 32 bytes instead of 40 on a 64-bit target.

The iterator already counts the nodes that have started and not yet ended, to know where the node it started at ends, and consumers that need the depth (both examples, an indented printer) had to count it again themselves. `FdtEvent::BeginNode` and `FdtEvent::EndNode` now carry `depth`, counted from the node the iterator started at: 0 for that node, so `Fdt::events` gives the depth below the root and `FdtNode::events` the depth below that node. An `FdtNode` does not know its own depth in the tree, so the depth cannot be absolute without growing every `FdtNode`. A node's `BeginNode` and `EndNode` carry the same depth. The size of `FdtEvent` does not change, since the `Property` variant is the largest.

calef · 2026-11-11T01:58:54Z

Thanks for the second look. Pushed four commits:

1a300d0 and b4edc0a: EndNode uses checked_sub and Prop asserts an open node, both panicking with the usual message on an unchecked tree; Fdt::new already rejected both shapes. Four tests cover the checked and unchecked paths.
4a9bbb5: FdtEventIter wraps a private Running/Stopped enum, like FdtChildIter; 40 -> 32 bytes.
4730681: BeginNode and EndNode carry depth, counted from the node the iterator started at. Docs and tests follow; the Fdt::events example no longer counts depth by hand.

On the earlier Miri thread: the job on this head runs in about 4 minutes, under the ~6 minutes on main, so I have left the test sizes as they are.

Description updated to match.

m4tx reviewed Nov 9, 2026

View reviewed changes

m4tx requested a review from qwandor November 9, 2026 02:15

calef added 5 commits November 9, 2026 02:38

test(fuzz): walk the events of every accepted tree in the parse target

fc5563d

The parse target checks that no operation on a tree accepted by `Fdt::new` panics. `events()` reads the structure block through its own path, so the target now walks it as well.

calef changed the title ~~feat(fdt): add Fdt::events for single-pass traversal~~ feat(fdt): add FdtNode::events and Fdt::events for single-pass traversal Nov 9, 2026

m4tx requested changes Nov 10, 2026

View reviewed changes

calef added 4 commits November 10, 2026 19:47

Conversation

calef commented Nov 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

m4tx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calef commented Nov 9, 2026

Uh oh!

m4tx left a comment

Choose a reason for hiding this comment

Uh oh!

m4tx Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calef commented Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

calef commented Nov 8, 2026 •

edited

Loading

codecov Bot commented Nov 8, 2026 •

edited

Loading

m4tx left a comment •

edited

Loading

m4tx Nov 10, 2026 •

edited

Loading

calef commented Nov 11, 2026 •

edited

Loading