Re: [PATCH 1/2] dtc: Apply deletes in the section which has them

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 14331 bytes --]

On Mon, Oct 26, 2026 at 11:35:19AM -0600, Simon Glass wrote:
> The parser holds each /delete-property/ and /delete-node/ as a
> placeholder entry, flagged as deleted, among the node's properties or
> children. It only takes effect when merge_nodes() merges its section
> into an earlier one. In the first section of a dts it is ignored, so
> 'a; /delete-property/ a;' leaves 'a' in the tree. The same happens in
> any node which a later section adds, since merge_nodes() attaches a new
> child as it is. An ignored /delete-node/ also leaves its placeholder in
> the tree, which then shows up as '(null): Duplicate node name'
> 
> Apply each delete when its node is built, by dropping the earlier
> entries of the same name from that node. Nothing can refer to the node
> yet, so they can be unlinked rather than flagged as deleted. Keep the
> placeholder, so that merging the node into an earlier definition still
> deletes there too.
> 
> When a node joins the tree without being merged, as the first section,
> as a new child in merge_nodes() or as an overlay fragment, drop its
> placeholders, since there is nothing earlier for them to delete. This
> keeps placeholders and entries deleted within a section out of the tree,
> so that nothing which walks the tree has to skip them.
> 
> Duplicate entries within a section are still reported as errors, since
> nothing is merged.
> 
> Closes: https://github.com/dgibson/dtc/issues/37
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Simon Glass <sjg@chromium.org>

I think this is an improvement over the current situation, hence,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

However, there are some semantics that could do with tweaking further
in future.

> diff --git a/dtc-parser.y b/dtc-parser.y
> index e47f80d..0282f7b 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -160,6 +160,7 @@ dt_ref: DT_LABEL_REF | DT_PATH_REF;
>  devicetree:
>  	  '/' nodedef
>  		{
> +			drop_delete_placeholders($2);
>  			$$ = name_node($2, "");
>  		}
>  	| devicetree '/' nodedef
> diff --git a/dtc.h b/dtc.h
> index 473552e..3e5109d 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -277,6 +277,7 @@ struct node *omit_node_if_unused(struct node *node);
>  struct node *reference_node(struct node *node);
>  struct node *chain_node(struct node *first, struct node *list);
>  struct node *merge_nodes(struct node *old_node, struct node *new_node);
> +void drop_delete_placeholders(struct node *node);
>  struct node *add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
>  
>  void add_property(struct node *node, struct property *prop);
> diff --git a/livetree.c b/livetree.c
> index abe73de..b474a05 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -85,6 +85,68 @@ struct property *reverse_properties(struct property *first)
>  	return head;
>  }
>  
> +/*
> + * The parser holds each /delete-property/ and /delete-node/ as a placeholder
> + * entry, flagged as deleted, among the node's properties or children. Apply
> + * each one by dropping the earlier entries of the same name in the node. The
> + * placeholders stay, so that merging the node into an earlier definition
> + * deletes there too
> + */
> +static void apply_deletes(struct node *node)
> +{
> +	struct property *prop, **pp;
> +	struct node *child, **np;
> +
> +	for (prop = node->proplist; prop; prop = prop->next) {
> +		if (!prop->deleted)
> +			continue;
> +		for (pp = &node->proplist; *pp != prop;) {
> +			if (streq((*pp)->name, prop->name))
> +				*pp = (*pp)->next;
> +			else
> +				pp = &(*pp)->next;
> +		}

This leaks, but that's more or less accepted practice in dtc.

> +	}
> +
> +	for (child = node->children; child; child = child->next_sibling) {
> +		if (!child->deleted)
> +			continue;
> +		for (np = &node->children; *np != child;) {
> +			if (streq((*np)->name, child->name))
> +				*np = (*np)->next_sibling;
> +			else
> +				np = &(*np)->next_sibling;
> +		}
> +	}
> +}
> +
> +/*
> + * Drop the /delete-property/ and /delete-node/ placeholders from a subtree
> + * which joins the tree without being merged, since there is nothing earlier

I find this description a bit confusing.  I think the salient point is
that this is called on a subtree that's the first in the list of
things to be merged.

> + * for them to delete
> + */
> +void drop_delete_placeholders(struct node *node)
> +{
> +	struct property **pp = &node->proplist;
> +	struct node **np = &node->children;
> +
> +	while (*pp) {
> +		if ((*pp)->deleted)
> +			*pp = (*pp)->next;
> +		else
> +			pp = &(*pp)->next;
> +	}
> +
> +	while (*np) {
> +		if ((*np)->deleted) {
> +			*np = (*np)->next_sibling;
> +		} else {
> +			drop_delete_placeholders(*np);
> +			np = &(*np)->next_sibling;
> +		}
> +	}
> +}
> +
>  struct node *build_node(struct property *proplist, struct node *children,
>  			struct srcpos *srcpos)
>  {
> @@ -100,6 +162,7 @@ struct node *build_node(struct property *proplist, struct node *children,
>  	for_each_child(new, child) {
>  		child->parent = new;
>  	}
> +	apply_deletes(new);
>  
>  	return new;
>  }
> @@ -212,8 +275,10 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  		}
>  
>  		/* if no collision occurred, add child to the old node. */
> -		if (new_child)
> +		if (new_child) {
> +			drop_delete_placeholders(new_child);

I think this is only correct if merges are always applied left to
right.  I think that's the case, but it's not terribly obvious right
here.  It effectively makes the merge operation non-associative, which
might be confusing.

>  			add_child(old_node, new_child);
> +		}
>  	}
>  
>  	old_node->srcpos = srcpos_extend(old_node->srcpos, new_node->srcpos);
> @@ -245,6 +310,8 @@ struct node * add_orphan_node(struct node *dt, struct node *new_node, char *ref)
>  		p = build_property("target", d, NULL);
>  	}
>  
> +	drop_delete_placeholders(new_node);
> +

