Re: [PATCH v3 01/18] tests: Check how the fragments of a dts file are merged

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

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

On Fri, Nov 06, 2026 at 12:51:28PM -0700, Simon Glass wrote:
> When the parser merges the top-level fragments of a dts file, it can
> report that a label or path is not found, or that a label-relative
> reference is not supported in a plugin. Most of these errors are not
> covered by any test, and the two tests which do reach them only check
> that dtc fails, not the message or where it is reported. The merge
> logic is about to move out of the grammar, so this needs better tests.
> 
> Add a test file for each error which is not covered, and check the
> message and its position for all of them, including the two existing
> ones. Also check that merging carries on after an error, so that later
> errors are reported too, that a reference error is still reported when
> the parser finds another error, that a labelled reference within a
> plugin adds the label, that / { ... } and &{/} { ... } differ in a
> plugin, and that a label in a plugin becomes an overlay fragment if it
> is not found or is written as &{label}, which applies after any local
> merges, since nothing covers these either. Also check that a node in an
> overlay fragment can be deleted by its label, and where the overlay
> fragment goes among the plugin's own nodes.
> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Simon Glass <sjg@chromium.org>

Adding tests for this is very valuable.  Few concerns about some of
the specific things tested, see below.


[snip]
> diff --git a/tests/merge-errors.dts b/tests/merge-errors.dts
> new file mode 100644
> index 0000000..1e97088
> --- /dev/null
> +++ b/tests/merge-errors.dts
> @@ -0,0 +1,14 @@
> +/dts-v1/;
> +
> +/ {
> +	label: node {
> +	};
> +};
> +
> +/* All three of the references below are bad, and each one is reported */

Which three references?  I only see one.

> +&nosuchnode1 {
> +	prop;
> +};
> +
> +/delete-node/ &nosuchnode2;
> +/omit-if-no-ref/ &nosuchnode3;
> diff --git a/tests/merge-first-ref-relative.dts b/tests/merge-first-ref-relative.dts
> new file mode 100644
> index 0000000..eece7a7
> --- /dev/null
> +++ b/tests/merge-first-ref-relative.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/* A plugin cannot define a node using a label-relative reference */
> +&{label/node} {
> +	prop;
> +};
> +
> +/* Later fragments are still merged */

Not sure this tests anything useful, since the compile will fail
overall.

> +/delete-node/ &nosuchnode;
> diff --git a/tests/merge-first-ref.dts b/tests/merge-first-ref.dts
> new file mode 100644
> index 0000000..8f23a44
> --- /dev/null
> +++ b/tests/merge-first-ref.dts
> @@ -0,0 +1,9 @@
> +/dts-v1/;
> +
> +/* Only a plugin can start with a reference */
> +&nosuchnode {
> +	prop;
> +};
> +
> +/* Later fragments are still merged */

Again, not sure what this is really testing.

