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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



On Fri, Apr 20, 2018 at 02:42:37PM +0200, Julia Lawall wrote:
> 
> 
> On Fri, 20 Apr 2018, David Gibson wrote:
> 
> > On Fri, Feb 02, 2018 at 09:41:49PM +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.  As srcpos_extend is
> > > performing a union, it is O(n^2).  The recursion serves to copy the new
> > > list and then add it to the front of the old one.  Using recursion keeps
> > > the positions in order from newer to older.  On each recursive call, the
> > > current element is compared against each element in the proposed new
> > > tail, to avoid duplicates.  Since the list of positions is only extended
> > > in srcpos_extend, and srcpos_extend does not produce lists with
> > > duplicates, it is not necessary to check for the current element in the
> > > tail of its own list.
> > >
> > > 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>
> >
> > Sorry I've taken so long to look at this properly.
> >
> > This looks pretty good overall.  Couple of nits noted below..
> >
> > [snip]
> > > diff --git a/srcpos.c b/srcpos.c
> > > index 9dd3297..c6688e7 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 the if - that's what xmalloc is for (plus we've already
> > written to the pointer in the memcpy().
> 
> Thanks.  I'll fix it up.

In fact you'd already fixed it in a later patch, so just need to move
the hunks about.

> > > +		/* 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;
> > > +
> > > +	next = srcpos_extend(pos->next, newtail);
> > > +
> > > +	for (p = newtail; p != NULL; p = p->next)
> > > +		if (same_pos(pos, p))
> > > +			return next;
> > > +
> > > +	pos = srcpos_copy(pos);
> > > +	pos->next = next;
> > > +	return pos;
> >
> > This copies pos and doesn't free the original - and the caller doesn't
> > free the original either, so there's a leak here.
> 
> I suspect that we can't free it, because someone else may be using it, but
> I will check on it.

Hrm.  AIUI the copies of the srcpos stored in nodes/properties are
owned by those nodes and properties and shoulnd't be shared with
anything.  And the only caller of srcpos_extend() is, I believe,
merging two nodes, so the input nodes should be logically discarded.
So I think it should be ok.

But, yeah, lifetime management is not very easy to follow in dtc, so
do check.

-- 
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


[Index of Archives]     [Device Tree]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux