[PATCH 0/5] annotations

[PATCH 0/5] annotations

[Julia Lawall]

These patches pick up on Frank Rowand's previous efforts to annotate
compiled device tree code with file names and line numbers.  Two new
options are provided: --annotate (abbreviated -T) and --annotate-full
(abbreviated -F).  The fomer provides short file names and line numbers,
while the latter provides full paths, and starting and ending lne and
column numbers.

[PATCH 1/5] annotations: Check for NULL pos

[Julia Lawall]

Check for NULL position and file name.  Check for xasprintf failure.
Builds on a patch proposed by Frank Rowand:

https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00377.html

Annotation extension will introduce the possibility of the position
being NULL.

Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
---
 srcpos.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/srcpos.c b/srcpos.c
index 9d38459..7f2626c 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -249,24 +249,35 @@ srcpos_copy(struct srcpos *pos)
 char *
 srcpos_string(struct srcpos *pos)
 {
-	const char *fname = "<no-file>";
+	const char *fname;
 	char *pos_str;
-
-	if (pos->file && pos->file->name)
+	int rc;
+
+	if (!pos) {
+		rc = asprintf(&pos_str, "%s:<no-line>", fname);
+		goto out;
+        } else if (!pos->file)
+		fname = "<no-file>";
+	else if (!pos->file->name)
+		fname = "<no-filename>";
+	else
 		fname = pos->file->name;
 
-
 	if (pos->first_line != pos->last_line)
-		xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
-			  pos->first_line, pos->first_column,
-			  pos->last_line, pos->last_column);
+		rc = xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
+			       pos->first_line, pos->first_column,
+			       pos->last_line, pos->last_column);
 	else if (pos->first_column != pos->last_column)
-		xasprintf(&pos_str, "%s:%d.%d-%d", fname,
-			  pos->first_line, pos->first_column,
-			  pos->last_column);
+		rc = xasprintf(&pos_str, "%s:%d.%d-%d", fname,
+			       pos->first_line, pos->first_column,
+			       pos->last_column);
 	else
-		xasprintf(&pos_str, "%s:%d.%d", fname,
-			  pos->first_line, pos->first_column);
+		rc = xasprintf(&pos_str, "%s:%d.%d", fname,
+			       pos->first_line, pos->first_column);
+
+out:
+	if (rc == -1)
+		die("Couldn't allocate in srcpos string");
 
 	return pos_str;
 }
-- 
2.7.4

Re: [PATCH 1/5] annotations: Check for NULL pos

[David Gibson]

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

On Mon, Jan 08, 2018 at 02:36:43PM +0100, Julia Lawall wrote:
> Check for NULL position and file name.  Check for xasprintf failure.
> Builds on a patch proposed by Frank Rowand:
> 
> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00377.html
> 
> Annotation extension will introduce the possibility of the position
> being NULL.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> ---
>  srcpos.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/srcpos.c b/srcpos.c
> index 9d38459..7f2626c 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -249,24 +249,35 @@ srcpos_copy(struct srcpos *pos)
>  char *
>  srcpos_string(struct srcpos *pos)
>  {
> -	const char *fname = "<no-file>";
> +	const char *fname;
>  	char *pos_str;
> -
> -	if (pos->file && pos->file->name)
> +	int rc;
> +
> +	if (!pos) {
> +		rc = asprintf(&pos_str, "%s:<no-line>", fname);

Uh.. you've removed the initializer of fname, so this branch will use
it uninitialized, no?

> +		goto out;
> +        } else if (!pos->file)

I prefer to keep braces on all branches if they're present on any
branch.

> +		fname = "<no-file>";
> +	else if (!pos->file->name)
> +		fname = "<no-filename>";
> +	else
>  		fname = pos->file->name;
>  
> -
>  	if (pos->first_line != pos->last_line)
> -		xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> -			  pos->first_line, pos->first_column,
> -			  pos->last_line, pos->last_column);
> +		rc = xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> +			       pos->first_line, pos->first_column,
> +			       pos->last_line, pos->last_column);
>  	else if (pos->first_column != pos->last_column)
> -		xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> -			  pos->first_line, pos->first_column,
> -			  pos->last_column);
> +		rc = xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> +			       pos->first_line, pos->first_column,
> +			       pos->last_column);
>  	else
> -		xasprintf(&pos_str, "%s:%d.%d", fname,
> -			  pos->first_line, pos->first_column);
> +		rc = xasprintf(&pos_str, "%s:%d.%d", fname,
> +			       pos->first_line, pos->first_column);
> +
> +out:
> +	if (rc == -1)
> +		die("Couldn't allocate in srcpos string");

You shouldn't need this - xasprintf() will already abort on allocation
failure, that's the whole point of it.

>  	return pos_str;
>  }

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

Re: [PATCH 1/5] annotations: Check for NULL pos

[Julia Lawall]

On Tue, 9 Jan 2018, David Gibson wrote:

> On Mon, Jan 08, 2018 at 02:36:43PM +0100, Julia Lawall wrote:
> > Check for NULL position and file name.  Check for xasprintf failure.
> > Builds on a patch proposed by Frank Rowand:
> >
> > https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00377.html
> >
> > Annotation extension will introduce the possibility of the position
> > being NULL.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> > ---
> >  srcpos.c | 35 +++++++++++++++++++++++------------
> >  1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/srcpos.c b/srcpos.c
> > index 9d38459..7f2626c 100644
> > --- a/srcpos.c
> > +++ b/srcpos.c
> > @@ -249,24 +249,35 @@ srcpos_copy(struct srcpos *pos)
> >  char *
> >  srcpos_string(struct srcpos *pos)
> >  {
> > -	const char *fname = "<no-file>";
> > +	const char *fname;
> >  	char *pos_str;
> > -
> > -	if (pos->file && pos->file->name)
> > +	int rc;
> > +
> > +	if (!pos) {
> > +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
>
> Uh.. you've removed the initializer of fname, so this branch will use
> it uninitialized, no?

Oops, not sure why that looked like a good idea.

>
> > +		goto out;
> > +        } else if (!pos->file)
>
> I prefer to keep braces on all branches if they're present on any
> branch.

OK.

>
> > +		fname = "<no-file>";
> > +	else if (!pos->file->name)
> > +		fname = "<no-filename>";
> > +	else
> >  		fname = pos->file->name;
> >
> > -
> >  	if (pos->first_line != pos->last_line)
> > -		xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> > -			  pos->first_line, pos->first_column,
> > -			  pos->last_line, pos->last_column);
> > +		rc = xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> > +			       pos->first_line, pos->first_column,
> > +			       pos->last_line, pos->last_column);
> >  	else if (pos->first_column != pos->last_column)
> > -		xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> > -			  pos->first_line, pos->first_column,
> > -			  pos->last_column);
> > +		rc = xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> > +			       pos->first_line, pos->first_column,
> > +			       pos->last_column);
> >  	else
> > -		xasprintf(&pos_str, "%s:%d.%d", fname,
> > -			  pos->first_line, pos->first_column);
> > +		rc = xasprintf(&pos_str, "%s:%d.%d", fname,
> > +			       pos->first_line, pos->first_column);
> > +
> > +out:
> > +	if (rc == -1)
> > +		die("Couldn't allocate in srcpos string");
>
> You shouldn't need this - xasprintf() will already abort on allocation
> failure, that's the whole point of it.

OK, I'll drop it.

Thanks for the feedback.  I'll try to send a new version later today.

julia

> >  	return pos_str;
> >  }
>
> --
> 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
>

Re: [PATCH 1/5] annotations: Check for NULL pos

[Frank Rowand]

On 01/08/18 05:36, Julia Lawall wrote:
> Check for NULL position and file name.  Check for xasprintf failure.
> Builds on a patch proposed by Frank Rowand:
> 
> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00377.html
> 
> Annotation extension will introduce the possibility of the position
> being NULL.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> ---
>  srcpos.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/srcpos.c b/srcpos.c
> index 9d38459..7f2626c 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -249,24 +249,35 @@ srcpos_copy(struct srcpos *pos)
>  char *
>  srcpos_string(struct srcpos *pos)
>  {
> -	const char *fname = "<no-file>";
> +	const char *fname;
>  	char *pos_str;
> -
> -	if (pos->file && pos->file->name)
> +	int rc;
> +
> +	if (!pos) {
> +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
> +		goto out;
> +        } else if (!pos->file)
   ^^^^^^^^
    spaces instead of a tab

-Frank

> +		fname = "<no-file>";
> +	else if (!pos->file->name)
> +		fname = "<no-filename>";
> +	else
>  		fname = pos->file->name;
>  
> -
>  	if (pos->first_line != pos->last_line)
> -		xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> -			  pos->first_line, pos->first_column,
> -			  pos->last_line, pos->last_column);
> +		rc = xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> +			       pos->first_line, pos->first_column,
> +			       pos->last_line, pos->last_column);
>  	else if (pos->first_column != pos->last_column)
> -		xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> -			  pos->first_line, pos->first_column,
> -			  pos->last_column);
> +		rc = xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> +			       pos->first_line, pos->first_column,
> +			       pos->last_column);
>  	else
> -		xasprintf(&pos_str, "%s:%d.%d", fname,
> -			  pos->first_line, pos->first_column);
> +		rc = xasprintf(&pos_str, "%s:%d.%d", fname,
> +			       pos->first_line, pos->first_column);
> +
> +out:
> +	if (rc == -1)
> +		die("Couldn't allocate in srcpos string");
>  
>  	return pos_str;
>  }
> 

Re: [PATCH 1/5] annotations: Check for NULL pos

[Julia Lawall]

On Tue, 9 Jan 2018, Frank Rowand wrote:

> On 01/08/18 05:36, Julia Lawall wrote:
> > Check for NULL position and file name.  Check for xasprintf failure.
> > Builds on a patch proposed by Frank Rowand:
> >
> > https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00377.html
> >
> > Annotation extension will introduce the possibility of the position
> > being NULL.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> > ---
> >  srcpos.c | 35 +++++++++++++++++++++++------------
> >  1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/srcpos.c b/srcpos.c
> > index 9d38459..7f2626c 100644
> > --- a/srcpos.c
> > +++ b/srcpos.c
> > @@ -249,24 +249,35 @@ srcpos_copy(struct srcpos *pos)
> >  char *
> >  srcpos_string(struct srcpos *pos)
> >  {
> > -	const char *fname = "<no-file>";
> > +	const char *fname;
> >  	char *pos_str;
> > -
> > -	if (pos->file && pos->file->name)
> > +	int rc;
> > +
> > +	if (!pos) {
> > +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
> > +		goto out;
> > +        } else if (!pos->file)
>    ^^^^^^^^
>     spaces instead of a tab

Thanks.  This code is different now.

julia

>
> -Frank
>
> > +		fname = "<no-file>";
> > +	else if (!pos->file->name)
> > +		fname = "<no-filename>";
> > +	else
> >  		fname = pos->file->name;
> >
> > -
> >  	if (pos->first_line != pos->last_line)
> > -		xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> > -			  pos->first_line, pos->first_column,
> > -			  pos->last_line, pos->last_column);
> > +		rc = xasprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
> > +			       pos->first_line, pos->first_column,
> > +			       pos->last_line, pos->last_column);
> >  	else if (pos->first_column != pos->last_column)
> > -		xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> > -			  pos->first_line, pos->first_column,
> > -			  pos->last_column);
> > +		rc = xasprintf(&pos_str, "%s:%d.%d-%d", fname,
> > +			       pos->first_line, pos->first_column,
> > +			       pos->last_column);
> >  	else
> > -		xasprintf(&pos_str, "%s:%d.%d", fname,
> > -			  pos->first_line, pos->first_column);
> > +		rc = xasprintf(&pos_str, "%s:%d.%d", fname,
> > +			       pos->first_line, pos->first_column);
> > +
> > +out:
> > +	if (rc == -1)
> > +		die("Couldn't allocate in srcpos string");
> >
> >  	return pos_str;
> >  }
> >
>
>

[PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

Builds on a patch proposed by Frank Rowand:

https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html

Added NULL position argument in a few new places in dtc-parser.tab.c (1)
and livetree.c (3).

For both '/' nodedef productions, include the '/' in the position.

Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
---
 dtc-parser.y | 23 +++++++++++++----------
 dtc.c        | 10 ++++++++++
 dtc.h        | 14 ++++++++++----
 flattree.c   |  2 +-
 fstree.c     |  8 +++++---
 livetree.c   | 43 ++++++++++++++++++++++++++++++-------------
 srcpos.c     | 35 +++++++++++++++++++++++++++++++++++
 srcpos.h     |  3 +++
 treesource.c | 38 +++++++++++++++++++++++++++++++++-----
 9 files changed, 140 insertions(+), 36 deletions(-)

diff --git a/dtc-parser.y b/dtc-parser.y
index 44af170..d668349 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -160,11 +160,11 @@ memreserve:
 devicetree:
 	  '/' nodedef
 		{
-			$$ = name_node($2, "");
+			$$ = name_node($2, "", &@$);
 		}
 	| devicetree '/' nodedef
 		{
-			$$ = merge_nodes($1, $3);
+			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
 		}
 	| DT_REF nodedef
 		{
@@ -175,7 +175,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, &@2);
 		}
 	| devicetree DT_LABEL DT_REF nodedef
 		{
@@ -183,7 +186,7 @@ devicetree:
 
 			if (target) {
 				add_label(&target->labels, $2);
-				merge_nodes(target, $4);
+				merge_nodes(target, $4, &@4);
 			} else
 				ERROR(&@3, "Label or path %s not found", $3);
 			$$ = $1;
@@ -193,7 +196,7 @@ devicetree:
 			struct node *target = get_node_by_ref($1, $2);
 
 			if (target) {
-				merge_nodes(target, $3);
+				merge_nodes(target, $3, &@3);
 			} else {
 				/*
 				 * We rely on the rule being always:
@@ -201,7 +204,7 @@ devicetree:
 				 * so $-1 is what we want (plugindecl)
 				 */
 				if ($<flags>-1 & DTSF_PLUGIN)
-					add_orphan_node($1, $3, $2);
+					add_orphan_node($1, $3, $2, &@3);
 				else
 					ERROR(&@2, "Label or path %s not found", $2);
 			}
@@ -242,11 +245,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 ';'
 		{
@@ -517,11 +520,11 @@ subnodes:
 subnode:
 	  DT_PROPNODENAME nodedef
 		{
-			$$ = name_node($2, $1);
+			$$ = name_node($2, $1, &@$);
 		}
 	| DT_DEL_NODE DT_PROPNODENAME ';'
 		{
-			$$ = name_node(build_node_delete(), $2);
+			$$ = name_node(build_node_delete(), $2, &@$);
 		}
 	| DT_LABEL subnode
 		{
diff --git a/dtc.c b/dtc.c
index c36994e..371d04c 100644
--- a/dtc.c
+++ b/dtc.c
@@ -35,6 +35,7 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
 int generate_symbols;	/* enable symbols & fixup support */
 int generate_fixups;		/* suppress generation of fixups on symbol support */
 int auto_label_aliases;		/* auto generate labels -> aliases */
+bool annotate = false; /* annotate .dts with input source location */
 
 static int is_power_of_2(int x)
 {
@@ -83,6 +84,7 @@ static struct option const usage_long_opts[] = {
 	{"auto-alias",       no_argument, NULL, 'A'},
 	{"help",             no_argument, NULL, 'h'},
 	{"version",          no_argument, NULL, 'v'},
+	{"annotate",         no_argument, NULL, 'T'},
 	{NULL,               no_argument, NULL, 0x0},
 };
 static const char * const usage_opts_help[] = {
@@ -116,6 +118,7 @@ static const char * const usage_opts_help[] = {
 	"\n\tEnable auto-alias of labels",
 	"\n\tPrint this help and exit",
 	"\n\tPrint version and exit",
+	"\n\tAnnotate output .dts with input source file and line",
 	NULL,
 };
 
@@ -185,6 +188,9 @@ int main(int argc, char *argv[])
 
 	while ((opt = util_getopt_long()) != EOF) {
 		switch (opt) {
+		case 'T':
+			annotate = true;
+			break;
 		case 'I':
 			inform = optarg;
 			break;
@@ -297,6 +303,10 @@ int main(int argc, char *argv[])
 				outform = "dts";
 		}
 	}
+
+	if (annotate && (!streq(inform, "dts") || !streq(outform, "dts")))
+		die("--annotate requires -I dts -O dts\n");
+
 	if (streq(inform, "dts"))
 		dti = dt_from_source(arg);
 	else if (streq(inform, "fs"))
diff --git a/dtc.h b/dtc.h
index 3b18a42..e7121ef 100644
--- a/dtc.h
+++ b/dtc.h
@@ -58,6 +58,7 @@ extern int phandle_format;	/* Use linux,phandle or phandle properties */
 extern int generate_symbols;	/* generate symbols for nodes with labels */
 extern int generate_fixups;	/* generate fixups */
 extern int auto_label_aliases;	/* auto generate labels -> aliases */
+extern bool annotate;		/* annotate .dts with input source location */
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
@@ -149,6 +150,7 @@ struct property {
 	struct property *next;
 
 	struct label *labels;
+	struct srcpos *srcpos;
 };
 
 struct node {
@@ -168,6 +170,7 @@ struct node {
 
 	struct label *labels;
 	const struct bus_type *bus;
+	struct srcpos *srcpos;
 };
 
 #define for_each_label_withdel(l0, l) \
@@ -194,17 +197,20 @@ 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 *name_node(struct node *node, char *name);
+struct node *name_node(struct node *node, char *name, struct srcpos *srcpos);
 struct node *chain_node(struct node *first, struct node *list);
-struct node *merge_nodes(struct node *old_node, struct node *new_node);
-struct node *add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
+struct node *merge_nodes(struct node *old_node, struct node *new_node,
+	struct srcpos *srcpos);
+struct node *add_orphan_node(struct node *old_node, struct node *new_node,
+	char *ref, struct srcpos *srcpos);
 
 void add_property(struct node *node, struct property *prop);
 void delete_property_by_name(struct node *node, char *name);
diff --git a/flattree.c b/flattree.c
index 8d268fb..f35ff5e 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);
 }
 
 
diff --git a/fstree.c b/fstree.c
index ae7d06c..da7e537 100644
--- a/fstree.c
+++ b/fstree.c
@@ -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);
 			}
@@ -68,7 +69,8 @@ static struct node *read_fstree(const char *dirname)
 			struct node *newchild;
 
 			newchild = read_fstree(tmpname);
-			newchild = name_node(newchild, xstrdup(de->d_name));
+			newchild = name_node(newchild, xstrdup(de->d_name),
+					     NULL);
 			add_child(tree, newchild);
 		}
 
@@ -84,7 +86,7 @@ struct dt_info *dt_from_fs(const char *dirname)
 	struct node *tree;
 
 	tree = read_fstree(dirname);
-	tree = name_node(tree, "");
+	tree = name_node(tree, "", NULL);
 
 	return build_dt_info(DTSF_V1, NULL, tree, guess_boot_cpuid(tree));
 }
diff --git a/livetree.c b/livetree.c
index 57b7db2..eb40c36 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_all(srcpos);
 
 	return new;
 }
@@ -125,16 +128,18 @@ struct node *build_node_delete(void)
 	return new;
 }
 
-struct node *name_node(struct node *node, char *name)
+struct node *name_node(struct node *node, char *name, struct srcpos *srcpos)
 {
 	assert(node->name == NULL);
 
 	node->name = name;
+	node->srcpos = srcpos_copy_all(srcpos);
 
 	return node;
 }
 
-struct node *merge_nodes(struct node *old_node, struct node *new_node)
+struct node *merge_nodes(struct node *old_node, struct node *new_node,
+			 struct srcpos *new_node_begin_srcpos)
 {
 	struct property *new_prop, *old_prop;
 	struct node *new_child, *old_child;
@@ -169,6 +174,7 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 
 				old_prop->val = new_prop->val;
 				old_prop->deleted = 0;
+				old_prop->srcpos = new_prop->srcpos;
 				free(new_prop);
 				new_prop = NULL;
 				break;
@@ -198,7 +204,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 		/* Search for a collision.  Merge if there is */
 		for_each_child_withdel(old_node, old_child) {
 			if (streq(old_child->name, new_child->name)) {
-				merge_nodes(old_child, new_child);
+				merge_nodes(old_child, new_child,
+					    new_child->srcpos);
 				new_child = 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_copy_all(new_node_begin_srcpos);
+
 	/* The new node contents are now merged into the old node.  Free
 	 * the new node. */
 	free(new_node);
@@ -216,7 +225,8 @@ 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,
+			      struct srcpos *srcpos)
 {
 	static unsigned int next_orphan_fragment = 0;
 	struct node *node;
@@ -227,13 +237,13 @@ 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, srcpos);
 
 	xasprintf(&name, "fragment@%u",
 			next_orphan_fragment++);
-	name_node(new_node, "__overlay__");
+	name_node(new_node, "__overlay__", srcpos);
 	node = build_node(p, new_node);
-	name_node(node, name);
+	name_node(node, name, srcpos);
 
 	add_child(dt, node);
 	return dt;
@@ -331,7 +341,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 +598,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
@@ -768,7 +783,7 @@ static struct node *build_and_name_child_node(struct node *parent, char *name)
 	struct node *node;
 
 	node = build_node(NULL, NULL);
-	name_node(node, xstrdup(name));
+	name_node(node, xstrdup(name), NULL);
 	add_child(parent, node);
 
 	return node;
@@ -827,9 +842,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 7f2626c..1a4db9c 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -246,6 +246,41 @@ srcpos_copy(struct srcpos *pos)
 	return pos_new;
 }
 
+struct srcpos *
+srcpos_copy_all(struct srcpos *pos)
+{
+	struct srcpos *pos_new;
+	struct srcfile_state *srcfile_state;
+
+	if (!pos)
+		return NULL;
+
+	pos_new = srcpos_copy(pos);
+
+	if (pos_new) {
+		/* 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;
+}
+
+struct srcpos *
+srcpos_combine(struct srcpos *left_srcpos, struct srcpos *right_srcpos)
+{
+	struct srcpos *pos_new;
+
+	pos_new = srcpos_copy(left_srcpos);
+
+	pos_new->last_line = right_srcpos->last_line;
+	pos_new->last_column = right_srcpos->last_column;
+
+	return pos_new;
+}
+
 char *
 srcpos_string(struct srcpos *pos)
 {
diff --git a/srcpos.h b/srcpos.h
index 9ded12a..ec69d89 100644
--- a/srcpos.h
+++ b/srcpos.h
@@ -105,6 +105,9 @@ 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_copy_all(struct srcpos *pos);
+extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
+	 struct srcpos *right_srcpos);
 extern char *srcpos_string(struct srcpos *pos);
 
 extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
diff --git a/treesource.c b/treesource.c
index 2461a3d..a99eca4 100644
--- a/treesource.c
+++ b/treesource.c
@@ -200,9 +200,16 @@ static void write_propval(FILE *f, struct property *prop)
 	int nnotstring = 0, nnul = 0;
 	int nnotstringlbl = 0, nnotcelllbl = 0;
 	int i;
+	char *srcstr;
 
 	if (len == 0) {
-		fprintf(f, ";\n");
+		if (annotate) {
+			srcstr = srcpos_string(prop->srcpos);
+			fprintf(f, "; /* %s */\n", srcstr);
+			free(srcstr);
+		} else {
+			fprintf(f, ";\n");
+		}
 		return;
 	}
 
@@ -230,7 +237,13 @@ static void write_propval(FILE *f, struct property *prop)
 		write_propval_bytes(f, prop->val);
 	}
 
-	fprintf(f, ";\n");
+	if (annotate) {
+		srcstr = srcpos_string(prop->srcpos);
+		fprintf(f, "; /* %s */\n", srcstr);
+		free(srcstr);
+	} else {
+		fprintf(f, ";\n");
+	}
 }
 
 static void write_tree_source_node(FILE *f, struct node *tree, int level)
@@ -238,14 +251,23 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 	struct property *prop;
 	struct node *child;
 	struct label *l;
+	char *srcstr;
 
 	write_prefix(f, level);
 	for_each_label(tree->labels, l)
 		fprintf(f, "%s: ", l->label);
 	if (tree->name && (*tree->name))
-		fprintf(f, "%s {\n", tree->name);
+		fprintf(f, "%s {", tree->name);
 	else
-		fprintf(f, "/ {\n");
+		fprintf(f, "/ {");
+
+	if (annotate) {
+		srcstr = srcpos_string(tree->srcpos);
+		fprintf(f, " /* %s */\n", srcstr);
+		free(srcstr);
+	} else {
+		fprintf(f, "\n");
+	}
 
 	for_each_property(tree, prop) {
 		write_prefix(f, level+1);
@@ -259,7 +281,13 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 		write_tree_source_node(f, child, level+1);
 	}
 	write_prefix(f, level);
-	fprintf(f, "};\n");
+	if (annotate) {
+		srcstr = srcpos_string(tree->srcpos);
+		fprintf(f, "}; /* %s */\n", srcstr);
+		free(srcstr);
+	} else {
+		fprintf(f, "};\n");
+	}
 }
 
 
-- 
2.7.4

Re: [PATCH 2/5] annotations: Add position information to various calls

[Rob Herring]

On Mon, Jan 8, 2018 at 7:36 AM, Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org> wrote:
> Builds on a patch proposed by Frank Rowand:
>
> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html

Better to just put the details here and keep Frank's S-o-b if
significant parts he wrote.

> Added NULL position argument in a few new places in dtc-parser.tab.c (1)
> and livetree.c (3).

I think this patch should probably be split into what's needed to keep
track of the source position and the annotating dts files part. Mainly
I say this because I'm interested in changing the checks to print the
dts position (if available) instead of the output file.

The changes themselves look good to me.

Rob

Re: [PATCH 2/5] annotations: Add position information to various calls

[Frank Rowand]

On 01/08/18 14:29, Rob Herring wrote:
> On Mon, Jan 8, 2018 at 7:36 AM, Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org> wrote:
>> Builds on a patch proposed by Frank Rowand:
>>
>> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
> 
> Better to just put the details here and keep Frank's S-o-b if
> significant parts he wrote.

If you keep my S-o-b, please update it with my current email:

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

> 
>> Added NULL position argument in a few new places in dtc-parser.tab.c (1)
>> and livetree.c (3).
> 
> I think this patch should probably be split into what's needed to keep
> track of the source position and the annotating dts files part. Mainly
> I say this because I'm interested in changing the checks to print the
> dts position (if available) instead of the output file.
> 
> The changes themselves look good to me.
> 
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

On Mon, 8 Jan 2018, Rob Herring wrote:

> On Mon, Jan 8, 2018 at 7:36 AM, Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org> wrote:
> > Builds on a patch proposed by Frank Rowand:
> >
> > https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
>
> Better to just put the details here and keep Frank's S-o-b if
> significant parts he wrote.
>
> > Added NULL position argument in a few new places in dtc-parser.tab.c (1)
> > and livetree.c (3).
>
> I think this patch should probably be split into what's needed to keep
> track of the source position and the annotating dts files part. Mainly
> I say this because I'm interested in changing the checks to print the
> dts position (if available) instead of the output file.

Thanks for the feedback.  I'm not sure to understand the last sentence.  I
thought the code was printing the original position of things?

I'll take care of the SOB as Frank suggested in the next version.

julia

>
> The changes themselves look good to me.
>
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/5] annotations: Add position information to various calls

[Rob Herring]

On Tue, Jan 9, 2018 at 12:16 AM, Julia Lawall <julia.lawall-L2FTfq7BK8M@public.gmane.org> wrote:
>
>
> On Mon, 8 Jan 2018, Rob Herring wrote:
>
>> On Mon, Jan 8, 2018 at 7:36 AM, Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org> wrote:
>> > Builds on a patch proposed by Frank Rowand:
>> >
>> > https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
>>
>> Better to just put the details here and keep Frank's S-o-b if
>> significant parts he wrote.
>>
>> > Added NULL position argument in a few new places in dtc-parser.tab.c (1)
>> > and livetree.c (3).
>>
>> I think this patch should probably be split into what's needed to keep
>> track of the source position and the annotating dts files part. Mainly
>> I say this because I'm interested in changing the checks to print the
>> dts position (if available) instead of the output file.
>
> Thanks for the feedback.  I'm not sure to understand the last sentence.  I
> thought the code was printing the original position of things?

No, the information was lost by the time checks run as they only have
livetree node and prop structs. See my patch I Cc'ed you on. Currently
the checks just print the dtb file with no position information though
typically they print the full node path. You have to manually find the
node in the source files which is not exact as you can only really
search for the basename.

Rob

Re: [PATCH 2/5] annotations: Add position information to various calls

[David Gibson]

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