> +/delete-node/ &nosuchnode2;
> diff --git a/tests/merge-label-base.dts b/tests/merge-label-base.dts
> new file mode 100644
> index 0000000..1a0a8bb
> --- /dev/null
> +++ b/tests/merge-label-base.dts
> @@ -0,0 +1,10 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +};
> +
> +/* A label which is not in the plugin may be in the base tree */
> +&{baselabel} {
> +	prop;
> +};
> diff --git a/tests/merge-label-braces.dts b/tests/merge-label-braces.dts
> new file mode 100644
> index 0000000..d5f8d0f
> --- /dev/null
> +++ b/tests/merge-label-braces.dts
> @@ -0,0 +1,12 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	label: node {
> +	};
> +};
> +
> +/* This becomes an overlay fragment, even though the label is in the plugin */
> +&{label} {

I'm not sure if this is testing behaviour we actually want to
enshrine.  My first thought was that &{label} should be illegal (the
path must be absolute.  But then I realised this does need to be
valid, as a degenerate case of a label-relative path.

That may currently have the side effect of forcing this to be an
orphan node, but I think that's an accident of implementation rather
than an intentional policy.

> +	prop;
> +};
> diff --git a/tests/merge-label-order.dts b/tests/merge-label-order.dts
> new file mode 100644
> index 0000000..f7923a0
> --- /dev/null
> +++ b/tests/merge-label-order.dts
> @@ -0,0 +1,17 @@
> +/dts-v1/;
> +/plugin/;
> +
> +&basenode {
> +	label: foo {
> +		p = "first";
> +	};
> +};
> +
> +/* The fragment for &{label} is applied last, so "second" wins */
> +&{label} {
> +	p = "second";
> +};
> +
> +&label {
> +	p = "third";
> +};

In this derived case, that existing behaviour just seems like a bug to
me.

> diff --git a/tests/merge-labelled-plugin.dts b/tests/merge-labelled-plugin.dts
> new file mode 100644
> index 0000000..3849cc5
> --- /dev/null
> +++ b/tests/merge-labelled-plugin.dts
> @@ -0,0 +1,12 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	label: node {
> +	};
> +};
> +
> +/* Add a label to a node defined in the plugin itself */
> +newlabel: &label {
> +	prop = "value";
> +};
> diff --git a/tests/merge-labelled-relative.dts b/tests/merge-labelled-relative.dts
> new file mode 100644
> index 0000000..9bedfd9
> --- /dev/null
> +++ b/tests/merge-labelled-relative.dts
> @@ -0,0 +1,14 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	label: node {
> +		subnode {
> +		};
> +	};
> +};
> +
> +/* Not allowed even though the reference is found */

Again, I'm not sure this is behaviour we want to enshrine in a test.
Since we can resolve this locally it would be reasonable to extend the
code so that it does.

> +newlabel: &{label/subnode} {
> +	prop;
> +};
> diff --git a/tests/merge-omit-missing.dts b/tests/merge-omit-missing.dts
> new file mode 100644
> index 0000000..0357648
> --- /dev/null
> +++ b/tests/merge-omit-missing.dts
> @@ -0,0 +1,9 @@
> +/dts-v1/;
> +
> +/ {
> +	label: node {
> +	};
> +};
> +
> +/* Try to mark a node using a non-existent path */
> +/omit-if-no-ref/ &{/nosuchnode};
> diff --git a/tests/merge-overlay-order.dts b/tests/merge-overlay-order.dts
> new file mode 100644
> index 0000000..2490157
> --- /dev/null
> +++ b/tests/merge-overlay-order.dts
> @@ -0,0 +1,19 @@
> +/dts-v1/;
> +/plugin/;
> +
> +&basenode {
> +	label: foo {
> +	};
> +
> +	bar {
> +	};
> +};
> +
> +/* A node in the plugin itself, after the overlay fragment */

As discussed on another thread, I'm not sure we really want to allow
this, although we might need to at least in the short to medium term,
for compatibility.

> +/ {
> +	node {
> +	};
> +};
> +
> +/* A label in an overlay fragment can be referred to */
> +/delete-node/ &label;
> diff --git a/tests/merge-parse-error.dts b/tests/merge-parse-error.dts
> new file mode 100644
> index 0000000..c39318b
> --- /dev/null
> +++ b/tests/merge-parse-error.dts
> @@ -0,0 +1,17 @@
> +/dts-v1/;
> +
> +/ {
> +	label: node {
> +	};
> +};
> +
> +/* Reported even though the parser finds another error below */

Reporting multiple errors is, in general, desirable behaviour, but..


This look like something that might be broken by the proposed changes:
the division by zero will caught during parse (since that's when we
resolve arithmetic) but we're going to defer the discovery of the
unknown node until the merge pass.  While reporting both errors would
be nice, it's not worth complexifying the implementation for it.
Therefore, I'm not sure we want to enshrine that behaviour in a test.