Dropping placeholders here is semantically consistent, but possibly
risky.  Accepting a fragment with /delete/s without error might give
the impression that those deletes will take effect when the overlay is
applied, but that's not the case (runtime overlays have no mechanism
for encoding deletes).

I think it would be preferable to give an error if a fragment has any
unapplied delete markers.

>  	xasprintf(&name, "fragment@%u",
>  			next_orphan_fragment++);
>  	name_node(new_node, "__overlay__");
> diff --git a/tests/delete_in_section.dts b/tests/delete_in_section.dts
> new file mode 100644
> index 0000000..e742e79
> --- /dev/null
> +++ b/tests/delete_in_section.dts
> @@ -0,0 +1,125 @@
> +/dts-v1/;
> +
> +/*
> + * Delete properties and nodes defined earlier in the same section. This is
> + * the first section, so nothing is merged into an earlier one
> + */
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	gone-prop;
> +	/delete-property/ gone-prop;
> +	kept-prop;
> +	again-prop = "old";
> +	/delete-property/ again-prop;
> +	again-prop = "new";
> +	both-prop;
> +
> +	/* Deleted and added again here, then changed by a later section */
> +	redel-prop = "old";
> +	/delete-property/ redel-prop;
> +	redel-prop = "new";
> +	reset-prop = "old";
> +	/delete-property/ reset-prop;
> +	reset-prop = "new";
> +
> +	gone-node {
> +	};
> +	/delete-node/ gone-node;
> +
> +	again-node {
> +		old;
> +	};
> +	/delete-node/ again-node;
> +	again-node {
> +		new;
> +	};
> +
> +	lbl: node {
> +		inner-prop;
> +		/delete-property/ inner-prop;
> +
> +		inner-node {
> +		};
> +		/delete-node/ inner-node;
> +	};
> +
> +	redel-node {
> +		old;
> +	};
> +	/delete-node/ redel-node;
> +	redel-node {
> +		new;
> +	};
> +
> +	remerge-node {
> +		old;
> +	};
> +	/delete-node/ remerge-node;
> +	remerge-node {
> +		new;
> +	};
> +
> +	/* Checks must not see what is deleted here */
> +	named-node {
> +		name = "other";
> +		/delete-property/ name;
> +	};
> +
> +	bus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		child@1 {
> +			reg = <1>;
> +		};
> +		/delete-node/ child@1;
> +	};
> +
> +	/* Deleted here, then added again by a later section */
> +	readd@1 {
> +		reg = <1>;
> +		old;
> +	};
> +	/delete-node/ readd@1;
> +	/delete-node/ placeholder@2;

