On Tue, Nov 13, 2018 at 03:30:08PM +1100, David Gibson wrote:
> On Fri, Oct 26, 2018 at 05:44:33PM +0200, Julia Lawall wrote:
> > Extend the parser to record positions, in build_node, build_node_delete,
> > and build_property.
> >
> > srcpos structures are added to the property and node types, and to the
> > parameter lists of the above functions that construct these types.
> > Nodes and properties that are created by the compiler rather than from
> > parsing source code have NULL as the srcpos value.
> >
> > merge_nodes, defined in livetree.c, uses srcpos_extend to combine
> > multiple positions, resulting in a list of positions. srcpos_extend is
> > defined in srcpos.c. New elements are added at the end. The srcpos
> > type, define in srcpos.h, is now a list structure with a next field.
> >
> > Another change to srcpos.c is to make srcpos_copy always do a full copy,
> > including a copy of the file substructure. This is required because
> > when dtc is used on the output of cpp, the successive detected file
> > names overwrite the file name in the file structure. The next field
> > does not need to be deep copied, because it is only updated in newly
> > copied positions and the positions to which it points have also been
> > copied. File names are only updated in uncopied position structures.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@xxxxxxx>
> > ---
> >
> > v6: SoB added, v5 information added
> >
> > v5: The main change is that srcpos_extend now concatenates the arguments in
> > the opposite order (new information added after the existing information),
> > which makes it possible to remove all of the complexity of the previous
> > implementation that was designed to avoid introducing circular
> > lists.
>
> Sorry I've taken a long time to look at this; I've been travelling
> then dealing with a backlog of work. I've now applied this.
... and now I've reverted it. Turns out it causes numerous SEGVs on
ppc. For whatever reason it doesn't blow up on x86, but valgrind does
detect that something is bad. The "make checkm" on Travis should have
picked that up, but while it showed some failures, it didn't actually
fail the test. I'm looking into that script error separately.
I didn't fully debug it, but a couple of things that look dubious:
> > dtc-parser.y | 13 ++++++++-----
> > dtc.h | 10 +++++++---
> > flattree.c | 4 ++--
> > fstree.c | 5 +++--
> > livetree.c | 33 ++++++++++++++++++++++-----------
> > srcpos.c | 22 ++++++++++++++++++++++
> > srcpos.h | 3 +++
> > 7 files changed, 67 insertions(+), 23 deletions(-)
> >
> > diff --git a/dtc-parser.y b/dtc-parser.y
> > index deda663..2ec981e 100644
> > --- a/dtc-parser.y
> > +++ b/dtc-parser.y
> > @@ -180,7 +180,10 @@ devicetree:
> > */
> > if (!($<flags>-1 & DTSF_PLUGIN))
> > ERROR(&@2, "Label or path %s not found", $1);
> > - $$ = add_orphan_node(name_node(build_node(NULL, NULL), ""), $2, $1);
> > + $$ = add_orphan_node(
> > + name_node(build_node(NULL, NULL, NULL),
> > + ""),
> > + $2, $1);
> > }
> > | devicetree DT_LABEL dt_ref nodedef
> > {
> > @@ -260,7 +263,7 @@ devicetree:
> > nodedef:
> > '{' proplist subnodes '}' ';'
> > {
> > - $$ = build_node($2, $3);
> > + $$ = build_node($2, $3, &@$);
> > }
> > ;
> >
> > @@ -278,11 +281,11 @@ proplist:
> > propdef:
> > DT_PROPNODENAME '=' propdata ';'
> > {
> > - $$ = build_property($1, $3);
> > + $$ = build_property($1, $3, &@$);
> > }
> > | DT_PROPNODENAME ';'
> > {
> > - $$ = build_property($1, empty_data);
> > + $$ = build_property($1, empty_data, &@$);
> > }
> > | DT_DEL_PROP DT_PROPNODENAME ';'
> > {
> > @@ -563,7 +566,7 @@ subnode:
> > }
> > | DT_DEL_NODE DT_PROPNODENAME ';'
> > {
> > - $$ = name_node(build_node_delete(), $2);
> > + $$ = name_node(build_node_delete(&@$), $2);
> > }
> > | DT_OMIT_NO_REF subnode
> > {
> > diff --git a/dtc.h b/dtc.h
> > index cbe5415..8722cbc 100644
> > --- a/dtc.h
> > +++ b/dtc.h
> > @@ -158,6 +158,7 @@ struct property {
> > struct property *next;
> >
> > struct label *labels;
> > + struct srcpos *srcpos;
> > };
> >
> > struct node {
> > @@ -177,6 +178,7 @@ struct node {
> >
> > struct label *labels;
> > const struct bus_type *bus;
> > + struct srcpos *srcpos;
> >
> > bool omit_if_unused, is_referenced;
> > };
> > @@ -205,13 +207,15 @@ struct node {
> > void add_label(struct label **labels, char *label);
> > void delete_labels(struct label **labels);
> >
> > -struct property *build_property(char *name, struct data val);
> > +struct property *build_property(char *name, struct data val,
> > + struct srcpos *srcpos);
> > struct property *build_property_delete(char *name);
> > struct property *chain_property(struct property *first, struct property *list);
> > struct property *reverse_properties(struct property *first);
> >
> > -struct node *build_node(struct property *proplist, struct node *children);
> > -struct node *build_node_delete(void);
> > +struct node *build_node(struct property *proplist, struct node *children,
> > + struct srcpos *srcpos);
> > +struct node *build_node_delete(struct srcpos *srcpos);
> > struct node *name_node(struct node *node, char *name);
> > struct node *omit_node_if_unused(struct node *node);
> > struct node *reference_node(struct node *node);
> > diff --git a/flattree.c b/flattree.c
> > index 851ea87..acf04c3 100644
> > --- a/flattree.c
> > +++ b/flattree.c
> > @@ -692,7 +692,7 @@ static struct property *flat_read_property(struct inbuf *dtbuf,
> >
> > val = flat_read_data(dtbuf, proplen);
> >
> > - return build_property(name, val);
> > + return build_property(name, val, NULL);
> > }
> >
> >
> > @@ -750,7 +750,7 @@ static struct node *unflatten_tree(struct inbuf *dtbuf,
> > char *flatname;
> > uint32_t val;
> >
> > - node = build_node(NULL, NULL);
> > + node = build_node(NULL, NULL, NULL);
> >
> > flatname = flat_read_string(dtbuf);
> >
> > diff --git a/fstree.c b/fstree.c
> > index ae7d06c..1e7eeba 100644
> > --- a/fstree.c
> > +++ b/fstree.c
> > @@ -34,7 +34,7 @@ static struct node *read_fstree(const char *dirname)
> > if (!d)
> > die("Couldn't opendir() \"%s\": %s\n", dirname, strerror(errno));
> >
> > - tree = build_node(NULL, NULL);
> > + tree = build_node(NULL, NULL, NULL);
> >
> > while ((de = readdir(d)) != NULL) {
> > char *tmpname;
> > @@ -60,7 +60,8 @@ static struct node *read_fstree(const char *dirname)
> > } else {
> > prop = build_property(xstrdup(de->d_name),
> > data_copy_file(pfile,
> > - st.st_size));
> > + st.st_size),
> > + NULL);
> > add_property(tree, prop);
> > fclose(pfile);
> > }
> > diff --git a/livetree.c b/livetree.c
> > index 4ff0679..7a2e644 100644
> > --- a/livetree.c
> > +++ b/livetree.c
> > @@ -19,6 +19,7 @@
> > */
> >
> > #include "dtc.h"
> > +#include "srcpos.h"
> >
> > /*
> > * Tree building functions
> > @@ -50,7 +51,8 @@ void delete_labels(struct label **labels)
> > label->deleted = 1;
> > }
> >
> > -struct property *build_property(char *name, struct data val)
> > +struct property *build_property(char *name, struct data val,
> > + struct srcpos *srcpos)
> > {
> > struct property *new = xmalloc(sizeof(*new));
> >
> > @@ -58,6 +60,7 @@ struct property *build_property(char *name, struct data val)
> >
> > new->name = name;
> > new->val = val;
> > + new->srcpos = srcpos_copy(srcpos);
> >
> > return new;
> > }
> > @@ -97,7 +100,8 @@ struct property *reverse_properties(struct property *first)
> > return head;
> > }
> >
> > -struct node *build_node(struct property *proplist, struct node *children)
> > +struct node *build_node(struct property *proplist, struct node *children,
> > + struct srcpos *srcpos)
> > {
> > struct node *new = xmalloc(sizeof(*new));
> > struct node *child;
> > @@ -106,6 +110,7 @@ struct node *build_node(struct property *proplist, struct node *children)
> >
> > new->proplist = reverse_properties(proplist);
> > new->children = children;
> > + new->srcpos = srcpos_copy(srcpos);
> >
> > for_each_child(new, child) {
> > child->parent = new;
> > @@ -114,13 +119,14 @@ struct node *build_node(struct property *proplist, struct node *children)
> > return new;
> > }
> >
> > -struct node *build_node_delete(void)
> > +struct node *build_node_delete(struct srcpos *srcpos)
> > {
> > struct node *new = xmalloc(sizeof(*new));
> >
> > memset(new, 0, sizeof(*new));
> >
> > new->deleted = 1;
> > + new->srcpos = srcpos_copy(srcpos);
> >
> > return new;
> > }
> > @@ -183,6 +189,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> >
> > old_prop->val = new_prop->val;
> > old_prop->deleted = 0;
> > + free(old_prop->srcpos);
> > + old_prop->srcpos = new_prop->srcpos;
> > free(new_prop);
> > new_prop = NULL;
> > break;
> > @@ -223,6 +231,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> > add_child(old_node, new_child);
> > }
> >
> > + old_node->srcpos = srcpos_extend(old_node->srcpos, new_node->srcpos);
> > +
> > /* The new node contents are now merged into the old node. Free
> > * the new node. */
> > free(new_node);
> > @@ -241,18 +251,18 @@ struct node * add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> > if (ref[0] == '/') {
> > d = data_append_data(d, ref, strlen(ref) + 1);
> >
> > - p = build_property("target-path", d);
> > + p = build_property("target-path", d, NULL);
> > } else {
> > d = data_add_marker(d, REF_PHANDLE, ref);
> > d = data_append_integer(d, 0xffffffff, 32);
> >
> > - p = build_property("target", d);
> > + p = build_property("target", d, NULL);
> > }
> >
> > xasprintf(&name, "fragment@%u",
> > next_orphan_fragment++);
> > name_node(new_node, "__overlay__");
> > - node = build_node(p, new_node);
> > + node = build_node(p, new_node, NULL);
> > name_node(node, name);
> >
> > add_child(dt, node);
> > @@ -351,7 +361,7 @@ void append_to_property(struct node *node,
> > p->val = d;
> > } else {
> > d = data_append_data(empty_data, data, len);
> > - p = build_property(name, d);
> > + p = build_property(name, d, NULL);
> > add_property(node, p);
> > }
> > }
> > @@ -609,11 +619,11 @@ cell_t get_node_phandle(struct node *root, struct node *node)
> >
> > if (!get_property(node, "linux,phandle")
> > && (phandle_format & PHANDLE_LEGACY))
> > - add_property(node, build_property("linux,phandle", d));
> > + add_property(node, build_property("linux,phandle", d, NULL));
> >
> > if (!get_property(node, "phandle")
> > && (phandle_format & PHANDLE_EPAPR))
> > - add_property(node, build_property("phandle", d));
> > + add_property(node, build_property("phandle", d, NULL));
> >
> > /* If the node *does* have a phandle property, we must
> > * be dealing with a self-referencing phandle, which will be
> > @@ -787,7 +797,7 @@ static struct node *build_and_name_child_node(struct node *parent, char *name)
> > {
> > struct node *node;
> >
> > - node = build_node(NULL, NULL);
> > + node = build_node(NULL, NULL, NULL);
> > name_node(node, xstrdup(name));
> > add_child(parent, node);
> >
> > @@ -849,7 +859,8 @@ static void generate_label_tree_internal(struct dt_info *dti,
> > /* insert it */
> > p = build_property(l->label,
> > data_copy_mem(node->fullpath,
> > - strlen(node->fullpath) + 1));
> > + strlen(node->fullpath) + 1),
> > + NULL);
> > add_property(an, p);
> > }
> >
> > diff --git a/srcpos.c b/srcpos.c
> > index cb6ed0e..cba1c0f 100644
> > --- a/srcpos.c
> > +++ b/srcpos.c
> > @@ -207,6 +207,7 @@ struct srcpos srcpos_empty = {
> > .last_line = 0,
> > .last_column = 0,
> > .file = NULL,
> > + .next = NULL,
> > };
AFAICT srcpos_empty isn't actually used anywhere, so we should just
eliminate it.
> >
> > void srcpos_update(struct srcpos *pos, const char *text, int len)
> > @@ -234,13 +235,34 @@ struct srcpos *
> > srcpos_copy(struct srcpos *pos)
> > {
> > struct srcpos *pos_new;
> > + struct srcfile_state *srcfile_state;
> > +
> > + if (!pos)
> > + return NULL;
> >
> > pos_new = xmalloc(sizeof(struct srcpos));
> > memcpy(pos_new, pos, sizeof(struct srcpos));
> >
> > + /* allocate without free */
> > + srcfile_state = xmalloc(sizeof(struct srcfile_state));
> > + memcpy(srcfile_state, pos->file, sizeof(struct srcfile_state));
> > + pos_new->file = srcfile_state;
> > +
This only does a shallow copy - it doesn't copy anything chained in
'next'. I don't think it actually ever gets used for a srcpos that's
supposed to have a chain, but in that case it should probably have an
assert to make it clearer what it can and can't do.
When I added such an assert, it trips repeatedly (on ppc), but that
seems to be because we're passing in a srcpos with uninitialized
'next' rather than because we're passing in a legitimately chained srcpos.
> > return pos_new;
> > }
> >
> > +struct srcpos *srcpos_extend(struct srcpos *pos, struct srcpos *newtail)
> > +{
> > + struct srcpos *p;
> > +
> > + if (!pos)
> > + return newtail;
> > +
> > + for (p = pos; p->next != NULL; p = p->next);
> > + p->next = newtail;
> > + return pos;
> > +}
> > +
> > char *
> > srcpos_string(struct srcpos *pos)
> > {
> > diff --git a/srcpos.h b/srcpos.h
> > index 9ded12a..d88e7cb 100644
> > --- a/srcpos.h
> > +++ b/srcpos.h
> > @@ -74,6 +74,7 @@ struct srcpos {
> > int last_line;
> > int last_column;
> > struct srcfile_state *file;
> > + struct srcpos *next;
> > };
> >
> > #define YYLTYPE struct srcpos
> > @@ -105,6 +106,8 @@ extern struct srcpos srcpos_empty;
> >
> > extern void srcpos_update(struct srcpos *pos, const char *text, int len);
> > extern struct srcpos *srcpos_copy(struct srcpos *pos);
> > +extern struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
> > + struct srcpos *old_srcpos);
> > extern char *srcpos_string(struct srcpos *pos);
> >
> > extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
>
--
David Gibson | 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:
signature.asc
Description: PGP signature
