Re: [PATCH 2/3 v3] annotations: add positions

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

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-L2FTfq7BK8M@public.gmane.org>
> Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]