On Mon, Jan 08, 2018 at 02:36:44PM +0100, Julia Lawall wrote:
> Builds on a patch proposed by Frank Rowand:
> 
> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
> 
> Added NULL position argument in a few new places in dtc-parser.tab.c (1)
> and livetree.c (3).
> 
> For both '/' nodedef productions, include the '/' in the position.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> ---
>  dtc-parser.y | 23 +++++++++++++----------
>  dtc.c        | 10 ++++++++++
>  dtc.h        | 14 ++++++++++----
>  flattree.c   |  2 +-
>  fstree.c     |  8 +++++---
>  livetree.c   | 43 ++++++++++++++++++++++++++++++-------------
>  srcpos.c     | 35 +++++++++++++++++++++++++++++++++++
>  srcpos.h     |  3 +++
>  treesource.c | 38 +++++++++++++++++++++++++++++++++-----
>  9 files changed, 140 insertions(+), 36 deletions(-)
> 
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 44af170..d668349 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -160,11 +160,11 @@ memreserve:
>  devicetree:
>  	  '/' nodedef
>  		{
> -			$$ = name_node($2, "");
> +			$$ = name_node($2, "", &@$);
>  		}
>  	| devicetree '/' nodedef
>  		{
> -			$$ = merge_nodes($1, $3);
> +			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
>  		}
>  	| DT_REF nodedef
>  		{
> @@ -175,7 +175,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, &@2);
>  		}
>  	| devicetree DT_LABEL DT_REF nodedef
>  		{
> @@ -183,7 +186,7 @@ devicetree:
>  
>  			if (target) {
>  				add_label(&target->labels, $2);
> -				merge_nodes(target, $4);
> +				merge_nodes(target, $4, &@4);
>  			} else
>  				ERROR(&@3, "Label or path %s not found", $3);
>  			$$ = $1;
> @@ -193,7 +196,7 @@ devicetree:
>  			struct node *target = get_node_by_ref($1, $2);
>  
>  			if (target) {
> -				merge_nodes(target, $3);
> +				merge_nodes(target, $3, &@3);
>  			} else {
>  				/*
>  				 * We rely on the rule being always:
> @@ -201,7 +204,7 @@ devicetree:
>  				 * so $-1 is what we want (plugindecl)
>  				 */
>  				if ($<flags>-1 & DTSF_PLUGIN)
> -					add_orphan_node($1, $3, $2);
> +					add_orphan_node($1, $3, $2, &@3);
>  				else
>  					ERROR(&@2, "Label or path %s not found", $2);
>  			}
> @@ -242,11 +245,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 ';'
>  		{
> @@ -517,11 +520,11 @@ subnodes:
>  subnode:
>  	  DT_PROPNODENAME nodedef
>  		{
> -			$$ = name_node($2, $1);
> +			$$ = name_node($2, $1, &@$);
>  		}
>  	| DT_DEL_NODE DT_PROPNODENAME ';'
>  		{
> -			$$ = name_node(build_node_delete(), $2);
> +			$$ = name_node(build_node_delete(), $2, &@$);
>  		}
>  	| DT_LABEL subnode
>  		{
> diff --git a/dtc.c b/dtc.c
> index c36994e..371d04c 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -35,6 +35,7 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
>  int generate_symbols;	/* enable symbols & fixup support */
>  int generate_fixups;		/* suppress generation of fixups on symbol support */
>  int auto_label_aliases;		/* auto generate labels -> aliases */
> +bool annotate = false; /* annotate .dts with input source location */
>  
>  static int is_power_of_2(int x)
>  {
> @@ -83,6 +84,7 @@ static struct option const usage_long_opts[] = {
>  	{"auto-alias",       no_argument, NULL, 'A'},
>  	{"help",             no_argument, NULL, 'h'},
>  	{"version",          no_argument, NULL, 'v'},
> +	{"annotate",         no_argument, NULL, 'T'},
>  	{NULL,               no_argument, NULL, 0x0},
>  };
>  static const char * const usage_opts_help[] = {
> @@ -116,6 +118,7 @@ static const char * const usage_opts_help[] = {
>  	"\n\tEnable auto-alias of labels",
>  	"\n\tPrint this help and exit",
>  	"\n\tPrint version and exit",
> +	"\n\tAnnotate output .dts with input source file and line",
>  	NULL,
>  };
>  
> @@ -185,6 +188,9 @@ int main(int argc, char *argv[])
>  
>  	while ((opt = util_getopt_long()) != EOF) {
>  		switch (opt) {
> +		case 'T':
> +			annotate = true;
> +			break;
>  		case 'I':
>  			inform = optarg;
>  			break;
> @@ -297,6 +303,10 @@ int main(int argc, char *argv[])
>  				outform = "dts";
>  		}
>  	}
> +
> +	if (annotate && (!streq(inform, "dts") || !streq(outform, "dts")))
> +		die("--annotate requires -I dts -O dts\n");
> +
>  	if (streq(inform, "dts"))
>  		dti = dt_from_source(arg);
>  	else if (streq(inform, "fs"))
> diff --git a/dtc.h b/dtc.h
> index 3b18a42..e7121ef 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -58,6 +58,7 @@ extern int phandle_format;	/* Use linux,phandle or phandle properties */
>  extern int generate_symbols;	/* generate symbols for nodes with labels */
>  extern int generate_fixups;	/* generate fixups */
>  extern int auto_label_aliases;	/* auto generate labels -> aliases */
> +extern bool annotate;		/* annotate .dts with input source location */
>  
>  #define PHANDLE_LEGACY	0x1
>  #define PHANDLE_EPAPR	0x2
> @@ -149,6 +150,7 @@ struct property {
>  	struct property *next;
>  
>  	struct label *labels;
> +	struct srcpos *srcpos;
>  };
>  
>  struct node {
> @@ -168,6 +170,7 @@ struct node {
>  
>  	struct label *labels;
>  	const struct bus_type *bus;
> +	struct srcpos *srcpos;
>  };
>  
>  #define for_each_label_withdel(l0, l) \
> @@ -194,17 +197,20 @@ 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 *name_node(struct node *node, char *name);
> +struct node *name_node(struct node *node, char *name, struct srcpos *srcpos);
>  struct node *chain_node(struct node *first, struct node *list);
> -struct node *merge_nodes(struct node *old_node, struct node *new_node);
> -struct node *add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
> +struct node *merge_nodes(struct node *old_node, struct node *new_node,
> +	struct srcpos *srcpos);
> +struct node *add_orphan_node(struct node *old_node, struct node *new_node,
> +	char *ref, struct srcpos *srcpos);
>  
>  void add_property(struct node *node, struct property *prop);
>  void delete_property_by_name(struct node *node, char *name);
> diff --git a/flattree.c b/flattree.c
> index 8d268fb..f35ff5e 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);
>  }
>  
>  
> diff --git a/fstree.c b/fstree.c
> index ae7d06c..da7e537 100644
> --- a/fstree.c
> +++ b/fstree.c
> @@ -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);
>  			}
> @@ -68,7 +69,8 @@ static struct node *read_fstree(const char *dirname)
>  			struct node *newchild;
>  
>  			newchild = read_fstree(tmpname);
> -			newchild = name_node(newchild, xstrdup(de->d_name));
> +			newchild = name_node(newchild, xstrdup(de->d_name),
> +					     NULL);
>  			add_child(tree, newchild);
>  		}
>  
> @@ -84,7 +86,7 @@ struct dt_info *dt_from_fs(const char *dirname)
>  	struct node *tree;
>  
>  	tree = read_fstree(dirname);
> -	tree = name_node(tree, "");
> +	tree = name_node(tree, "", NULL);
>  
>  	return build_dt_info(DTSF_V1, NULL, tree, guess_boot_cpuid(tree));
>  }
> diff --git a/livetree.c b/livetree.c
> index 57b7db2..eb40c36 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_all(srcpos);
>  
>  	return new;
>  }
> @@ -125,16 +128,18 @@ struct node *build_node_delete(void)
>  	return new;
>  }
>  
> -struct node *name_node(struct node *node, char *name)
> +struct node *name_node(struct node *node, char *name, struct srcpos *srcpos)
>  {
>  	assert(node->name == NULL);
>  
>  	node->name = name;
> +	node->srcpos = srcpos_copy_all(srcpos);
>  
>  	return node;
>  }
>  
> -struct node *merge_nodes(struct node *old_node, struct node *new_node)
> +struct node *merge_nodes(struct node *old_node, struct node *new_node,
> +			 struct srcpos *new_node_begin_srcpos)
>  {
>  	struct property *new_prop, *old_prop;
>  	struct node *new_child, *old_child;
> @@ -169,6 +174,7 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  
>  				old_prop->val = new_prop->val;
>  				old_prop->deleted = 0;
> +				old_prop->srcpos = new_prop->srcpos;
>  				free(new_prop);
>  				new_prop = NULL;
>  				break;
> @@ -198,7 +204,8 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  		/* Search for a collision.  Merge if there is */
>  		for_each_child_withdel(old_node, old_child) {
>  			if (streq(old_child->name, new_child->name)) {
> -				merge_nodes(old_child, new_child);
> +				merge_nodes(old_child, new_child,
> +					    new_child->srcpos);
>  				new_child = 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_copy_all(new_node_begin_srcpos);

This doesn't seem right.  Replacing the old position with the new
makes sense for indivudal properties where the whole value is also
replaced.  But for nodes we really need to track both locations.

I think the extra complexity here is why I didn't add this tracking
earlier.

>  	/* The new node contents are now merged into the old node.  Free
>  	 * the new node. */
>  	free(new_node);
> @@ -216,7 +225,8 @@ 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,
> +			      struct srcpos *srcpos)
>  {
>  	static unsigned int next_orphan_fragment = 0;
>  	struct node *node;
> @@ -227,13 +237,13 @@ 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, srcpos);
>  
>  	xasprintf(&name, "fragment@%u",
>  			next_orphan_fragment++);
> -	name_node(new_node, "__overlay__");
> +	name_node(new_node, "__overlay__", srcpos);
>  	node = build_node(p, new_node);
> -	name_node(node, name);
> +	name_node(node, name, srcpos);
>  
>  	add_child(dt, node);
>  	return dt;
> @@ -331,7 +341,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 +598,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
> @@ -768,7 +783,7 @@ static struct node *build_and_name_child_node(struct node *parent, char *name)
>  	struct node *node;
>  
>  	node = build_node(NULL, NULL);
> -	name_node(node, xstrdup(name));
> +	name_node(node, xstrdup(name), NULL);
>  	add_child(parent, node);
>  
>  	return node;
> @@ -827,9 +842,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 7f2626c..1a4db9c 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -246,6 +246,41 @@ srcpos_copy(struct srcpos *pos)
>  	return pos_new;
>  }
>  
> +struct srcpos *
> +srcpos_copy_all(struct srcpos *pos)
> +{
> +	struct srcpos *pos_new;
> +	struct srcfile_state *srcfile_state;
> +
> +	if (!pos)
> +		return NULL;
> +
> +	pos_new = srcpos_copy(pos);
> +
> +	if (pos_new) {
> +		/* 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;
> +}

I don't really see a reason we'd need both a deep and a shallow copy.
If you need a deep copy, I'd suggest just changing srcpos_copy() to do
that.

> +struct srcpos *
> +srcpos_combine(struct srcpos *left_srcpos, struct srcpos *right_srcpos)
> +{
> +	struct srcpos *pos_new;
> +
> +	pos_new = srcpos_copy(left_srcpos);
> +
> +	pos_new->last_line = right_srcpos->last_line;
> +	pos_new->last_column = right_srcpos->last_column;
> +
> +	return pos_new;
> +}
> +
>  char *
>  srcpos_string(struct srcpos *pos)
>  {
> diff --git a/srcpos.h b/srcpos.h
> index 9ded12a..ec69d89 100644
> --- a/srcpos.h
> +++ b/srcpos.h
> @@ -105,6 +105,9 @@ 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_copy_all(struct srcpos *pos);
> +extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
> +	 struct srcpos *right_srcpos);
>  extern char *srcpos_string(struct srcpos *pos);
>  
>  extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
> diff --git a/treesource.c b/treesource.c
> index 2461a3d..a99eca4 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -200,9 +200,16 @@ static void write_propval(FILE *f, struct property *prop)
>  	int nnotstring = 0, nnul = 0;
>  	int nnotstringlbl = 0, nnotcelllbl = 0;
>  	int i;
> +	char *srcstr;
>  
>  	if (len == 0) {
> -		fprintf(f, ";\n");
> +		if (annotate) {
> +			srcstr = srcpos_string(prop->srcpos);
> +			fprintf(f, "; /* %s */\n", srcstr);
> +			free(srcstr);
> +		} else {
> +			fprintf(f, ";\n");
> +		}
>  		return;
>  	}
>  
> @@ -230,7 +237,13 @@ static void write_propval(FILE *f, struct property *prop)
>  		write_propval_bytes(f, prop->val);
>  	}
>  
> -	fprintf(f, ";\n");
> +	if (annotate) {
> +		srcstr = srcpos_string(prop->srcpos);
> +		fprintf(f, "; /* %s */\n", srcstr);
> +		free(srcstr);
> +	} else {
> +		fprintf(f, ";\n");
> +	}
>  }
>  
>  static void write_tree_source_node(FILE *f, struct node *tree, int level)
> @@ -238,14 +251,23 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  	struct property *prop;
>  	struct node *child;
>  	struct label *l;
> +	char *srcstr;
>  
>  	write_prefix(f, level);
>  	for_each_label(tree->labels, l)
>  		fprintf(f, "%s: ", l->label);
>  	if (tree->name && (*tree->name))
> -		fprintf(f, "%s {\n", tree->name);
> +		fprintf(f, "%s {", tree->name);
>  	else
> -		fprintf(f, "/ {\n");
> +		fprintf(f, "/ {");
> +
> +	if (annotate) {
> +		srcstr = srcpos_string(tree->srcpos);
> +		fprintf(f, " /* %s */\n", srcstr);
> +		free(srcstr);
> +	} else {
> +		fprintf(f, "\n");
> +	}
>  
>  	for_each_property(tree, prop) {
>  		write_prefix(f, level+1);
> @@ -259,7 +281,13 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  		write_tree_source_node(f, child, level+1);
>  	}
>  	write_prefix(f, level);
> -	fprintf(f, "};\n");
> +	if (annotate) {
> +		srcstr = srcpos_string(tree->srcpos);
> +		fprintf(f, "}; /* %s */\n", srcstr);
> +		free(srcstr);
> +	} else {
> +		fprintf(f, "};\n");
> +	}
>  }
>  
>  

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

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