I feel like not throwing an error on deletes of things that don't
exist is a footgun.

> +};
> +
> +/* Delete within nodes which a later section adds, and within the merge */
> +
> +/ {
> +	both-prop = "new";
> +	/delete-property/ both-prop;
> +	/delete-property/ redel-prop;
> +	reset-prop = "later";
> +
> +	new-node {
> +		new-prop;
> +		/delete-property/ new-prop;
> +		kept;
> +	};
> +
> +	/delete-node/ redel-node;
> +
> +	remerge-node {
> +		later;
> +	};
> +
> +	readd@1 {
> +		reg = <1>;
> +		later;
> +	};
> +
> +	placeholder@2 {
> +		reg = <2>;
> +	};
> +};
> +
> +&lbl {
> +	child {
> +		child-prop;
> +		/delete-property/ child-prop;
> +	};
> +};
> diff --git a/tests/delete_in_section_badref.dts b/tests/delete_in_section_badref.dts
> new file mode 100644
> index 0000000..79a40e4
> --- /dev/null
> +++ b/tests/delete_in_section_badref.dts
> @@ -0,0 +1,11 @@
> +/dts-v1/;
> +
> +/* A reference to a node deleted in the same section has nothing to refer to */
> +
> +/ {
> +	ref = <&gone>;
> +
> +	gone: node {
> +	};
> +	/delete-node/ node;
> +};
> diff --git a/tests/delete_in_section_labels.dts b/tests/delete_in_section_labels.dts
> new file mode 100644
> index 0000000..fbde838
> --- /dev/null
> +++ b/tests/delete_in_section_labels.dts
> @@ -0,0 +1,28 @@
> +/dts-v1/;
> +
> +/*
> + * A label moves to the new definition of a node deleted in the same
> + * section, and labels on deleted nodes stay out of __symbols__
> + */
> +
> +/ {
> +	ref = <&moved>;
> +
> +	moved: node {
> +		old;
> +	};
> +	/delete-node/ node;
> +	moved: node {
> +		new;
> +	};
> +
> +	gone: gone-node {
> +	};
> +	/delete-node/ gone-node;
> +};
> +
> +/ {
> +	later: later-node {
> +	};
> +	/delete-node/ later-node;
> +};
> diff --git a/tests/delete_in_section_labels_ref.dts b/tests/delete_in_section_labels_ref.dts
> new file mode 100644
> index 0000000..5d8cf38
> --- /dev/null
> +++ b/tests/delete_in_section_labels_ref.dts
> @@ -0,0 +1,9 @@
> +/dts-v1/;
> +
> +/ {
> +	ref = <&moved>;
> +
> +	moved: node {
> +		new;
> +	};
> +};
> diff --git a/tests/delete_in_section_overlay.dts b/tests/delete_in_section_overlay.dts
> new file mode 100644
> index 0000000..e3e8f71
> --- /dev/null
> +++ b/tests/delete_in_section_overlay.dts
> @@ -0,0 +1,21 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/* Delete within the fragments of an overlay */
> +
> +&target {
> +	a;
> +	/delete-property/ a;
> +	b;
> +	/delete-property/ c;
> +
> +	gone {
> +	};
> +	/delete-node/ gone;
> +	/delete-node/ d;
> +};
> +
> +&{/path} {
> +	p;
> +	/delete-property/ p;
> +};
> diff --git a/tests/delete_in_section_overlay_ref.dts b/tests/delete_in_section_overlay_ref.dts
> new file mode 100644
> index 0000000..48f36b3
> --- /dev/null
> +++ b/tests/delete_in_section_overlay_ref.dts
> @@ -0,0 +1,9 @@
> +/dts-v1/;
> +/plugin/;
> +
> +&target {
> +	b;
> +};
> +
> +&{/path} {
> +};
> diff --git a/tests/delete_in_section_ref.dts b/tests/delete_in_section_ref.dts
> new file mode 100644
> index 0000000..20d459a
> --- /dev/null
> +++ b/tests/delete_in_section_ref.dts
> @@ -0,0 +1,44 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	kept-prop;
> +	again-prop = "new";
> +	reset-prop = "later";
> +
> +	again-node {
> +		new;
> +	};
> +
> +	lbl: node {
> +		child {
> +		};
> +	};
> +
> +	remerge-node {
> +		new;
> +		later;
> +	};
> +
> +	named-node {
> +	};
> +
> +	bus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	new-node {
> +		kept;
> +	};
> +
> +	readd@1 {
> +		reg = <1>;
> +		later;
> +	};
> +
> +	placeholder@2 {
> +		reg = <2>;
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 842b543..2650df6 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -725,6 +725,27 @@ dtc_tests () {
>      run_dtc_test -I dts -O dts -o delete_reinstate_multilabel.dts.test.dts "$SRCDIR/delete_reinstate_multilabel.dts"
>      run_wrap_test cmp delete_reinstate_multilabel.dts.test.dts "$SRCDIR/delete_reinstate_multilabel_ref.dts"
>  
> +    # Check deletes within the section which has them
> +    run_dtc_test -E reg_format -E avoid_unnecessary_addr_size -I dts -O dtb \
> +	-o delete_in_section.test.dtb "$SRCDIR/delete_in_section.dts"
> +    run_dtc_test -I dts -O dtb -o delete_in_section_ref.test.dtb \
> +	"$SRCDIR/delete_in_section_ref.dts"
> +    run_test dtbs_equal_ordered delete_in_section.test.dtb delete_in_section_ref.test.dtb
> +    run_dtc_test -@ -I dts -O dtb -o delete_in_section_labels.test.dtb \
> +	"$SRCDIR/delete_in_section_labels.dts"
> +    run_dtc_test -@ -I dts -O dtb -o delete_in_section_labels_ref.test.dtb \
> +	"$SRCDIR/delete_in_section_labels_ref.dts"
> +    run_test dtbs_equal_ordered delete_in_section_labels.test.dtb \
> +	delete_in_section_labels_ref.test.dtb
> +    run_dtc_test -I dts -O dtb -o delete_in_section_overlay.test.dtb \
> +	"$SRCDIR/delete_in_section_overlay.dts"
> +    run_dtc_test -I dts -O dtb -o delete_in_section_overlay_ref.test.dtb \
> +	"$SRCDIR/delete_in_section_overlay_ref.dts"
> +    run_test dtbs_equal_ordered delete_in_section_overlay.test.dtb \
> +	delete_in_section_overlay_ref.test.dtb
> +    run_sh_test "$SRCDIR/dtc-checkfails.sh" phandle_references \
> +	-n duplicate_node_names -- -I dts -O dtb "$SRCDIR/delete_in_section_badref.dts"
> +
>      # Check some checks
>      check_tests "$SRCDIR/dup-nodename.dts" duplicate_node_names
>      check_tests "$SRCDIR/dup-propname.dts" duplicate_property_names
> -- 
> 2.43.0
> 
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]