Re: [PATCH v2 07/15] dtc: Move label-reference handling out of the grammar

[Herve Codina <herve.codina@bootlin.com>]

Hi Simon,

On Thu, 5 Nov 2026 14:49:45 -0700
Simon Glass <sjg@chromium.org> wrote:

> Hi Herve,
> 
> On Thu, 5 Nov 2026 at 02:07, Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi David, Simon,
> >
> > On Wed,  4 Nov 2026 14:17:46 -0700
> > Simon Glass <sjg@chromium.org> wrote:
> >  
> > > The grammar action for a top-level &label { ... } holds its merge logic,
> > > including making an overlay fragment in a plugin when the label is not
> > > found.
> > >
> > > Move it into a new merge_ref(), passing it a struct fragment for the
> > > reference and node.
> > >
> > > The output does not change.
> > >
> > > Assisted-by: Claude:claude-opus-5-5
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Rename merge_label_ref() to merge_ref()
> > > - Build the fragment in one step, with build_fragment_ref()
> > > - Return true on success from merge_ref()
> > > - Check the fragment type with assert()
> > >
> > >  dtc-parser.y | 23 +++++++++--------------
> > >  dtc.h        |  1 +
> > >  livetree.c   | 26 ++++++++++++++++++++++++++
> > >  3 files changed, 36 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/dtc-parser.y b/dtc-parser.y
> > > index 9b9b6db..c58a66d 100644
> > > --- a/dtc-parser.y
> > > +++ b/dtc-parser.y
> > > @@ -214,21 +214,16 @@ devicetree:
> > >               }
> > >       | devicetree DT_LABEL_REF nodedef
> > >               {
> > > -                     struct node *target = get_node_by_ref($1, $2);
> > > +                     struct fragment *f;
> > >
> > > -                     if (target) {
> > > -                             merge_nodes(target, $3);
> > > -                     } else {
> > > -                             /*
> > > -                              * We rely on the rule being always:
> > > -                              *   versioninfo plugindecl memreserves devicetree
> > > -                              * so $-1 is what we want (plugindecl)
> > > -                              */
> > > -                             if ($<flags>-1 & DTSF_PLUGIN)
> > > -                                     add_orphan_node($1, $3, $2);
> > > -                             else
> > > -                                     ERROR(&@2, "Label or path %s not found", $2);
> > > -                     }
> > > +                     f = build_fragment_ref(NULL, NULL, $2, &@2, $3);
> > > +                     /*
> > > +                      * We rely on the rule being always:
> > > +                      *   versioninfo plugindecl memreserves devicetree
> > > +                      * so $-1 is what we want (plugindecl)
> > > +                      */
> > > +                     if (!merge_ref($1, f, $<flags>-1 & DTSF_PLUGIN))
> > > +                             treesource_error = true;
> > >                       $$ = $1;
> > >               }
> > >       | devicetree DT_DEL_NODE dt_ref ';'
> > > diff --git a/dtc.h b/dtc.h
> > > index 8c4d7bf..9b4389b 100644
> > > --- a/dtc.h
> > > +++ b/dtc.h
> > > @@ -373,6 +373,7 @@ struct fragment *build_fragment_omit_if_no_ref(char *ref,
> > >  struct fragment *append_fragment(struct fragment *list, struct fragment *new);
> > >  void free_fragment(struct fragment *f);
> > >  bool is_ref_relative(const char *ref);
> > > +bool merge_ref(struct node *dt, struct fragment *f, bool plugin);
> > >  bool merge_delete_node(struct node *dt, struct fragment *f);
> > >  bool merge_omit_if_no_ref(struct node *dt, struct fragment *f);
> > >
> > > diff --git a/livetree.c b/livetree.c
> > > index 3ff4e14..179866d 100644
> > > --- a/livetree.c
> > > +++ b/livetree.c
> > > @@ -573,6 +573,32 @@ bool is_ref_relative(const char *ref)
> > >       return ref[0] != '/' && strchr(&ref[1], '/');
> > >  }
> > >
> > > +/*
> > > + * Merge a fragment which refers to a label into the node with that label. In
> > > + * a plugin, a label which is not found becomes an overlay fragment.
> > > + *
> > > + * Returns true on success, false on an error, which has been reported.
> > > + */
> > > +bool merge_ref(struct node *dt, struct fragment *f, bool plugin)
> > > +{
> > > +     struct node *target = get_node_by_ref(dt, f->ref);
> > > +
> > > +     assert(f->type == FRAGMENT_NODE);
> > > +     if (target) {
> > > +             merge_nodes(target, f->node);
> > > +             return true;
> > > +     }
> > > +     if (!plugin) {
> > > +             srcpos_error(f->ref_srcpos, "Error",
> > > +                          "Label or path %s not found", f->ref);
> > > +             return false;
> > > +     }
> > > +     /* The label may be in the base tree, so add a fragment@N node for it */
> > > +     add_orphan_node(dt, f->node, f->ref);
> > > +
> > > +     return true;
> > > +}
> > > +  
> >
> > Even if this function is slightly modified in later patches, I think it mixes
> > two different operations:
> >  - Merging fragments
> >  - Converting fragments in overlay output format.
> >
> > IHMO, those two operations should be fully decoupled.
> >
> > merge_ref() or any other operation whose goal is to merge a fragment should
> > only merge fragment either in the existing root node or in a previous fragment
> > available in the fragment list.
> >
> > add_orphan_node() should not be called from a merge_*() function related to
> > fragments.
> >
> > After merging all fragment we should have a root node and/or a list of fragment
> > reduced to unmerged ones.
> >
> > Base on that, the overlay conversion can be performed using a dedicated
> > function, out of merge_*() functions.
> >
> > Something like this:
> > ---- 8< ----
> > void convert_to_overlay_output_format(struct dt_info *dti)
> > {
> >         struct fragment *f;
> >
> >         if (!dti->dt) {
> >                 /* The root node is not present. Create an empty one */
> >                 dti->dt = ...;
> >         }
> >
> >         /*
> >          * The fragment list have been previously merged.
> >          * Available fragment left in the list are fragments
> >          * that cannot be merged and so those fragments are
> >          * orphan nodes.
> >          *
> >          * Convert those orphan nodes to the /fragment@N format overlay
> >          * format.
> >          */
> >         for (f = dti->fragments; f; f = next) {
> >                 add_orphan_node(dti->dt, f->node, f->ref);
> >
> >         /* Add __fixups__ and __local_fixups__ nodes ? */
> >         todo();
> > }
> >
> > in main():
> >
> >         /* Parse input */
> >         dti = dt_from_*();
> >
> >         checks_pre_fragment_merge(dti);
> >
> >         merge_fragments(dti); /* <--- Fragments are merged.
> >                                *      Remaining items in the fragment list, if
> >                                *      any are Orphan nodes (i.e. fragments that
> >                                *      couldn't be merged).
> >                                */
> >
> >         checks_post_fragment_merge(dti); <--- Orphan ok for overlays but ko for !overlay dts
> >
> >         /* Convert outputs */
> >         if (dti->dtsflags & DTSF_PLUGIN) {
> >                 convert_to_overlay_output_format(dti);
> >                 checks_overlay_format(dti);  /* <--- Not sure it is needed */
> >         }
> >
> >         /* Generate output */
> >         dt_to_*();
> > }
> > --- 8< ---  
> 
> I tend to agree - we are heading in this direction.
> 
> But this series is meant to keep the output the same (apart from the
> &{label} case), so I would prefer to do the split as a follow-up. I
> just filed an issue for the overall effort here:

I don't see why splitting merge and "overlay format conversion" changes the
output. 

This series could split these operations in two distinct functions.

> 
> https://github.com/dgibson/dtc/issues/201
> 
> What do you think?
> 
> Regards,
> Simon

Best regards,
Hervé