> +&nosuchnode {
> +	prop;
> +};
> +
> +/ {
> +	node {
> +		prop = <(1/0)>;
> +	};
> +};
> diff --git a/tests/merge-path-ref-missing.dts b/tests/merge-path-ref-missing.dts
> new file mode 100644
> index 0000000..ad2e35e
> --- /dev/null
> +++ b/tests/merge-path-ref-missing.dts
> @@ -0,0 +1,11 @@
> +/dts-v1/;
> +
> +/ {
> +	node {
> +	};
> +};
> +
> +/* Try to redefine a node using a non-existent path */
> +&{/nosuchnode} {
> +	prop;
> +};
> diff --git a/tests/merge-path-relative.dts b/tests/merge-path-relative.dts
> new file mode 100644
> index 0000000..c4e00a4
> --- /dev/null
> +++ b/tests/merge-path-relative.dts
> @@ -0,0 +1,12 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	label: node {
> +	};
> +};
> +
> +/* A plugin cannot define a node using a label-relative path reference */

It's possible in future we could allow this in some cases, if the
label can be resolved locally in the plugin.  What we definitely can't
do is use a label-relative path reference with an external label, so
that would be the more useful case to test.

> +&{label/node} {
> +	prop;
> +};
> diff --git a/tests/merge-root-plugin.dts b/tests/merge-root-plugin.dts
> new file mode 100644
> index 0000000..f67355e
> --- /dev/null
> +++ b/tests/merge-root-plugin.dts
> @@ -0,0 +1,12 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/* This is in the root node of the plugin */
> +/ {
> +	prop1;
> +};
> +
> +/* This is an overlay fragment for the root node of the base tree */
> +&{/} {
> +	prop2;
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index f750faa..2d392e2 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -770,6 +770,80 @@ dtc_tests () {
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" phandle_references -- -I dts -O dtb "$SRCDIR/nonexist-label-ref.dts"
>      run_sh_test "$SRCDIR/dtc-fatal.sh" -m "Label or path nosuchnode not found" \
>  	-I dts -O dtb "$SRCDIR/nonexist-node-ref2.dts"
> +
> +    # Check each error when merging the fragments of a dts file, with its
> +    # position
> +    run_wrap_error_msg_test "test_label_ref.dts:7.8-15 Label or path handle not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/test_label_ref.dts"
> +    run_wrap_error_msg_test "nonexist-node-ref2.dts:9.1-12 Label or path nosuchnode not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/nonexist-node-ref2.dts"
> +    run_wrap_error_msg_test "merge-first-ref.dts:4.13-6.3 Label or path nosuchnode not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-first-ref.dts"
> +    run_wrap_error_msg_test "merge-first-ref.dts:9.15-27 Label or path nosuchnode2 not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-first-ref.dts"
> +    run_wrap_error_msg_test "merge-first-ref-relative.dts:5.15-7.3 Label-relative reference label/node not supported in plugin" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-first-ref-relative.dts"
> +    run_wrap_error_msg_test "merge-first-ref-relative.dts:10.15-26 Label or path nosuchnode not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-first-ref-relative.dts"
> +    run_wrap_error_msg_test "merge-labelled-relative.dts:12.1-10 Label-relative reference label/subnode not supported in plugin" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-labelled-relative.dts"
> +    run_wrap_error_msg_test "merge-path-relative.dts:10.1-14 Label-relative reference label/node not supported in plugin" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-path-relative.dts"
> +    run_wrap_error_msg_test "merge-path-ref-missing.dts:9.1-15 Label or path /nosuchnode not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-path-ref-missing.dts"
> +    run_wrap_error_msg_test "merge-delete-missing.dts:9.15-26 Label or path nosuchnode not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-delete-missing.dts"
> +    run_wrap_error_msg_test "merge-omit-missing.dts:9.18-32 Label or path /nosuchnode not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-omit-missing.dts"
> +    run_wrap_error_msg_test "merge-errors.dts:9.1-13 Label or path nosuchnode1 not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-errors.dts"
> +    run_wrap_error_msg_test "merge-errors.dts:13.15-27 Label or path nosuchnode2 not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-errors.dts"
> +    run_wrap_error_msg_test "merge-errors.dts:14.18-30 Label or path nosuchnode3 not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-errors.dts"
> +    run_wrap_error_msg_test "merge-parse-error.dts:9.1-12 Label or path nosuchnode not found" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-parse-error.dts"
> +    run_wrap_error_msg_test "merge-parse-error.dts:15.12-15 Division by zero" \
> +	$DTC -I dts -O dtb -o /dev/null "$SRCDIR/merge-parse-error.dts"
> +
> +    # A labelled reference to a node in a plugin adds the label to it
> +    run_dtc_test -@ -I dts -O dtb -o merge-labelled-plugin.test.dtb \
> +	"$SRCDIR/merge-labelled-plugin.dts"
> +    run_fdtget_test "/node" merge-labelled-plugin.test.dtb /__symbols__ newlabel
> +    run_fdtget_test "value" merge-labelled-plugin.test.dtb /node prop
> +
> +    # In a plugin, &{label} becomes an overlay fragment, even if it is found
> +    run_dtc_test -I dts -O dtb -o merge-label-braces.test.dtb \
> +	"$SRCDIR/merge-label-braces.dts"
> +    run_fdtget_test "" merge-label-braces.test.dtb /fragment@0/__overlay__ prop
> +    run_fdtget_test "node\nfragment@0\n__local_fixups__" merge-label-braces.test.dtb -l /
> +
> +    # The fragment for &{label} is applied after any local merges
> +    run_dtc_test -I dts -O dtb -o merge-label-order.test.dtb \
> +	"$SRCDIR/merge-label-order.dts"
> +    run_fdtget_test "third" merge-label-order.test.dtb /fragment@0/__overlay__/foo p
> +    run_fdtget_test "second" merge-label-order.test.dtb /fragment@1/__overlay__ p
> +
> +    # In a plugin, / { ... } and &{/} { ... } are not the same thing
> +    run_dtc_test -I dts -O dtb -o merge-root-plugin.test.dtb \
> +	"$SRCDIR/merge-root-plugin.dts"
> +    run_fdtget_test "" merge-root-plugin.test.dtb / prop1
> +    run_fdtget_test "/" merge-root-plugin.test.dtb /fragment@0 target-path
> +    run_fdtget_test "" merge-root-plugin.test.dtb /fragment@0/__overlay__ prop2
> +
> +    # In a plugin, a label which is not found becomes an overlay fragment
> +    run_dtc_test -I dts -O dtb -o merge-label-base.test.dtb \
> +	"$SRCDIR/merge-label-base.dts"
> +    run_fdtget_test "" merge-label-base.test.dtb /fragment@0/__overlay__ prop
> +    run_fdtget_test "/fragment@0:target:0" merge-label-base.test.dtb /__fixups__ baselabel
> +
> +    # In a plugin, a node in an overlay fragment can be deleted by its label,
> +    # and the overlay fragment comes before the later node in the plugin
> +    run_dtc_test -I dts -O dtb -o merge-overlay-order.test.dtb \
> +	"$SRCDIR/merge-overlay-order.dts"
> +    run_fdtget_test "fragment@0\nnode\n__fixups__" merge-overlay-order.test.dtb -l /
> +    run_fdtget_test "bar" merge-overlay-order.test.dtb -l /fragment@0/__overlay__
> +
>      check_tests "$SRCDIR/bad-name-property.dts" name_properties
>  
>      check_tests "$SRCDIR/bad-ncells.dts" address_cells_is_cell size_cells_is_cell interrupts_extended_is_cell
> -- 
> 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 --]