On Tue, Jan 23, 2018 at 11:21:30PM +0100, Julia Lawall wrote:
> The parser is extended 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 and removes duplicates.
>
> 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>
> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
This is looking pretty good, but a few remaining nits.
[snip]
> @@ -169,6 +175,7 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>
> old_prop->val = new_prop->val;
> old_prop->deleted = 0;
You need to free old_prop->srcpos here first, no?
> + old_prop->srcpos = new_prop->srcpos;
> free(new_prop);
> new_prop = NULL;
> break;
> @@ -209,6 +216,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> add_child(old_node, new_child);
> }
>
> + old_node->srcpos = srcpos_extend(new_node->srcpos, old_node->srcpos);
> +
> /* The new node contents are now merged into the old node. Free
> * the new node. */
> free(new_node);
> @@ -216,7 +225,7 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> return old_node;
> }
>
> -struct node * add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> +struct node *add_orphan_node(struct node *dt, struct node *new_node, char *ref)
Extraneous whitespace change.
> {
> static unsigned int next_orphan_fragment = 0;
> struct node *node;
> @@ -227,12 +236,12 @@ struct node * add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> 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);
> @@ -331,7 +340,8 @@ void append_to_property(struct node *node,
> p->val = d;
> } else {
> d = data_append_data(empty_data, data, len);
> - p = build_property(name, d);
> + /* used from fixup or local_fixup, so no position */
> + p = build_property(name, d, NULL);
> add_property(node, p);
> }
> }
> @@ -587,13 +597,17 @@ cell_t get_node_phandle(struct node *root, struct node *node)
> && (phandle_format & PHANDLE_LEGACY))
> add_property(node,
> build_property("linux,phandle",
> - data_append_cell(empty_data, phandle)));
> + data_append_cell(empty_data,
> + phandle),
> + NULL));
>
> if (!get_property(node, "phandle")
> && (phandle_format & PHANDLE_EPAPR))
> add_property(node,
> build_property("phandle",
> - data_append_cell(empty_data, phandle)));
> + data_append_cell(empty_data,
> + phandle),
> + NULL));
>
> /* If the node *does* have a phandle property, we must
> * be dealing with a self-referencing phandle, which will be
> @@ -767,7 +781,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);
>
> @@ -827,9 +841,11 @@ static void generate_label_tree_internal(struct dt_info *dti,
> }
>
> /* insert it */
> + /* no position information for symbols and aliases */
> 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 9dd3297..a2d621f 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,
> };
>
> void srcpos_update(struct srcpos *pos, const char *text, int len)
> @@ -234,13 +235,52 @@ 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));
>
> + if (pos_new) {
No need for this test, xmalloc() will abort on failure.
> + /* allocate without free */
> + srcfile_state = xmalloc(sizeof(struct srcfile_state));
> + memcpy(srcfile_state, pos->file, sizeof(struct srcfile_state));
> +
> + pos_new->file = srcfile_state;
> + }
> +
> return pos_new;
> }
>
> +static bool same_pos(struct srcpos *p1, struct srcpos *p2)
> +{
> + return (p1->first_line == p2->first_line &&
> + p1->first_column == p2->first_column &&
> + p1->last_line == p2->last_line &&
> + p1->last_column == p2->last_column &&
> + !strcmp(p1->file->name, p2->file->name));
> +}
> +
> +struct srcpos *srcpos_extend(struct srcpos *pos, struct srcpos *newtail)
> +{
> + struct srcpos *next, *p;
> +
> + if (!pos)
> + return newtail;
I'm having a lot of trouble following this. You've got both a
recursion and a loop through the list, so you seem to have an O(n^2)
with multiple redundant checks.
> +
> + next = srcpos_extend(pos->next, newtail);
> +
> + for (p = next; p != NULL; p = p->next)
> + if (same_pos(pos, p))
> + return next;
> +
> + pos = srcpos_copy(pos);
And while I'm not *that* fussed by memory leaks, I have a nasty
feeling this will leak stuff at every level of the recursion, which is
a bit much.
> + pos->next = next;
> + 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