> > +struct srcpos *
> > +srcpos_copy_all(struct srcpos *pos)
> > +{
> > +	struct srcpos *pos_new;
> > +	struct srcfile_state *srcfile_state;
> > +
> > +	if (!pos)
> > +		return NULL;
> > +
> > +	pos_new = srcpos_copy(pos);
> > +
> > +	if (pos_new) {
> > +		/* 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;
> > +}
>
> I don't really see a reason we'd need both a deep and a shallow copy.
> If you need a deep copy, I'd suggest just changing srcpos_copy() to do
> that.

The deep copy is needed due to the treatment of #includes.  The following
function overwrites the file name information:

void srcpos_set_line(char *f, int l)
{
        current_srcfile->name = f;
        current_srcfile->lineno = l;
}

srcpos_combine doesn't need to use the deep copy, because the result gets
copied again.  Maybe this is not a big issue or there is a better way to
solve this.

julia

Re: [PATCH 2/5] annotations: Add position information to various calls

[David Gibson]

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

On Tue, Jan 09, 2018 at 01:27:48PM +0100, Julia Lawall wrote:
> > > +struct srcpos *
> > > +srcpos_copy_all(struct srcpos *pos)
> > > +{
> > > +	struct srcpos *pos_new;
> > > +	struct srcfile_state *srcfile_state;
> > > +
> > > +	if (!pos)
> > > +		return NULL;
> > > +
> > > +	pos_new = srcpos_copy(pos);
> > > +
> > > +	if (pos_new) {
> > > +		/* 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;
> > > +}
> >
> > I don't really see a reason we'd need both a deep and a shallow copy.
> > If you need a deep copy, I'd suggest just changing srcpos_copy() to do
> > that.
> 
> The deep copy is needed due to the treatment of #includes.  The following
> function overwrites the file name information:
> 
> void srcpos_set_line(char *f, int l)
> {
>         current_srcfile->name = f;
>         current_srcfile->lineno = l;
> }
> 
> srcpos_combine doesn't need to use the deep copy, because the result gets
> copied again.  Maybe this is not a big issue or there is a better way to
> solve this.

Well, dtbs are generally so small that performance really isn't an
issue for dtc.  So I think simplifying the code to only have one
(deep) copy routine is worth more than the minor speedup from avoiding
deep copies in some places they're not necessary.

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

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

On Wed, 10 Jan 2018, David Gibson wrote:

> On Tue, Jan 09, 2018 at 01:27:48PM +0100, Julia Lawall wrote:
> > > > +struct srcpos *
> > > > +srcpos_copy_all(struct srcpos *pos)
> > > > +{
> > > > +	struct srcpos *pos_new;
> > > > +	struct srcfile_state *srcfile_state;
> > > > +
> > > > +	if (!pos)
> > > > +		return NULL;
> > > > +
> > > > +	pos_new = srcpos_copy(pos);
> > > > +
> > > > +	if (pos_new) {
> > > > +		/* 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;
> > > > +}
> > >
> > > I don't really see a reason we'd need both a deep and a shallow copy.
> > > If you need a deep copy, I'd suggest just changing srcpos_copy() to do
> > > that.
> >
> > The deep copy is needed due to the treatment of #includes.  The following
> > function overwrites the file name information:
> >
> > void srcpos_set_line(char *f, int l)
> > {
> >         current_srcfile->name = f;
> >         current_srcfile->lineno = l;
> > }
> >
> > srcpos_combine doesn't need to use the deep copy, because the result gets
> > copied again.  Maybe this is not a big issue or there is a better way to
> > solve this.
>
> Well, dtbs are generally so small that performance really isn't an
> issue for dtc.  So I think simplifying the code to only have one
> (deep) copy routine is worth more than the minor speedup from avoiding
> deep copies in some places they're not necessary.

OK, I will do this.

julia

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

> >  			add_child(old_node, new_child);
> >  	}
> >
> > +	old_node->srcpos = srcpos_copy_all(new_node_begin_srcpos);
>
> This doesn't seem right.  Replacing the old position with the new
> makes sense for indivudal properties where the whole value is also
> replaced.  But for nodes we really need to track both locations.
>
> I think the extra complexity here is why I didn't add this tracking
> earlier.

I've added a next field to be able to make a list of positions.  Here, I
append the lists, with the new ones at the front.

julia

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

> > +	old_node->srcpos = srcpos_copy_all(new_node_begin_srcpos);
>
> This doesn't seem right.  Replacing the old position with the new
> makes sense for indivudal properties where the whole value is also
> replaced.  But for nodes we really need to track both locations.
>
> I think the extra complexity here is why I didn't add this tracking
> earlier.

I have the following example:

/dts-v1/;

/ {
        #address-cells = < 1 >;
        #size-cells = < 1 >;

        tree_1: soc@0 {
        	reg = <0x0 0x0>;
        };

        foo: foo_node {
                prop_1: added-prop = <0x99>;
        };

        bar_node {
                added-prop = <0x77>;
        };

};

/include/ "test_b1.dts"
/include/ "test_c1.dts"

-------------------

Then test_b1.dts and test_c1.dts are both /include/ "test_d.dts".  And
test_d.dts is:

/ {
        foo_node {
                added-prop = <0x1 0x2 0x3>;
        };

        foo_node {
                added-prop = <0x1 0x2 0x3>;
        };

        bar: bar_node {
                added-prop = <0x5>;
        };
};

----------

The point of the example is that via the includes there are tw ways to
reach test_d.dts.

Accordingly, by accumulating a list of positions in merge_nodes, I end up with:

/dts-v1/;

/ { /* tests/test_d.dts:1, tests/test_d.dts:1, tests/test_a.dts:10 */
	#address-cells = <0x1>; /* tests/test_a.dts:11 */
	#size-cells = <0x1>; /* tests/test_a.dts:12 */

	tree_1: soc@0 { /* tests/test_a.dts:14 */
		reg = <0x0 0x0>; /* tests/test_a.dts:15 */
	}; /* tests/test_a.dts:16 */

	foo: foo_node { /* tests/test_d.dts:6, tests/test_d.dts:2, tests/test_d.dts:6, tests/test_d.dts:2, tests/test_a.dts:18 */
		prop_1: added-prop = <0x1 0x2 0x3>; /* tests/test_d.dts:7 */
	}; /* tests/test_d.dts:8, tests/test_d.dts:4, tests/test_d.dts:8, tests/test_d.dts:4, tests/test_a.dts:20 */

	bar: bar_node { /* tests/test_d.dts:10, tests/test_d.dts:10, tests/test_a.dts:22 */
		added-prop = <0x5>; /* tests/test_d.dts:11 */
	}; /* tests/test_d.dts:12, tests/test_d.dts:12, tests/test_a.dts:24 */
}; /* tests/test_d.dts:13, tests/test_d.dts:13, tests/test_a.dts:26 */

--------

For example, in the last line, we see tests/test_d.dts:13 twice, because it
is merged in twice.  Is this what is wanted?  Should it be there only once?
Should there be some indication on how tests/test_d.dts:13 is reached?
This is a fake example, but I saw some examples with duplicated positions
in the Linux kernel code.

julia

Re: [PATCH 2/5] annotations: Add position information to various calls

[David Gibson]

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

On Thu, Jan 11, 2018 at 04:21:44PM +0100, Julia Lawall wrote:
> > > +	old_node->srcpos = srcpos_copy_all(new_node_begin_srcpos);
> >
> > This doesn't seem right.  Replacing the old position with the new
> > makes sense for indivudal properties where the whole value is also
> > replaced.  But for nodes we really need to track both locations.
> >
> > I think the extra complexity here is why I didn't add this tracking
> > earlier.
> 
> I have the following example:
> 
> /dts-v1/;
> 
> / {
>         #address-cells = < 1 >;
>         #size-cells = < 1 >;
> 
>         tree_1: soc@0 {
>         	reg = <0x0 0x0>;
>         };
> 
>         foo: foo_node {
>                 prop_1: added-prop = <0x99>;
>         };
> 
>         bar_node {
>                 added-prop = <0x77>;
>         };
> 
> };
> 
> /include/ "test_b1.dts"
> /include/ "test_c1.dts"
> 
> -------------------
> 
> Then test_b1.dts and test_c1.dts are both /include/ "test_d.dts".  And
> test_d.dts is:
> 
> / {
>         foo_node {
>                 added-prop = <0x1 0x2 0x3>;
>         };
> 
>         foo_node {
>                 added-prop = <0x1 0x2 0x3>;
>         };
> 
>         bar: bar_node {
>                 added-prop = <0x5>;
>         };
> };
> 
> ----------
> 
> The point of the example is that via the includes there are tw ways to
> reach test_d.dts.
> 
> Accordingly, by accumulating a list of positions in merge_nodes, I end up with:
> 
> /dts-v1/;
> 
> / { /* tests/test_d.dts:1, tests/test_d.dts:1, tests/test_a.dts:10 */
> 	#address-cells = <0x1>; /* tests/test_a.dts:11 */
> 	#size-cells = <0x1>; /* tests/test_a.dts:12 */
> 
> 	tree_1: soc@0 { /* tests/test_a.dts:14 */
> 		reg = <0x0 0x0>; /* tests/test_a.dts:15 */
> 	}; /* tests/test_a.dts:16 */
> 
> 	foo: foo_node { /* tests/test_d.dts:6, tests/test_d.dts:2, tests/test_d.dts:6, tests/test_d.dts:2, tests/test_a.dts:18 */
> 		prop_1: added-prop = <0x1 0x2 0x3>; /* tests/test_d.dts:7 */
> 	}; /* tests/test_d.dts:8, tests/test_d.dts:4, tests/test_d.dts:8, tests/test_d.dts:4, tests/test_a.dts:20 */
> 
> 	bar: bar_node { /* tests/test_d.dts:10, tests/test_d.dts:10, tests/test_a.dts:22 */
> 		added-prop = <0x5>; /* tests/test_d.dts:11 */
> 	}; /* tests/test_d.dts:12, tests/test_d.dts:12, tests/test_a.dts:24 */
> }; /* tests/test_d.dts:13, tests/test_d.dts:13, tests/test_a.dts:26 */
> 
> --------
> 
> For example, in the last line, we see tests/test_d.dts:13 twice, because it
> is merged in twice.  Is this what is wanted?  Should it be there only once?
> Should there be some indication on how tests/test_d.dts:13 is reached?
> This is a fake example, but I saw some examples with duplicated positions
> in the Linux kernel code.

Right, that's ugly, but it's not that easy to fix.  Really we'd need
to merge/overlap each element in the list to accumulate them.  Going
back to the original proposal isn't a solution though because of cases
like this, which will be much more common that duplicated includes:

/ {
	foo {
		compatible = "...";
		reg = < .. >;
		ranges = < ... >;
		lots = ...;
		of = ...;
		other = ...;
		properties = ...;
	};
}

&{/foo} {
	one-tiny-change = "yes";
};

The original proposal would annotate the output with *only* the
location of the one-tiny-change, which is extremely misleading.  Much
worse than duplicated locations.		

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

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

On Mon, 15 Jan 2018, David Gibson wrote:

> On Thu, Jan 11, 2018 at 04:21:44PM +0100, Julia Lawall wrote:
> > > > +	old_node->srcpos = srcpos_copy_all(new_node_begin_srcpos);
> > >
> > > This doesn't seem right.  Replacing the old position with the new
> > > makes sense for indivudal properties where the whole value is also
> > > replaced.  But for nodes we really need to track both locations.
> > >
> > > I think the extra complexity here is why I didn't add this tracking
> > > earlier.
> >
> > I have the following example:
> >
> > /dts-v1/;
> >
> > / {
> >         #address-cells = < 1 >;
> >         #size-cells = < 1 >;
> >
> >         tree_1: soc@0 {
> >         	reg = <0x0 0x0>;
> >         };
> >
> >         foo: foo_node {
> >                 prop_1: added-prop = <0x99>;
> >         };
> >
> >         bar_node {
> >                 added-prop = <0x77>;
> >         };
> >
> > };
> >
> > /include/ "test_b1.dts"
> > /include/ "test_c1.dts"
> >
> > -------------------
> >
> > Then test_b1.dts and test_c1.dts are both /include/ "test_d.dts".  And
> > test_d.dts is:
> >
> > / {
> >         foo_node {
> >                 added-prop = <0x1 0x2 0x3>;
> >         };
> >
> >         foo_node {
> >                 added-prop = <0x1 0x2 0x3>;
> >         };
> >
> >         bar: bar_node {
> >                 added-prop = <0x5>;
> >         };
> > };
> >
> > ----------
> >
> > The point of the example is that via the includes there are tw ways to
> > reach test_d.dts.
> >
> > Accordingly, by accumulating a list of positions in merge_nodes, I end up with:
> >
> > /dts-v1/;
> >
> > / { /* tests/test_d.dts:1, tests/test_d.dts:1, tests/test_a.dts:10 */
> > 	#address-cells = <0x1>; /* tests/test_a.dts:11 */
> > 	#size-cells = <0x1>; /* tests/test_a.dts:12 */
> >
> > 	tree_1: soc@0 { /* tests/test_a.dts:14 */
> > 		reg = <0x0 0x0>; /* tests/test_a.dts:15 */
> > 	}; /* tests/test_a.dts:16 */
> >
> > 	foo: foo_node { /* tests/test_d.dts:6, tests/test_d.dts:2, tests/test_d.dts:6, tests/test_d.dts:2, tests/test_a.dts:18 */
> > 		prop_1: added-prop = <0x1 0x2 0x3>; /* tests/test_d.dts:7 */
> > 	}; /* tests/test_d.dts:8, tests/test_d.dts:4, tests/test_d.dts:8, tests/test_d.dts:4, tests/test_a.dts:20 */
> >
> > 	bar: bar_node { /* tests/test_d.dts:10, tests/test_d.dts:10, tests/test_a.dts:22 */
> > 		added-prop = <0x5>; /* tests/test_d.dts:11 */
> > 	}; /* tests/test_d.dts:12, tests/test_d.dts:12, tests/test_a.dts:24 */
> > }; /* tests/test_d.dts:13, tests/test_d.dts:13, tests/test_a.dts:26 */
> >
> > --------
> >
> > For example, in the last line, we see tests/test_d.dts:13 twice, because it
> > is merged in twice.  Is this what is wanted?  Should it be there only once?
> > Should there be some indication on how tests/test_d.dts:13 is reached?
> > This is a fake example, but I saw some examples with duplicated positions
> > in the Linux kernel code.
>
> Right, that's ugly, but it's not that easy to fix.  Really we'd need
> to merge/overlap each element in the list to accumulate them.  Going
> back to the original proposal isn't a solution though because of cases
> like this, which will be much more common that duplicated includes:
>
> / {
> 	foo {
> 		compatible = "...";
> 		reg = < .. >;
> 		ranges = < ... >;
> 		lots = ...;
> 		of = ...;
> 		other = ...;
> 		properties = ...;
> 	};
> }
>
> &{/foo} {
> 	one-tiny-change = "yes";
> };
>
> The original proposal would annotate the output with *only* the
> location of the one-tiny-change, which is extremely misleading.  Much
> worse than duplicated locations.

It's only the label on the braces that is affected.  compatible, reg, etc
should still have their original locations.

I can remove the duplicates, if that would be beneficial.

julia

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

Re: [PATCH 2/5] annotations: Add position information to various calls

[David Gibson]

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

On Mon, Jan 15, 2018 at 10:28:38AM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 15 Jan 2018, David Gibson wrote:
> 
> > On Thu, Jan 11, 2018 at 04:21:44PM +0100, Julia Lawall wrote:
> > > > > +	old_node->srcpos = srcpos_copy_all(new_node_begin_srcpos);
> > > >
> > > > This doesn't seem right.  Replacing the old position with the new
> > > > makes sense for indivudal properties where the whole value is also
> > > > replaced.  But for nodes we really need to track both locations.
> > > >
> > > > I think the extra complexity here is why I didn't add this tracking
> > > > earlier.
> > >
> > > I have the following example:
> > >
> > > /dts-v1/;
> > >
> > > / {
> > >         #address-cells = < 1 >;
> > >         #size-cells = < 1 >;
> > >
> > >         tree_1: soc@0 {
> > >         	reg = <0x0 0x0>;
> > >         };
> > >
> > >         foo: foo_node {
> > >                 prop_1: added-prop = <0x99>;
> > >         };
> > >
> > >         bar_node {
> > >                 added-prop = <0x77>;
> > >         };
> > >
> > > };
> > >
> > > /include/ "test_b1.dts"
> > > /include/ "test_c1.dts"
> > >
> > > -------------------
> > >
> > > Then test_b1.dts and test_c1.dts are both /include/ "test_d.dts".  And
> > > test_d.dts is:
> > >
> > > / {
> > >         foo_node {
> > >                 added-prop = <0x1 0x2 0x3>;
> > >         };
> > >
> > >         foo_node {
> > >                 added-prop = <0x1 0x2 0x3>;
> > >         };
> > >
> > >         bar: bar_node {
> > >                 added-prop = <0x5>;
> > >         };
> > > };
> > >
> > > ----------
> > >
> > > The point of the example is that via the includes there are tw ways to
> > > reach test_d.dts.
> > >
> > > Accordingly, by accumulating a list of positions in merge_nodes, I end up with:
> > >
> > > /dts-v1/;
> > >
> > > / { /* tests/test_d.dts:1, tests/test_d.dts:1, tests/test_a.dts:10 */
> > > 	#address-cells = <0x1>; /* tests/test_a.dts:11 */
> > > 	#size-cells = <0x1>; /* tests/test_a.dts:12 */
> > >
> > > 	tree_1: soc@0 { /* tests/test_a.dts:14 */
> > > 		reg = <0x0 0x0>; /* tests/test_a.dts:15 */
> > > 	}; /* tests/test_a.dts:16 */
> > >
> > > 	foo: foo_node { /* tests/test_d.dts:6, tests/test_d.dts:2, tests/test_d.dts:6, tests/test_d.dts:2, tests/test_a.dts:18 */
> > > 		prop_1: added-prop = <0x1 0x2 0x3>; /* tests/test_d.dts:7 */
> > > 	}; /* tests/test_d.dts:8, tests/test_d.dts:4, tests/test_d.dts:8, tests/test_d.dts:4, tests/test_a.dts:20 */
> > >
> > > 	bar: bar_node { /* tests/test_d.dts:10, tests/test_d.dts:10, tests/test_a.dts:22 */
> > > 		added-prop = <0x5>; /* tests/test_d.dts:11 */
> > > 	}; /* tests/test_d.dts:12, tests/test_d.dts:12, tests/test_a.dts:24 */
> > > }; /* tests/test_d.dts:13, tests/test_d.dts:13, tests/test_a.dts:26 */
> > >
> > > --------
> > >
> > > For example, in the last line, we see tests/test_d.dts:13 twice, because it
> > > is merged in twice.  Is this what is wanted?  Should it be there only once?
> > > Should there be some indication on how tests/test_d.dts:13 is reached?
> > > This is a fake example, but I saw some examples with duplicated positions
> > > in the Linux kernel code.
> >
> > Right, that's ugly, but it's not that easy to fix.  Really we'd need
> > to merge/overlap each element in the list to accumulate them.  Going
> > back to the original proposal isn't a solution though because of cases
> > like this, which will be much more common that duplicated includes:
> >
> > / {
> > 	foo {
> > 		compatible = "...";
> > 		reg = < .. >;
> > 		ranges = < ... >;
> > 		lots = ...;
> > 		of = ...;
> > 		other = ...;
> > 		properties = ...;
> > 	};
> > }
> >
> > &{/foo} {
> > 	one-tiny-change = "yes";
> > };
> >
> > The original proposal would annotate the output with *only* the
> > location of the one-tiny-change, which is extremely misleading.  Much
> > worse than duplicated locations.
> 
> It's only the label on the braces that is affected.  compatible, reg, etc
> should still have their original locations.

Sure, it'll be ok if the errors are able to reference a specific
property.  But we can't really count on that - it seems likely that
there will be errors which refer to a node, but can't really point at
a specific property as the problem.

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

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

On Tue, 16 Jan 2018, David Gibson wrote:

> On Mon, Jan 15, 2018 at 10:28:38AM +0100, Julia Lawall wrote:
> >
> >
> > On Mon, 15 Jan 2018, David Gibson wrote:
> >
> > > On Thu, Jan 11, 2018 at 04:21:44PM +0100, Julia Lawall wrote:
> > > > > > +	old_node->srcpos = srcpos_copy_all(new_node_begin_srcpos);
> > > > >
> > > > > This doesn't seem right.  Replacing the old position with the new
> > > > > makes sense for indivudal properties where the whole value is also
> > > > > replaced.  But for nodes we really need to track both locations.
> > > > >
> > > > > I think the extra complexity here is why I didn't add this tracking
> > > > > earlier.
> > > >
> > > > I have the following example:
> > > >
> > > > /dts-v1/;
> > > >
> > > > / {
> > > >         #address-cells = < 1 >;
> > > >         #size-cells = < 1 >;
> > > >
> > > >         tree_1: soc@0 {
> > > >         	reg = <0x0 0x0>;
> > > >         };
> > > >
> > > >         foo: foo_node {
> > > >                 prop_1: added-prop = <0x99>;
> > > >         };
> > > >
> > > >         bar_node {
> > > >                 added-prop = <0x77>;
> > > >         };
> > > >
> > > > };
> > > >
> > > > /include/ "test_b1.dts"
> > > > /include/ "test_c1.dts"
> > > >
> > > > -------------------
> > > >
> > > > Then test_b1.dts and test_c1.dts are both /include/ "test_d.dts".  And
> > > > test_d.dts is:
> > > >
> > > > / {
> > > >         foo_node {
> > > >                 added-prop = <0x1 0x2 0x3>;
> > > >         };
> > > >
> > > >         foo_node {
> > > >                 added-prop = <0x1 0x2 0x3>;
> > > >         };
> > > >
> > > >         bar: bar_node {
> > > >                 added-prop = <0x5>;
> > > >         };
> > > > };
> > > >
> > > > ----------
> > > >
> > > > The point of the example is that via the includes there are tw ways to
> > > > reach test_d.dts.
> > > >
> > > > Accordingly, by accumulating a list of positions in merge_nodes, I end up with:
> > > >
> > > > /dts-v1/;
> > > >
> > > > / { /* tests/test_d.dts:1, tests/test_d.dts:1, tests/test_a.dts:10 */
> > > > 	#address-cells = <0x1>; /* tests/test_a.dts:11 */
> > > > 	#size-cells = <0x1>; /* tests/test_a.dts:12 */
> > > >
> > > > 	tree_1: soc@0 { /* tests/test_a.dts:14 */
> > > > 		reg = <0x0 0x0>; /* tests/test_a.dts:15 */
> > > > 	}; /* tests/test_a.dts:16 */
> > > >
> > > > 	foo: foo_node { /* tests/test_d.dts:6, tests/test_d.dts:2, tests/test_d.dts:6, tests/test_d.dts:2, tests/test_a.dts:18 */
> > > > 		prop_1: added-prop = <0x1 0x2 0x3>; /* tests/test_d.dts:7 */
> > > > 	}; /* tests/test_d.dts:8, tests/test_d.dts:4, tests/test_d.dts:8, tests/test_d.dts:4, tests/test_a.dts:20 */
> > > >
> > > > 	bar: bar_node { /* tests/test_d.dts:10, tests/test_d.dts:10, tests/test_a.dts:22 */
> > > > 		added-prop = <0x5>; /* tests/test_d.dts:11 */
> > > > 	}; /* tests/test_d.dts:12, tests/test_d.dts:12, tests/test_a.dts:24 */
> > > > }; /* tests/test_d.dts:13, tests/test_d.dts:13, tests/test_a.dts:26 */
> > > >
> > > > --------
> > > >
> > > > For example, in the last line, we see tests/test_d.dts:13 twice, because it
> > > > is merged in twice.  Is this what is wanted?  Should it be there only once?
> > > > Should there be some indication on how tests/test_d.dts:13 is reached?
> > > > This is a fake example, but I saw some examples with duplicated positions
> > > > in the Linux kernel code.
> > >
> > > Right, that's ugly, but it's not that easy to fix.  Really we'd need
> > > to merge/overlap each element in the list to accumulate them.  Going
> > > back to the original proposal isn't a solution though because of cases
> > > like this, which will be much more common that duplicated includes:
> > >
> > > / {
> > > 	foo {
> > > 		compatible = "...";
> > > 		reg = < .. >;
> > > 		ranges = < ... >;
> > > 		lots = ...;
> > > 		of = ...;
> > > 		other = ...;
> > > 		properties = ...;
> > > 	};
> > > }
> > >
> > > &{/foo} {
> > > 	one-tiny-change = "yes";
> > > };
> > >
> > > The original proposal would annotate the output with *only* the
> > > location of the one-tiny-change, which is extremely misleading.  Much
> > > worse than duplicated locations.
> >
> > It's only the label on the braces that is affected.  compatible, reg, etc
> > should still have their original locations.
>
> Sure, it'll be ok if the errors are able to reference a specific
> property.  But we can't really count on that - it seems likely that
> there will be errors which refer to a node, but can't really point at
> a specific property as the problem.

OK, there is the complete list with no duplicates in the revised patch
series.

julia

Re: [PATCH 2/5] annotations: Add position information to various calls

[Frank Rowand]

On 01/08/18 05:36, Julia Lawall wrote:
> Builds on a patch proposed by Frank Rowand:
> 
> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
> 
> Added NULL position argument in a few new places in dtc-parser.tab.c (1)
> and livetree.c (3).
> 
> For both '/' nodedef productions, include the '/' in the position.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> ---
>  dtc-parser.y | 23 +++++++++++++----------
>  dtc.c        | 10 ++++++++++
>  dtc.h        | 14 ++++++++++----
>  flattree.c   |  2 +-
>  fstree.c     |  8 +++++---
>  livetree.c   | 43 ++++++++++++++++++++++++++++++-------------
>  srcpos.c     | 35 +++++++++++++++++++++++++++++++++++
>  srcpos.h     |  3 +++
>  treesource.c | 38 +++++++++++++++++++++++++++++++++-----
>  9 files changed, 140 insertions(+), 36 deletions(-)
> 
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 44af170..d668349 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -160,11 +160,11 @@ memreserve:
>  devicetree:
>  	  '/' nodedef
>  		{
> -			$$ = name_node($2, "");
> +			$$ = name_node($2, "", &@$);
>  		}
>  	| devicetree '/' nodedef
>  		{
> -			$$ = merge_nodes($1, $3);
> +			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
>  		}
>  	| DT_REF nodedef
>  		{
> @@ -175,7 +175,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, &@2);
>  		}
>  	| devicetree DT_LABEL DT_REF nodedef
>  		{
> @@ -183,7 +186,7 @@ devicetree:
>  
>  			if (target) {
>  				add_label(&target->labels, $2);
> -				merge_nodes(target, $4);
> +				merge_nodes(target, $4, &@4);
>  			} else
>  				ERROR(&@3, "Label or path %s not found", $3);
>  			$$ = $1;
> @@ -193,7 +196,7 @@ devicetree:
>  			struct node *target = get_node_by_ref($1, $2);
>  
>  			if (target) {
> -				merge_nodes(target, $3);
> +				merge_nodes(target, $3, &@3);
>  			} else {
>  				/*
>  				 * We rely on the rule being always:
> @@ -201,7 +204,7 @@ devicetree:
>  				 * so $-1 is what we want (plugindecl)
>  				 */
>  				if ($<flags>-1 & DTSF_PLUGIN)
> -					add_orphan_node($1, $3, $2);
> +					add_orphan_node($1, $3, $2, &@3);
>  				else
>  					ERROR(&@2, "Label or path %s not found", $2);
>  			}
> @@ -242,11 +245,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 ';'
>  		{
> @@ -517,11 +520,11 @@ subnodes:
>  subnode:
>  	  DT_PROPNODENAME nodedef
>  		{
> -			$$ = name_node($2, $1);
> +			$$ = name_node($2, $1, &@$);
>  		}
>  	| DT_DEL_NODE DT_PROPNODENAME ';'
>  		{
> -			$$ = name_node(build_node_delete(), $2);
> +			$$ = name_node(build_node_delete(), $2, &@$);
>  		}
>  	| DT_LABEL subnode
>  		{
> diff --git a/dtc.c b/dtc.c
> index c36994e..371d04c 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -35,6 +35,7 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
>  int generate_symbols;	/* enable symbols & fixup support */
>  int generate_fixups;		/* suppress generation of fixups on symbol support */
>  int auto_label_aliases;		/* auto generate labels -> aliases */
> +bool annotate = false; /* annotate .dts with input source location */
>  
>  static int is_power_of_2(int x)
>  {
> @@ -83,6 +84,7 @@ static struct option const usage_long_opts[] = {
>  	{"auto-alias",       no_argument, NULL, 'A'},
>  	{"help",             no_argument, NULL, 'h'},
>  	{"version",          no_argument, NULL, 'v'},
> +	{"annotate",         no_argument, NULL, 'T'},

The "annotate" entry should be just before the "help" entry.

-Frank

Re: [PATCH 2/5] annotations: Add position information to various calls

[Frank Rowand]

On 01/09/18 22:25, Frank Rowand wrote:
> On 01/08/18 05:36, Julia Lawall wrote:
>> Builds on a patch proposed by Frank Rowand:
>>
>> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
>>
>> Added NULL position argument in a few new places in dtc-parser.tab.c (1)
>> and livetree.c (3).
>>
>> For both '/' nodedef productions, include the '/' in the position.
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
>> ---
>>  dtc-parser.y | 23 +++++++++++++----------
>>  dtc.c        | 10 ++++++++++
>>  dtc.h        | 14 ++++++++++----
>>  flattree.c   |  2 +-
>>  fstree.c     |  8 +++++---
>>  livetree.c   | 43 ++++++++++++++++++++++++++++++-------------
>>  srcpos.c     | 35 +++++++++++++++++++++++++++++++++++
>>  srcpos.h     |  3 +++
>>  treesource.c | 38 +++++++++++++++++++++++++++++++++-----
>>  9 files changed, 140 insertions(+), 36 deletions(-)
>>
>> diff --git a/dtc-parser.y b/dtc-parser.y
>> index 44af170..d668349 100644
>> --- a/dtc-parser.y
>> +++ b/dtc-parser.y
>> @@ -160,11 +160,11 @@ memreserve:
>>  devicetree:
>>  	  '/' nodedef
>>  		{
>> -			$$ = name_node($2, "");
>> +			$$ = name_node($2, "", &@$);
>>  		}
>>  	| devicetree '/' nodedef
>>  		{
>> -			$$ = merge_nodes($1, $3);
>> +			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
>>  		}
>>  	| DT_REF nodedef
>>  		{
>> @@ -175,7 +175,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, &@2);
>>  		}
>>  	| devicetree DT_LABEL DT_REF nodedef
>>  		{
>> @@ -183,7 +186,7 @@ devicetree:
>>  
>>  			if (target) {
>>  				add_label(&target->labels, $2);
>> -				merge_nodes(target, $4);
>> +				merge_nodes(target, $4, &@4);
>>  			} else
>>  				ERROR(&@3, "Label or path %s not found", $3);
>>  			$$ = $1;
>> @@ -193,7 +196,7 @@ devicetree:
>>  			struct node *target = get_node_by_ref($1, $2);
>>  
>>  			if (target) {
>> -				merge_nodes(target, $3);
>> +				merge_nodes(target, $3, &@3);
>>  			} else {
>>  				/*
>>  				 * We rely on the rule being always:
>> @@ -201,7 +204,7 @@ devicetree:
>>  				 * so $-1 is what we want (plugindecl)
>>  				 */
>>  				if ($<flags>-1 & DTSF_PLUGIN)
>> -					add_orphan_node($1, $3, $2);
>> +					add_orphan_node($1, $3, $2, &@3);
>>  				else
>>  					ERROR(&@2, "Label or path %s not found", $2);
>>  			}
>> @@ -242,11 +245,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 ';'
>>  		{
>> @@ -517,11 +520,11 @@ subnodes:
>>  subnode:
>>  	  DT_PROPNODENAME nodedef
>>  		{
>> -			$$ = name_node($2, $1);
>> +			$$ = name_node($2, $1, &@$);
>>  		}
>>  	| DT_DEL_NODE DT_PROPNODENAME ';'
>>  		{
>> -			$$ = name_node(build_node_delete(), $2);
>> +			$$ = name_node(build_node_delete(), $2, &@$);
>>  		}
>>  	| DT_LABEL subnode
>>  		{
>> diff --git a/dtc.c b/dtc.c
>> index c36994e..371d04c 100644
>> --- a/dtc.c
>> +++ b/dtc.c
>> @@ -35,6 +35,7 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
>>  int generate_symbols;	/* enable symbols & fixup support */
>>  int generate_fixups;		/* suppress generation of fixups on symbol support */
>>  int auto_label_aliases;		/* auto generate labels -> aliases */
>> +bool annotate = false; /* annotate .dts with input source location */
>>  
>>  static int is_power_of_2(int x)
>>  {
>> @@ -83,6 +84,7 @@ static struct option const usage_long_opts[] = {
>>  	{"auto-alias",       no_argument, NULL, 'A'},
>>  	{"help",             no_argument, NULL, 'h'},
>>  	{"version",          no_argument, NULL, 'v'},
>> +	{"annotate",         no_argument, NULL, 'T'},
> 
> The "annotate" entry should be just before the "help" entry.
> 
> -Frank
> 

Ah, I see that patch 5 makes this correction.  See my comment that I'm about to
write on patch 5.

-Frank

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

On Tue, 9 Jan 2018, Frank Rowand wrote:

> On 01/09/18 22:25, Frank Rowand wrote:
> > On 01/08/18 05:36, Julia Lawall wrote:
> >> Builds on a patch proposed by Frank Rowand:
> >>
> >> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
> >>
> >> Added NULL position argument in a few new places in dtc-parser.tab.c (1)
> >> and livetree.c (3).
> >>
> >> For both '/' nodedef productions, include the '/' in the position.
> >>
> >> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> >> ---
> >>  dtc-parser.y | 23 +++++++++++++----------
> >>  dtc.c        | 10 ++++++++++
> >>  dtc.h        | 14 ++++++++++----
> >>  flattree.c   |  2 +-
> >>  fstree.c     |  8 +++++---
> >>  livetree.c   | 43 ++++++++++++++++++++++++++++++-------------
> >>  srcpos.c     | 35 +++++++++++++++++++++++++++++++++++
> >>  srcpos.h     |  3 +++
> >>  treesource.c | 38 +++++++++++++++++++++++++++++++++-----
> >>  9 files changed, 140 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/dtc-parser.y b/dtc-parser.y
> >> index 44af170..d668349 100644
> >> --- a/dtc-parser.y
> >> +++ b/dtc-parser.y
> >> @@ -160,11 +160,11 @@ memreserve:
> >>  devicetree:
> >>  	  '/' nodedef
> >>  		{
> >> -			$$ = name_node($2, "");
> >> +			$$ = name_node($2, "", &@$);
> >>  		}
> >>  	| devicetree '/' nodedef
> >>  		{
> >> -			$$ = merge_nodes($1, $3);
> >> +			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
> >>  		}
> >>  	| DT_REF nodedef
> >>  		{
> >> @@ -175,7 +175,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, &@2);
> >>  		}
> >>  	| devicetree DT_LABEL DT_REF nodedef
> >>  		{
> >> @@ -183,7 +186,7 @@ devicetree:
> >>
> >>  			if (target) {
> >>  				add_label(&target->labels, $2);
> >> -				merge_nodes(target, $4);
> >> +				merge_nodes(target, $4, &@4);
> >>  			} else
> >>  				ERROR(&@3, "Label or path %s not found", $3);
> >>  			$$ = $1;
> >> @@ -193,7 +196,7 @@ devicetree:
> >>  			struct node *target = get_node_by_ref($1, $2);
> >>
> >>  			if (target) {
> >> -				merge_nodes(target, $3);
> >> +				merge_nodes(target, $3, &@3);
> >>  			} else {
> >>  				/*
> >>  				 * We rely on the rule being always:
> >> @@ -201,7 +204,7 @@ devicetree:
> >>  				 * so $-1 is what we want (plugindecl)
> >>  				 */
> >>  				if ($<flags>-1 & DTSF_PLUGIN)
> >> -					add_orphan_node($1, $3, $2);
> >> +					add_orphan_node($1, $3, $2, &@3);
> >>  				else
> >>  					ERROR(&@2, "Label or path %s not found", $2);
> >>  			}
> >> @@ -242,11 +245,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 ';'
> >>  		{
> >> @@ -517,11 +520,11 @@ subnodes:
> >>  subnode:
> >>  	  DT_PROPNODENAME nodedef
> >>  		{
> >> -			$$ = name_node($2, $1);
> >> +			$$ = name_node($2, $1, &@$);
> >>  		}
> >>  	| DT_DEL_NODE DT_PROPNODENAME ';'
> >>  		{
> >> -			$$ = name_node(build_node_delete(), $2);
> >> +			$$ = name_node(build_node_delete(), $2, &@$);
> >>  		}
> >>  	| DT_LABEL subnode
> >>  		{
> >> diff --git a/dtc.c b/dtc.c
> >> index c36994e..371d04c 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -35,6 +35,7 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
> >>  int generate_symbols;	/* enable symbols & fixup support */
> >>  int generate_fixups;		/* suppress generation of fixups on symbol support */
> >>  int auto_label_aliases;		/* auto generate labels -> aliases */
> >> +bool annotate = false; /* annotate .dts with input source location */
> >>
> >>  static int is_power_of_2(int x)
> >>  {
> >> @@ -83,6 +84,7 @@ static struct option const usage_long_opts[] = {
> >>  	{"auto-alias",       no_argument, NULL, 'A'},
> >>  	{"help",             no_argument, NULL, 'h'},
> >>  	{"version",          no_argument, NULL, 'v'},
> >> +	{"annotate",         no_argument, NULL, 'T'},
> >
> > The "annotate" entry should be just before the "help" entry.
> >
> > -Frank
> >
>
> Ah, I see that patch 5 makes this correction.  See my comment that I'm about to
> write on patch 5.

I've put it in the right place in the right patch now.

julia

Re: [PATCH 2/5] annotations: Add position information to various calls

[Julia Lawall]

On Tue, 9 Jan 2018, Frank Rowand wrote:

> On 01/08/18 05:36, Julia Lawall wrote:
> > Builds on a patch proposed by Frank Rowand:
> >
> > https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00372.html
> >
> > Added NULL position argument in a few new places in dtc-parser.tab.c (1)
> > and livetree.c (3).
> >
> > For both '/' nodedef productions, include the '/' in the position.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> > ---
> >  dtc-parser.y | 23 +++++++++++++----------
> >  dtc.c        | 10 ++++++++++
> >  dtc.h        | 14 ++++++++++----
> >  flattree.c   |  2 +-
> >  fstree.c     |  8 +++++---
> >  livetree.c   | 43 ++++++++++++++++++++++++++++++-------------
> >  srcpos.c     | 35 +++++++++++++++++++++++++++++++++++
> >  srcpos.h     |  3 +++
> >  treesource.c | 38 +++++++++++++++++++++++++++++++++-----
> >  9 files changed, 140 insertions(+), 36 deletions(-)
> >
> > diff --git a/dtc-parser.y b/dtc-parser.y
> > index 44af170..d668349 100644
> > --- a/dtc-parser.y
> > +++ b/dtc-parser.y
> > @@ -160,11 +160,11 @@ memreserve:
> >  devicetree:
> >  	  '/' nodedef
> >  		{
> > -			$$ = name_node($2, "");
> > +			$$ = name_node($2, "", &@$);
> >  		}
> >  	| devicetree '/' nodedef
> >  		{
> > -			$$ = merge_nodes($1, $3);
> > +			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
> >  		}
> >  	| DT_REF nodedef
> >  		{
> > @@ -175,7 +175,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, &@2);
> >  		}
> >  	| devicetree DT_LABEL DT_REF nodedef
> >  		{
> > @@ -183,7 +186,7 @@ devicetree:
> >
> >  			if (target) {
> >  				add_label(&target->labels, $2);
> > -				merge_nodes(target, $4);
> > +				merge_nodes(target, $4, &@4);
> >  			} else
> >  				ERROR(&@3, "Label or path %s not found", $3);
> >  			$$ = $1;
> > @@ -193,7 +196,7 @@ devicetree:
> >  			struct node *target = get_node_by_ref($1, $2);
> >
> >  			if (target) {
> > -				merge_nodes(target, $3);
> > +				merge_nodes(target, $3, &@3);
> >  			} else {
> >  				/*
> >  				 * We rely on the rule being always:
> > @@ -201,7 +204,7 @@ devicetree:
> >  				 * so $-1 is what we want (plugindecl)
> >  				 */
> >  				if ($<flags>-1 & DTSF_PLUGIN)
> > -					add_orphan_node($1, $3, $2);
> > +					add_orphan_node($1, $3, $2, &@3);
> >  				else
> >  					ERROR(&@2, "Label or path %s not found", $2);
> >  			}
> > @@ -242,11 +245,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 ';'
> >  		{
> > @@ -517,11 +520,11 @@ subnodes:
> >  subnode:
> >  	  DT_PROPNODENAME nodedef
> >  		{
> > -			$$ = name_node($2, $1);
> > +			$$ = name_node($2, $1, &@$);
> >  		}
> >  	| DT_DEL_NODE DT_PROPNODENAME ';'
> >  		{
> > -			$$ = name_node(build_node_delete(), $2);
> > +			$$ = name_node(build_node_delete(), $2, &@$);
> >  		}
> >  	| DT_LABEL subnode
> >  		{
> > diff --git a/dtc.c b/dtc.c
> > index c36994e..371d04c 100644
> > --- a/dtc.c
> > +++ b/dtc.c
> > @@ -35,6 +35,7 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
> >  int generate_symbols;	/* enable symbols & fixup support */
> >  int generate_fixups;		/* suppress generation of fixups on symbol support */
> >  int auto_label_aliases;		/* auto generate labels -> aliases */
> > +bool annotate = false; /* annotate .dts with input source location */
> >
> >  static int is_power_of_2(int x)
> >  {
> > @@ -83,6 +84,7 @@ static struct option const usage_long_opts[] = {
> >  	{"auto-alias",       no_argument, NULL, 'A'},
> >  	{"help",             no_argument, NULL, 'h'},
> >  	{"version",          no_argument, NULL, 'v'},
> > +	{"annotate",         no_argument, NULL, 'T'},
>
> The "annotate" entry should be just before the "help" entry.

Yes, thanks.

julia

[PATCH 3/5] annotations: short annotations

[Julia Lawall]

Based on the following patch by Frank Rowand (no changes):

https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00378.html

Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
---
 srcpos.c     | 34 ++++++++++++++++++++++++++++++++++
 srcpos.h     |  2 ++
 treesource.c |  8 ++++----
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/srcpos.c b/srcpos.c
index 1a4db9c..85657c6 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -317,6 +317,40 @@ out:
 	return pos_str;
 }
 
+static char *
+srcpos_string_short(struct srcpos *pos, bool first_line)
+{
+	const char *fname = "<no-file>";
+	char *pos_str;
+	int rc;
+
+	if (pos) {
+		fname = pos->file->name;
+		rc = asprintf(&pos_str, "%s:%d", fname,
+			      (first_line) ? pos->first_line: pos->last_line);
+	}
+	else {
+		rc = asprintf(&pos_str, "%s:<no-line>", fname);
+	}
+
+	if (rc == -1)
+		die("Couldn't allocate in srcpos_string_short");
+
+	return pos_str;
+}
+
+char *
+srcpos_string_first(struct srcpos *pos)
+{
+	return srcpos_string_short(pos, true);
+}
+
+char *
+srcpos_string_last(struct srcpos *pos)
+{
+	return srcpos_string_short(pos, false);
+}
+
 void srcpos_verror(struct srcpos *pos, const char *prefix,
 		   const char *fmt, va_list va)
 {
diff --git a/srcpos.h b/srcpos.h
index ec69d89..9281cba 100644
--- a/srcpos.h
+++ b/srcpos.h
@@ -109,6 +109,8 @@ extern struct srcpos *srcpos_copy_all(struct srcpos *pos);
 extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
 	 struct srcpos *right_srcpos);
 extern char *srcpos_string(struct srcpos *pos);
+extern char *srcpos_string_first(struct srcpos *pos);
+extern char *srcpos_string_last(struct srcpos *pos);
 
 extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
 					const char *fmt, va_list va);
diff --git a/treesource.c b/treesource.c
index a99eca4..9a22b5e 100644
--- a/treesource.c
+++ b/treesource.c
@@ -204,7 +204,7 @@ static void write_propval(FILE *f, struct property *prop)
 
 	if (len == 0) {
 		if (annotate) {
-			srcstr = srcpos_string(prop->srcpos);
+			srcstr = srcpos_string_first(prop->srcpos);
 			fprintf(f, "; /* %s */\n", srcstr);
 			free(srcstr);
 		} else {
@@ -238,7 +238,7 @@ static void write_propval(FILE *f, struct property *prop)
 	}
 
 	if (annotate) {
-		srcstr = srcpos_string(prop->srcpos);
+		srcstr = srcpos_string_first(prop->srcpos);
 		fprintf(f, "; /* %s */\n", srcstr);
 		free(srcstr);
 	} else {
@@ -262,7 +262,7 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 		fprintf(f, "/ {");
 
 	if (annotate) {
-		srcstr = srcpos_string(tree->srcpos);
+		srcstr = srcpos_string_first(tree->srcpos);
 		fprintf(f, " /* %s */\n", srcstr);
 		free(srcstr);
 	} else {
@@ -282,7 +282,7 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 	}
 	write_prefix(f, level);
 	if (annotate) {
-		srcstr = srcpos_string(tree->srcpos);
+		srcstr = srcpos_string_last(tree->srcpos);
 		fprintf(f, "}; /* %s */\n", srcstr);
 		free(srcstr);
 	} else {
-- 
2.7.4

Re: [PATCH 3/5] annotations: short annotations

[David Gibson]

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

On Mon, Jan 08, 2018 at 02:36:45PM +0100, Julia Lawall wrote:
> Based on the following patch by Frank Rowand (no changes):
> 
>
>https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00378.html

If there are no changes, then it should definitely have Frank's S-o-b.

> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> ---
>  srcpos.c     | 34 ++++++++++++++++++++++++++++++++++
>  srcpos.h     |  2 ++
>  treesource.c |  8 ++++----
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/srcpos.c b/srcpos.c
> index 1a4db9c..85657c6 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -317,6 +317,40 @@ out:
>  	return pos_str;
>  }
>  
> +static char *
> +srcpos_string_short(struct srcpos *pos, bool first_line)
> +{
> +	const char *fname = "<no-file>";
> +	char *pos_str;
> +	int rc;
> +
> +	if (pos) {
> +		fname = pos->file->name;
> +		rc = asprintf(&pos_str, "%s:%d", fname,

You can use xasprintf to avoid having to check for failure.

> +			      (first_line) ? pos->first_line: pos->last_line);
> +	}

else on the same line as }, please - dtc uses kernel coding style
(more or less).

> +	else {
> +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
> +	}
> +
> +	if (rc == -1)
> +		die("Couldn't allocate in srcpos_string_short");
> +
> +	return pos_str;
> +}
> +
> +char *
> +srcpos_string_first(struct srcpos *pos)
> +{
> +	return srcpos_string_short(pos, true);
> +}
> +
> +char *
> +srcpos_string_last(struct srcpos *pos)
> +{
> +	return srcpos_string_short(pos, false);
> +}
> +
>  void srcpos_verror(struct srcpos *pos, const char *prefix,
>  		   const char *fmt, va_list va)
>  {
> diff --git a/srcpos.h b/srcpos.h
> index ec69d89..9281cba 100644
> --- a/srcpos.h
> +++ b/srcpos.h
> @@ -109,6 +109,8 @@ extern struct srcpos *srcpos_copy_all(struct srcpos *pos);
>  extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
>  	 struct srcpos *right_srcpos);
>  extern char *srcpos_string(struct srcpos *pos);
> +extern char *srcpos_string_first(struct srcpos *pos);
> +extern char *srcpos_string_last(struct srcpos *pos);
>  
>  extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
>  					const char *fmt, va_list va);
> diff --git a/treesource.c b/treesource.c
> index a99eca4..9a22b5e 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -204,7 +204,7 @@ static void write_propval(FILE *f, struct property *prop)
>  
>  	if (len == 0) {
>  		if (annotate) {
> -			srcstr = srcpos_string(prop->srcpos);
> +			srcstr = srcpos_string_first(prop->srcpos);
>  			fprintf(f, "; /* %s */\n", srcstr);
>  			free(srcstr);
>  		} else {
> @@ -238,7 +238,7 @@ static void write_propval(FILE *f, struct property *prop)
>  	}
>  
>  	if (annotate) {
> -		srcstr = srcpos_string(prop->srcpos);
> +		srcstr = srcpos_string_first(prop->srcpos);
>  		fprintf(f, "; /* %s */\n", srcstr);
>  		free(srcstr);
>  	} else {
> @@ -262,7 +262,7 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  		fprintf(f, "/ {");
>  
>  	if (annotate) {
> -		srcstr = srcpos_string(tree->srcpos);
> +		srcstr = srcpos_string_first(tree->srcpos);
>  		fprintf(f, " /* %s */\n", srcstr);
>  		free(srcstr);
>  	} else {
> @@ -282,7 +282,7 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  	}
>  	write_prefix(f, level);
>  	if (annotate) {
> -		srcstr = srcpos_string(tree->srcpos);
> +		srcstr = srcpos_string_last(tree->srcpos);
>  		fprintf(f, "}; /* %s */\n", srcstr);
>  		free(srcstr);
>  	} else {

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

Re: [PATCH 3/5] annotations: short annotations

[Frank Rowand]

Hi David,

On 01/09/18 03:21, David Gibson wrote:
> On Mon, Jan 08, 2018 at 02:36:45PM +0100, Julia Lawall wrote:
>> Based on the following patch by Frank Rowand (no changes):
>>
>>
>> https://www.mail-archive.com/devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg00378.html
> 
> If there are no changes, then it should definitely have Frank's S-o-b.
> 
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
>> ---

< snip >

In the context of the S-o-b questions, I was referencing the Linux kernel
documentation for the meaning of S-o-b.  I realized that I should check
that the same (or similar) definitions existed in the dtc project, but
have not found that definition.  Should that definition be added to
file README.license?

I think the appropriate portion of the Linux kernel file
Documentation/process/submitting-patches.rst (which to the best of my
understanding is GPL v2) is a portion of section "11) Sign your
work - the Developer's Certificate of Origin":


The sign-off is a simple line at the end of the explanation for the
patch, which certifies that you wrote it or otherwise have the right to
pass it on as an open-source patch.  The rules are pretty simple: if you
can certify the below:

Developer's Certificate of Origin 1.1
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

then you just add a line saying::

        Signed-off-by: Random J Developer <random-ld4jwAGwUXQYGaZWVHDzw80vGNN6ct63@public.gmane.org>

using your real name (sorry, no pseudonyms or anonymous contributions.)

[PATCH 4/5] annotations: shorten file names

[Julia Lawall]

The file name provided on the command line is represented by its
basename only.  Other names are represented as relative offsets from the
directory of the starting one.

This has to be adapted in the case of files on which cpp has been run
first.  In that case, at least when using scripts/dtc/dtx_diff from the
Linux kernel, the command line file name is -, representing standard
input.  The starting file name is then taken from the first line
beginning with #, as detected by a call to srcpos_set_line.

Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
---
 srcpos.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/srcpos.c b/srcpos.c
index 85657c6..fe756a7 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -33,6 +33,9 @@ struct search_path {
 /* This is the list of directories that we search for source files */
 static struct search_path *search_path_head, **search_path_tail;
 
+/* Detect infinite include recursion. */
+#define MAX_SRCFILE_DEPTH     (100)
+static int srcfile_depth; /* = 0 */
 
 static char *get_dirname(const char *path)
 {
@@ -51,11 +54,50 @@ static char *get_dirname(const char *path)
 
 FILE *depfile; /* = NULL */
 struct srcfile_state *current_srcfile; /* = NULL */
+static char *initial_path; /* = NULL */
+static int initial_pathlen; /* = NULL */
+static bool initial_cpp = true;
+
+static void set_initial_path(char *fname) {
+	int i, len = strlen(fname);
+
+	initial_path = fname;
+	initial_pathlen = 0;
+	for (i = 0; i != len; i++)
+		if (initial_path[i] == '/')
+			initial_pathlen++;
+}
 
-/* Detect infinite include recursion. */
-#define MAX_SRCFILE_DEPTH     (100)
-static int srcfile_depth; /* = 0 */
-
+static char *
+shorten_to_initial_path(char *fname) {
+	char *p1, *p2, *prevslash1 = NULL;
+	int slashes = 0;
+
+	for (p1 = fname, p2 = initial_path; *p1 && *p2; p1++, p2++) {
+		if (*p1 != *p2)
+			break;
+		if (*p1 == '/') {
+			prevslash1 = p1;
+			slashes++;
+		}
+	}
+	p1 = prevslash1 + 1;
+	if (prevslash1) {
+		int diff = initial_pathlen - slashes, i, j;
+		int restlen = strlen(fname) - (p1 - fname);
+		char *res;
+
+		res = xmalloc((3 * diff) + restlen + 1);
+		for (i = 0, j = 0; i != diff; i++) {
+			res[j++] = '.';
+			res[j++] = '.';
+			res[j++] = '/';
+		}
+		strcpy(res + j, p1);
+		return res;
+	}
+	return fname;
+}
 
 /**
  * Try to open a file in a given directory.
@@ -157,6 +199,9 @@ void srcfile_push(const char *fname)
 	srcfile->colno = 1;
 
 	current_srcfile = srcfile;
+
+	if (srcfile_depth == 1)
+		set_initial_path(srcfile->name);
 }
 
 bool srcfile_pop(void)
@@ -325,7 +370,7 @@ srcpos_string_short(struct srcpos *pos, bool first_line)
 	int rc;
 
 	if (pos) {
-		fname = pos->file->name;
+		fname = shorten_to_initial_path(pos->file->name);
 		rc = asprintf(&pos_str, "%s:%d", fname,
 			      (first_line) ? pos->first_line: pos->last_line);
 	}
@@ -379,4 +424,9 @@ void srcpos_set_line(char *f, int l)
 {
 	current_srcfile->name = f;
 	current_srcfile->lineno = l;
+
+	if (initial_cpp) {
+		initial_cpp = false;
+		set_initial_path(f);
+	}
 }
-- 
2.7.4

Re: [PATCH 4/5] annotations: shorten file names

[David Gibson]

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

On Mon, Jan 08, 2018 at 02:36:46PM +0100, Julia Lawall wrote:
1;5002;0c> The file name provided on the command line is represented by its
> basename only.  Other names are represented as relative offsets from the
> directory of the starting one.
> 
> This has to be adapted in the case of files on which cpp has been run
> first.  In that case, at least when using scripts/dtc/dtx_diff from the
> Linux kernel, the command line file name is -, representing standard
> input.  The starting file name is then taken from the first line
> beginning with #, as detected by a call to srcpos_set_line.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> ---
>  srcpos.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/srcpos.c b/srcpos.c
> index 85657c6..fe756a7 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -33,6 +33,9 @@ struct search_path {
>  /* This is the list of directories that we search for source files */
>  static struct search_path *search_path_head, **search_path_tail;
>  
> +/* Detect infinite include recursion. */
> +#define MAX_SRCFILE_DEPTH     (100)
> +static int srcfile_depth; /* = 0 */
>  
>  static char *get_dirname(const char *path)
>  {
> @@ -51,11 +54,50 @@ static char *get_dirname(const char *path)
>  
>  FILE *depfile; /* = NULL */
>  struct srcfile_state *current_srcfile; /* = NULL */
> +static char *initial_path; /* = NULL */
> +static int initial_pathlen; /* = NULL */
> +static bool initial_cpp = true;
> +
> +static void set_initial_path(char *fname) {
> +	int i, len = strlen(fname);
> +
> +	initial_path = fname;

This is setting an indefinite lifetime global to a pointer, which
AFAICT is dynamically allocated elswhere.  That doesn't seem very
safe.

> +	initial_pathlen = 0;
> +	for (i = 0; i != len; i++)
> +		if (initial_path[i] == '/')
> +			initial_pathlen++;
> +}
>  
> -/* Detect infinite include recursion. */
> -#define MAX_SRCFILE_DEPTH     (100)
> -static int srcfile_depth; /* = 0 */
> -
> +static char *
> +shorten_to_initial_path(char *fname) {

Return type on same line, { on separate line, please (for function
definitions only; Linux coding style).

> +	char *p1, *p2, *prevslash1 = NULL;
> +	int slashes = 0;
> +
> +	for (p1 = fname, p2 = initial_path; *p1 && *p2; p1++, p2++) {
> +		if (*p1 != *p2)
> +			break;
> +		if (*p1 == '/') {
> +			prevslash1 = p1;
> +			slashes++;
> +		}
> +	}
> +	p1 = prevslash1 + 1;
> +	if (prevslash1) {
> +		int diff = initial_pathlen - slashes, i, j;
> +		int restlen = strlen(fname) - (p1 - fname);
> +		char *res;
> +
> +		res = xmalloc((3 * diff) + restlen + 1);
> +		for (i = 0, j = 0; i != diff; i++) {
> +			res[j++] = '.';
> +			res[j++] = '.';
> +			res[j++] = '/';
> +		}
> +		strcpy(res + j, p1);
> +		return res;
> +	}
> +	return fname;
> +}
>  
>  /**
>   * Try to open a file in a given directory.
> @@ -157,6 +199,9 @@ void srcfile_push(const char *fname)
>  	srcfile->colno = 1;
>  
>  	current_srcfile = srcfile;
> +
> +	if (srcfile_depth == 1)
> +		set_initial_path(srcfile->name);
>  }
>  
>  bool srcfile_pop(void)
> @@ -325,7 +370,7 @@ srcpos_string_short(struct srcpos *pos, bool first_line)
>  	int rc;
>  
>  	if (pos) {
> -		fname = pos->file->name;
> +		fname = shorten_to_initial_path(pos->file->name);
>  		rc = asprintf(&pos_str, "%s:%d", fname,
>  			      (first_line) ? pos->first_line: pos->last_line);
>  	}
> @@ -379,4 +424,9 @@ void srcpos_set_line(char *f, int l)
>  {
>  	current_srcfile->name = f;
>  	current_srcfile->lineno = l;
> +
> +	if (initial_cpp) {
> +		initial_cpp = false;
> +		set_initial_path(f);
> +	}
>  }

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

Re: [PATCH 4/5] annotations: shorten file names

[Julia Lawall]

On Wed, 10 Jan 2018, David Gibson wrote:

> On Mon, Jan 08, 2018 at 02:36:46PM +0100, Julia Lawall wrote:
> 1;5002;0c> The file name provided on the command line is represented by its
> > basename only.  Other names are represented as relative offsets from the
> > directory of the starting one.
> >
> > This has to be adapted in the case of files on which cpp has been run
> > first.  In that case, at least when using scripts/dtc/dtx_diff from the
> > Linux kernel, the command line file name is -, representing standard
> > input.  The starting file name is then taken from the first line
> > beginning with #, as detected by a call to srcpos_set_line.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> > ---
> >  srcpos.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 55 insertions(+), 5 deletions(-)
> >
> > diff --git a/srcpos.c b/srcpos.c
> > index 85657c6..fe756a7 100644
> > --- a/srcpos.c
> > +++ b/srcpos.c
> > @@ -33,6 +33,9 @@ struct search_path {
> >  /* This is the list of directories that we search for source files */
> >  static struct search_path *search_path_head, **search_path_tail;
> >
> > +/* Detect infinite include recursion. */
> > +#define MAX_SRCFILE_DEPTH     (100)
> > +static int srcfile_depth; /* = 0 */
> >
> >  static char *get_dirname(const char *path)
> >  {
> > @@ -51,11 +54,50 @@ static char *get_dirname(const char *path)
> >
> >  FILE *depfile; /* = NULL */
> >  struct srcfile_state *current_srcfile; /* = NULL */
> > +static char *initial_path; /* = NULL */
> > +static int initial_pathlen; /* = NULL */
> > +static bool initial_cpp = true;
> > +
> > +static void set_initial_path(char *fname) {
> > +	int i, len = strlen(fname);
> > +
> > +	initial_path = fname;
>
> This is setting an indefinite lifetime global to a pointer, which
> AFAICT is dynamically allocated elswhere.  That doesn't seem very
> safe.

Yes.  I will fix this, and the format issue noted below.

thanks,
julia

>
> > +	initial_pathlen = 0;
> > +	for (i = 0; i != len; i++)
> > +		if (initial_path[i] == '/')
> > +			initial_pathlen++;
> > +}
> >
> > -/* Detect infinite include recursion. */
> > -#define MAX_SRCFILE_DEPTH     (100)
> > -static int srcfile_depth; /* = 0 */
> > -
> > +static char *
> > +shorten_to_initial_path(char *fname) {
>
> Return type on same line, { on separate line, please (for function
> definitions only; Linux coding style).
>
> > +	char *p1, *p2, *prevslash1 = NULL;
> > +	int slashes = 0;
> > +
> > +	for (p1 = fname, p2 = initial_path; *p1 && *p2; p1++, p2++) {
> > +		if (*p1 != *p2)
> > +			break;
> > +		if (*p1 == '/') {
> > +			prevslash1 = p1;
> > +			slashes++;
> > +		}
> > +	}
> > +	p1 = prevslash1 + 1;
> > +	if (prevslash1) {
> > +		int diff = initial_pathlen - slashes, i, j;
> > +		int restlen = strlen(fname) - (p1 - fname);
> > +		char *res;
> > +
> > +		res = xmalloc((3 * diff) + restlen + 1);
> > +		for (i = 0, j = 0; i != diff; i++) {
> > +			res[j++] = '.';
> > +			res[j++] = '.';
> > +			res[j++] = '/';
> > +		}
> > +		strcpy(res + j, p1);
> > +		return res;
> > +	}
> > +	return fname;
> > +}
> >
> >  /**
> >   * Try to open a file in a given directory.
> > @@ -157,6 +199,9 @@ void srcfile_push(const char *fname)
> >  	srcfile->colno = 1;
> >
> >  	current_srcfile = srcfile;
> > +
> > +	if (srcfile_depth == 1)
> > +		set_initial_path(srcfile->name);
> >  }
> >
> >  bool srcfile_pop(void)
> > @@ -325,7 +370,7 @@ srcpos_string_short(struct srcpos *pos, bool first_line)
> >  	int rc;
> >
> >  	if (pos) {
> > -		fname = pos->file->name;
> > +		fname = shorten_to_initial_path(pos->file->name);
> >  		rc = asprintf(&pos_str, "%s:%d", fname,
> >  			      (first_line) ? pos->first_line: pos->last_line);
> >  	}
> > @@ -379,4 +424,9 @@ void srcpos_set_line(char *f, int l)
> >  {
> >  	current_srcfile->name = f;
> >  	current_srcfile->lineno = l;
> > +
> > +	if (initial_cpp) {
> > +		initial_cpp = false;
> > +		set_initial_path(f);
> > +	}
> >  }
>
> --
> 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
>

[PATCH 5/5] annotations: add --annotate-full option

[Julia Lawall]

With --annotate, one gets only short file names and line numbers.

With --annotate-full, one gets complete paths, starting and ending line
numbers and starting and ending columns.

--annotate-full indicates no-file and no-line for things that are not
connected to the source code (fixups, symbols, aliases, etc.).
--annotate simply has nothing in those cases.

Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
---
 dtc.c        | 13 +++++++++----
 dtc.h        |  1 +
 srcpos.c     | 27 +++++++++++++++++++--------
 srcpos.h     |  4 ++--
 treesource.c | 48 ++++++++++++++++++++++++++++--------------------
 5 files changed, 59 insertions(+), 34 deletions(-)

diff --git a/dtc.c b/dtc.c
index 371d04c..24b58eb 100644
--- a/dtc.c
+++ b/dtc.c
@@ -35,7 +35,8 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
 int generate_symbols;	/* enable symbols & fixup support */
 int generate_fixups;		/* suppress generation of fixups on symbol support */
 int auto_label_aliases;		/* auto generate labels -> aliases */
-bool annotate = false; /* annotate .dts with input source location */
+bool annotate = false;		/* annotate .dts with input source location */
+bool annotate_full = false; /* annotate .dts with full input source location */
 
 static int is_power_of_2(int x)
 {
@@ -61,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
 
 /* Usage related data. */
 static const char usage_synopsis[] = "dtc [options] <input file>";
-static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@Ahv";
+static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@ATFhv";
 static struct option const usage_long_opts[] = {
 	{"quiet",            no_argument, NULL, 'q'},
 	{"in-format",         a_argument, NULL, 'I'},
@@ -82,9 +83,10 @@ static struct option const usage_long_opts[] = {
 	{"error",             a_argument, NULL, 'E'},
 	{"symbols",	     no_argument, NULL, '@'},
 	{"auto-alias",       no_argument, NULL, 'A'},
+	{"annotate",         no_argument, NULL, 'T'},
+	{"annotate-full",    no_argument, NULL, 'F'},
 	{"help",             no_argument, NULL, 'h'},
 	{"version",          no_argument, NULL, 'v'},
-	{"annotate",         no_argument, NULL, 'T'},
 	{NULL,               no_argument, NULL, 0x0},
 };
 static const char * const usage_opts_help[] = {
@@ -119,6 +121,7 @@ static const char * const usage_opts_help[] = {
 	"\n\tPrint this help and exit",
 	"\n\tPrint version and exit",
 	"\n\tAnnotate output .dts with input source file and line",
+	"\n\tAnnotate output .dts with input source file (full path) line and column",
 	NULL,
 };
 
@@ -188,6 +191,8 @@ int main(int argc, char *argv[])
 
 	while ((opt = util_getopt_long()) != EOF) {
 		switch (opt) {
+		case 'F':
+			annotate_full = true;
 		case 'T':
 			annotate = true;
 			break;
@@ -305,7 +310,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (annotate && (!streq(inform, "dts") || !streq(outform, "dts")))
-		die("--annotate requires -I dts -O dts\n");
+		die("--annotate and --annotate-full require -I dts -O dts\n");
 
 	if (streq(inform, "dts"))
 		dti = dt_from_source(arg);
diff --git a/dtc.h b/dtc.h
index e7121ef..769f6a3 100644
--- a/dtc.h
+++ b/dtc.h
@@ -59,6 +59,7 @@ extern int generate_symbols;	/* generate symbols for nodes with labels */
 extern int generate_fixups;	/* generate fixups */
 extern int auto_label_aliases;	/* auto generate labels -> aliases */
 extern bool annotate;		/* annotate .dts with input source location */
+extern bool annotate_full;/* annotate .dts with detailed input source location */
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
diff --git a/srcpos.c b/srcpos.c
index fe756a7..8cf0c3d 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -363,19 +363,30 @@ out:
 }
 
 static char *
-srcpos_string_short(struct srcpos *pos, bool first_line)
+srcpos_string_short(struct srcpos *pos, bool first_line, bool full)
 {
 	const char *fname = "<no-file>";
 	char *pos_str;
 	int rc;
 
 	if (pos) {
-		fname = shorten_to_initial_path(pos->file->name);
-		rc = asprintf(&pos_str, "%s:%d", fname,
+		if (full) {
+			rc = asprintf(&pos_str, "%s:%d:%d-%d:%d",
+				      pos->file->name,
+				      pos->first_line, pos->first_column,
+				      pos->last_line, pos->last_column);
+		}
+		else {
+			fname = shorten_to_initial_path(pos->file->name);
+			rc = asprintf(&pos_str, "%s:%d", fname,
 			      (first_line) ? pos->first_line: pos->last_line);
+		}
 	}
 	else {
-		rc = asprintf(&pos_str, "%s:<no-line>", fname);
+		if (full)
+			rc = asprintf(&pos_str, "%s:<no-line>", fname);
+		else
+			return NULL;
 	}
 
 	if (rc == -1)
@@ -385,15 +396,15 @@ srcpos_string_short(struct srcpos *pos, bool first_line)
 }
 
 char *
-srcpos_string_first(struct srcpos *pos)
+srcpos_string_first(struct srcpos *pos, bool full)
 {
-	return srcpos_string_short(pos, true);
+	return srcpos_string_short(pos, true, full);
 }
 
 char *
-srcpos_string_last(struct srcpos *pos)
+srcpos_string_last(struct srcpos *pos, bool full)
 {
-	return srcpos_string_short(pos, false);
+	return srcpos_string_short(pos, false, full);
 }
 
 void srcpos_verror(struct srcpos *pos, const char *prefix,
diff --git a/srcpos.h b/srcpos.h
index 9281cba..0f1dde4 100644
--- a/srcpos.h
+++ b/srcpos.h
@@ -109,8 +109,8 @@ extern struct srcpos *srcpos_copy_all(struct srcpos *pos);
 extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
 	 struct srcpos *right_srcpos);
 extern char *srcpos_string(struct srcpos *pos);
-extern char *srcpos_string_first(struct srcpos *pos);
-extern char *srcpos_string_last(struct srcpos *pos);
+extern char *srcpos_string_first(struct srcpos *pos, bool full);
+extern char *srcpos_string_last(struct srcpos *pos, bool full);
 
 extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
 					const char *fmt, va_list va);
diff --git a/treesource.c b/treesource.c
index 9a22b5e..e6af67e 100644
--- a/treesource.c
+++ b/treesource.c
@@ -203,13 +203,16 @@ static void write_propval(FILE *f, struct property *prop)
 	char *srcstr;
 
 	if (len == 0) {
+		fprintf(f, ";");
 		if (annotate) {
-			srcstr = srcpos_string_first(prop->srcpos);
-			fprintf(f, "; /* %s */\n", srcstr);
-			free(srcstr);
-		} else {
-			fprintf(f, ";\n");
+			srcstr = srcpos_string_first(prop->srcpos,
+						     annotate_full);
+			if (srcstr) {
+				fprintf(f, " /* %s */", srcstr);
+				free(srcstr);
+			}
 		}
+		fprintf(f, "\n");
 		return;
 	}
 
@@ -237,13 +240,15 @@ static void write_propval(FILE *f, struct property *prop)
 		write_propval_bytes(f, prop->val);
 	}
 
+	fprintf(f, ";");
 	if (annotate) {
-		srcstr = srcpos_string_first(prop->srcpos);
-		fprintf(f, "; /* %s */\n", srcstr);
-		free(srcstr);
-	} else {
-		fprintf(f, ";\n");
+		srcstr = srcpos_string_first(prop->srcpos, annotate_full);
+		if (srcstr) {
+			fprintf(f, " /* %s */", srcstr);
+			free(srcstr);
+		}
 	}
+	fprintf(f, "\n");
 }
 
 static void write_tree_source_node(FILE *f, struct node *tree, int level)
@@ -262,12 +267,13 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 		fprintf(f, "/ {");
 
 	if (annotate) {
-		srcstr = srcpos_string_first(tree->srcpos);
-		fprintf(f, " /* %s */\n", srcstr);
-		free(srcstr);
-	} else {
-		fprintf(f, "\n");
+		srcstr = srcpos_string_first(tree->srcpos, annotate_full);
+		if (srcstr) {
+			fprintf(f, " /* %s */", srcstr);
+			free(srcstr);
+		}
 	}
+	fprintf(f, "\n");
 
 	for_each_property(tree, prop) {
 		write_prefix(f, level+1);
@@ -281,13 +287,15 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 		write_tree_source_node(f, child, level+1);
 	}
 	write_prefix(f, level);
+	fprintf(f, "};");
 	if (annotate) {
-		srcstr = srcpos_string_last(tree->srcpos);
-		fprintf(f, "}; /* %s */\n", srcstr);
-		free(srcstr);
-	} else {
-		fprintf(f, "};\n");
+		srcstr = srcpos_string_last(tree->srcpos, annotate_full);
+		if (srcstr) {
+			fprintf(f, " /* %s */", srcstr);
+			free(srcstr);
+		}
 	}
+	fprintf(f, "\n");
 }
 
 
-- 
2.7.4

Re: [PATCH 5/5] annotations: add --annotate-full option

[Frank Rowand]

On 01/08/18 05:36, Julia Lawall wrote:
> With --annotate, one gets only short file names and line numbers.
> 
> With --annotate-full, one gets complete paths, starting and ending line
> numbers and starting and ending columns.
> 
> --annotate-full indicates no-file and no-line for things that are not
> connected to the source code (fixups, symbols, aliases, etc.).
> --annotate simply has nothing in those cases.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> ---
>  dtc.c        | 13 +++++++++----
>  dtc.h        |  1 +
>  srcpos.c     | 27 +++++++++++++++++++--------
>  srcpos.h     |  4 ++--
>  treesource.c | 48 ++++++++++++++++++++++++++++--------------------
>  5 files changed, 59 insertions(+), 34 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index 371d04c..24b58eb 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -35,7 +35,8 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
>  int generate_symbols;	/* enable symbols & fixup support */
>  int generate_fixups;		/* suppress generation of fixups on symbol support */
>  int auto_label_aliases;		/* auto generate labels -> aliases */
> -bool annotate = false; /* annotate .dts with input source location */
> +bool annotate = false;		/* annotate .dts with input source location */
> +bool annotate_full = false; /* annotate .dts with full input source location */
>  
>  static int is_power_of_2(int x)
>  {
> @@ -61,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>  
>  /* Usage related data. */
>  static const char usage_synopsis[] = "dtc [options] <input file>";
> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@Ahv";
> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@ATFhv";
>  static struct option const usage_long_opts[] = {
>  	{"quiet",            no_argument, NULL, 'q'},
>  	{"in-format",         a_argument, NULL, 'I'},
> @@ -82,9 +83,10 @@ static struct option const usage_long_opts[] = {
>  	{"error",             a_argument, NULL, 'E'},
>  	{"symbols",	     no_argument, NULL, '@'},
>  	{"auto-alias",       no_argument, NULL, 'A'},
> +	{"annotate",         no_argument, NULL, 'T'},
> +	{"annotate-full",    no_argument, NULL, 'F'},
>  	{"help",             no_argument, NULL, 'h'},
>  	{"version",          no_argument, NULL, 'v'},
> -	{"annotate",         no_argument, NULL, 'T'},
>  	{NULL,               no_argument, NULL, 0x0},
>  };
>  static const char * const usage_opts_help[] = {
> @@ -119,6 +121,7 @@ static const char * const usage_opts_help[] = {
>  	"\n\tPrint this help and exit",

I am getting a really weird result.  This patch fragment is clearly moving
the "annotate" and "annotate-full" lines to before the "help" line.  But
for some reason my version of patch is placing the "annotate" and
"annotate-full" lines after the "version" line.  So the patch tool seems
to be broken ?????


$ patch -v
GNU patch 2.7.1


-Frank

Re: [PATCH 5/5] annotations: add --annotate-full option

[Julia Lawall]

On Tue, 9 Jan 2018, Frank Rowand wrote:

> On 01/08/18 05:36, Julia Lawall wrote:
> > With --annotate, one gets only short file names and line numbers.
> >
> > With --annotate-full, one gets complete paths, starting and ending line
> > numbers and starting and ending columns.
> >
> > --annotate-full indicates no-file and no-line for things that are not
> > connected to the source code (fixups, symbols, aliases, etc.).
> > --annotate simply has nothing in those cases.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> > ---
> >  dtc.c        | 13 +++++++++----
> >  dtc.h        |  1 +
> >  srcpos.c     | 27 +++++++++++++++++++--------
> >  srcpos.h     |  4 ++--
> >  treesource.c | 48 ++++++++++++++++++++++++++++--------------------
> >  5 files changed, 59 insertions(+), 34 deletions(-)
> >
> > diff --git a/dtc.c b/dtc.c
> > index 371d04c..24b58eb 100644
> > --- a/dtc.c
> > +++ b/dtc.c
> > @@ -35,7 +35,8 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
> >  int generate_symbols;	/* enable symbols & fixup support */
> >  int generate_fixups;		/* suppress generation of fixups on symbol support */
> >  int auto_label_aliases;		/* auto generate labels -> aliases */
> > -bool annotate = false; /* annotate .dts with input source location */
> > +bool annotate = false;		/* annotate .dts with input source location */
> > +bool annotate_full = false; /* annotate .dts with full input source location */
> >
> >  static int is_power_of_2(int x)
> >  {
> > @@ -61,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >
> >  /* Usage related data. */
> >  static const char usage_synopsis[] = "dtc [options] <input file>";
> > -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@Ahv";
> > +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@ATFhv";
> >  static struct option const usage_long_opts[] = {
> >  	{"quiet",            no_argument, NULL, 'q'},
> >  	{"in-format",         a_argument, NULL, 'I'},
> > @@ -82,9 +83,10 @@ static struct option const usage_long_opts[] = {
> >  	{"error",             a_argument, NULL, 'E'},
> >  	{"symbols",	     no_argument, NULL, '@'},
> >  	{"auto-alias",       no_argument, NULL, 'A'},
> > +	{"annotate",         no_argument, NULL, 'T'},
> > +	{"annotate-full",    no_argument, NULL, 'F'},
> >  	{"help",             no_argument, NULL, 'h'},
> >  	{"version",          no_argument, NULL, 'v'},
> > -	{"annotate",         no_argument, NULL, 'T'},
> >  	{NULL,               no_argument, NULL, 0x0},
> >  };
> >  static const char * const usage_opts_help[] = {
> > @@ -119,6 +121,7 @@ static const char * const usage_opts_help[] = {
> >  	"\n\tPrint this help and exit",
>
> I am getting a really weird result.  This patch fragment is clearly moving
> the "annotate" and "annotate-full" lines to before the "help" line.  But
> for some reason my version of patch is placing the "annotate" and
> "annotate-full" lines after the "version" line.  So the patch tool seems
> to be broken ?????

That seems very strange.  They will be in the right place from the start
in the next version.

julia


>
>
> $ patch -v
> GNU patch 2.7.1
>
>
> -Frank
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 5/5] annotations: add --annotate-full option

[David Gibson]

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

On Mon, Jan 08, 2018 at 02:36:47PM +0100, Julia Lawall wrote:
> With --annotate, one gets only short file names and line numbers.
> 
> With --annotate-full, one gets complete paths, starting and ending line
> numbers and starting and ending columns.
> 
> --annotate-full indicates no-file and no-line for things that are not
> connected to the source code (fixups, symbols, aliases, etc.).
> --annotate simply has nothing in those cases.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>

Looks fine, except that it will obviously need a little rework to go
on top of the revised earlier patches.

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