[PATCH 0/3 v2] annotations

[PATCH 0/3 v2] annotations

[Julia Lawall]

A second attempt at supporting file location annotations.

The main change is support for a list of positions, as needed for
merge_nodes.  I have also reorganized the patches to separate position
information from annotations and addressed many other small issues.

[PATCH 1/3 v2] annotations: check for NULL position

[Julia Lawall]

Check for NULL position and NULL filename.

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

v2: Use xasprintf without result check.  Only use fname when it is defined.

 srcpos.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/srcpos.c b/srcpos.c
index e8fced9..9067ba3 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -250,12 +250,20 @@ 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)
-		fname = pos->file->name;
+	if (!pos) {
+		xasprintf(&pos_str, "<no-file>:<no-line>");
+		return pos_str;
+	}
 
+	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,
-- 
2.7.4

[PATCH 2/3 v2] annotations: add positions

[Julia Lawall]

The parser is extended to record positions.  For productions with a
nodedef, this is the position of the nodedef, with preceding / if any.
For other productions it is the position of the complete set of terms
matched by the production.

srcpos structures are added to the property and node types, and to the
parameter lists of various functions that construct these types.  Nodes
and properties that are created by the compiler rather than from parsing
source code typically have NULL as the srcpos value.

merge_nodes, defined in livetree.c uses srcpos_extend to combine
multiple positions, resulting in a list of positions.

srcpos_extend is defined in srcpos.c and is somewhat complex.  It
addresses two issues: the lists of positions to merge may share a common
tail and the lists of positions to merge may contain duplicates.  In
srcpos_extend, the nested loops detect a common tail (merge_point) if
any.  The second list is then copied up to the merge point, and then the
resulting copy is placed at the end of a copy of the first list.
Duplicates are detected by srcpos_copy_list within the copying process.

srcpos_combine is also newly defined in srcpos.c.  It is only used to
merge positions of adjacent terms matched by the parser, and thus
doesn't have to be concerned with common tails and duplicates.

Another change to srcpos.c is to make srcpos_copy always do a full copy,
including a copy of the file substructure.  This is required because
when dtc is used on the output of cpp, the successive detected file
names overwrite the file name in the file structure.

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

v2: Allow lists of positions, make srcpos_copy always do a full copy, move
annotation support into another patch.

 dtc-parser.y | 23 +++++++++++---------
 dtc.h        | 13 +++++++----
 flattree.c   |  2 +-
 fstree.c     |  8 ++++---
 livetree.c   | 44 ++++++++++++++++++++++++++-----------
 srcpos.c     | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 srcpos.h     |  5 +++++
 7 files changed, 135 insertions(+), 31 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.h b/dtc.h
index 3b18a42..3a85058 100644
--- a/dtc.h
+++ b/dtc.h
@@ -149,6 +149,7 @@ struct property {
 	struct property *next;
 
 	struct label *labels;
+	struct srcpos *srcpos;
 };
 
 struct node {
@@ -168,6 +169,7 @@ struct node {
 
 	struct label *labels;
 	const struct bus_type *bus;
+	struct srcpos *srcpos;
 };
 
 #define for_each_label_withdel(l0, l) \
@@ -194,17 +196,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..9317739 100644
--- a/livetree.c
+++ b/livetree.c
@@ -19,6 +19,7 @@
  */
 
 #include "dtc.h"
+#include "srcpos.h"
 
 /*
  * Tree building functions
@@ -50,7 +51,8 @@ void delete_labels(struct label **labels)
 		label->deleted = 1;
 }
 
-struct property *build_property(char *name, struct data val)
+struct property *build_property(char *name, struct data val,
+				struct srcpos *srcpos)
 {
 	struct property *new = xmalloc(sizeof(*new));
 
@@ -58,6 +60,7 @@ struct property *build_property(char *name, struct data val)
 
 	new->name = name;
 	new->val = val;
+	new->srcpos = srcpos_copy(srcpos);
 
 	return new;
 }
@@ -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(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,9 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 			add_child(old_node, new_child);
 	}
 
+	old_node->srcpos =
+		srcpos_extend(new_node_begin_srcpos, old_node->srcpos);
+
 	/* The new node contents are now merged into the old node.  Free
 	 * the new node. */
 	free(new_node);
@@ -216,7 +226,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 +238,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 +342,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 +599,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 +784,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 +843,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 9067ba3..2ed794b 100644
--- a/srcpos.c
+++ b/srcpos.c
@@ -207,6 +207,7 @@ struct srcpos srcpos_empty = {
 	.last_line = 0,
 	.last_column = 0,
 	.file = NULL,
+	.next = NULL,
 };
 
 #define TAB_SIZE      8
@@ -240,13 +241,83 @@ struct srcpos *
 srcpos_copy(struct srcpos *pos)
 {
 	struct srcpos *pos_new;
+	struct srcfile_state *srcfile_state;
+
+	if (!pos)
+		return NULL;
 
 	pos_new = xmalloc(sizeof(struct srcpos));
 	memcpy(pos_new, pos, sizeof(struct srcpos));
 
+	if (pos_new) {
+		/* 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;
 }
 
+static bool same_pos(struct srcpos *p1, struct srcpos *p2)
+{
+	return (p1->first_line == p2->first_line &&
+		p1->first_column == p2->first_column &&
+		p1->last_line == p2->last_line &&
+		p1->last_column == p2->last_column &&
+		!strcmp(p1->file->name, p2->file->name));
+}
+
+static struct srcpos *srcpos_copy_list(struct srcpos *pos, struct srcpos *end,
+				       struct srcpos *newtail)
+{
+	struct srcpos *next, *p;
+
+	if (pos == end)
+		return newtail;
+
+	next = srcpos_copy_list(pos->next, end, newtail);
+
+	for (p = next; p != NULL; p = p->next)
+		if (same_pos(pos, p))
+			return next;
+
+	pos = srcpos_copy(pos);
+	pos->next = next;
+	return pos;
+}
+
+struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
+			     struct srcpos *old_srcpos)
+{
+	struct srcpos *pos, *p;
+	struct srcpos *merge_point = NULL;
+
+	for (pos = new_srcpos; pos->next != NULL; pos = pos->next)
+		for (p = old_srcpos; p != NULL; p = p->next)
+			if (pos->next == p) {
+				merge_point = p;
+				goto after;
+			}
+after:
+	old_srcpos = srcpos_copy_list(old_srcpos, merge_point, NULL);
+	return srcpos_copy_list(new_srcpos, NULL, old_srcpos);
+}
+
 char *
 srcpos_string(struct srcpos *pos)
 {
diff --git a/srcpos.h b/srcpos.h
index 9ded12a..c22d6ba 100644
--- a/srcpos.h
+++ b/srcpos.h
@@ -74,6 +74,7 @@ struct srcpos {
     int last_line;
     int last_column;
     struct srcfile_state *file;
+    struct srcpos *next;
 };
 
 #define YYLTYPE struct srcpos
@@ -105,6 +106,10 @@ 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_combine(struct srcpos *left_srcpos,
+				     struct srcpos *right_srcpos);
+extern struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
+				    struct srcpos *old_srcpos);
 extern char *srcpos_string(struct srcpos *pos);
 
 extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
-- 
2.7.4

[PATCH 3/3 v2] annotations: add the annotation functionality

[Julia Lawall]

This commit provides two new command-line options:

--annotate (abbreviated -T)
--annotate-full (abbreviated -F)

--annotate provides one or more filenames and line numbers indicating
the origin of a given line.  The filename is expressed relative the the
filename provided on the command line.  Nothing is printed for overlays,
etc.

--annotate-full provides one or more tuples of: filename, starting line,
starting column, ending line ending column.  The full path is given for
the file name.  Overlays, etc are annotated with <no-file>:<no-line>.

There are numerous changes in srcpos to provide the relative filenames
(variables initial_path, initial_pathlen and initial_cpp, new functions
set_initial_path and shorten_to_initial_path, and changes in
srcfile_push and srcpos_set_line).  The change in srcpos_set_line takes
care of the case where cpp is used as a preprocessor.  In that case the
initial file name is not the one provided on the command line but the
one found at the beginnning of the cpp output.

The new functions srcpos_string_comment, srcpos_string_first, and
srcpos_string_last print the annotations.  srcpos_string_comment is
recursive to print a list of source file positions.

Note that the column numbers in the --annotate-full case may be of
limited use when cpp is first used, because the columns are those in the
output generated by cpp, which are not the same as the ones in the
source code.  It may be that cpp simply replaces tabs by spaces.

Various changes are sprinkled throughout treesource.c to print the
annotations.

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

v2: Squash all of the annotation support into a single patch, enable
printing a list of positions.

 dtc.c        |  17 ++++++++-
 dtc.h        |   2 ++
 srcpos.c     | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 srcpos.h     |   2 ++
 treesource.c |  47 +++++++++++++++++++++----
 5 files changed, 169 insertions(+), 10 deletions(-)

diff --git a/dtc.c b/dtc.c
index c36994e..0153b1a 100644
--- a/dtc.c
+++ b/dtc.c
@@ -35,6 +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_full; /* =false, annotate .dts with full input source location */
 
 static int is_power_of_2(int x)
 {
@@ -60,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'},
@@ -81,6 +83,8 @@ 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'},
 	{NULL,               no_argument, NULL, 0x0},
@@ -114,6 +118,8 @@ static const char * const usage_opts_help[] = {
 	"\n\tEnable/disable errors (prefix with \"no-\")",
 	"\n\tEnable generation of symbols",
 	"\n\tEnable auto-alias of labels",
+	"\n\tAnnotate output .dts with input source file and line",
+	"\n\tAnnotate output .dts with input source file (full path) line and column",
 	"\n\tPrint this help and exit",
 	"\n\tPrint version and exit",
 	NULL,
@@ -259,6 +265,11 @@ int main(int argc, char *argv[])
 		case 'A':
 			auto_label_aliases = 1;
 			break;
+		case 'F':
+			annotate_full = true;
+		case 'T':
+			annotate = true;
+			break;
 
 		case 'h':
 			usage(NULL);
@@ -297,6 +308,10 @@ int main(int argc, char *argv[])
 				outform = "dts";
 		}
 	}
+
+	if (annotate && (!streq(inform, "dts") || !streq(outform, "dts")))
+		die("--annotate and --annotate-full require -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 3a85058..0b8141b 100644
--- a/dtc.h
+++ b/dtc.h
@@ -58,6 +58,8 @@ 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 */
+extern bool annotate_full;   /* annotate .dts with detailed source location */
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
diff --git a/srcpos.c b/srcpos.c
index 2ed794b..04e78ba 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,51 @@ 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; /* = 0 */
+static bool initial_cpp = true;
 
-/* Detect infinite include recursion. */
-#define MAX_SRCFILE_DEPTH     (100)
-static int srcfile_depth; /* = 0 */
+static void set_initial_path(char *fname)
+{
+	int i, len = strlen(fname);
+
+	xasprintf(&initial_path, "%s", fname);
+	initial_pathlen = 0;
+	for (i = 0; i != len; i++)
+		if (initial_path[i] == '/')
+			initial_pathlen++;
+}
 
+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 +200,9 @@ void srcfile_push(const char *fname)
 	srcfile->colno = 0;
 
 	current_srcfile = srcfile;
+
+	if (srcfile_depth == 1)
+		set_initial_path(srcfile->name);
 }
 
 bool srcfile_pop(void)
@@ -351,6 +397,60 @@ srcpos_string(struct srcpos *pos)
 	return pos_str;
 }
 
+static char *
+srcpos_string_comment(struct srcpos *pos, bool first_line, bool full)
+{
+	char *pos_str, *fname, *first, *rest;
+
+	if (!pos) {
+		if (full) {
+			xasprintf(&pos_str, "<no-file>:<no-line>");
+			return pos_str;
+		} else {
+			return NULL;
+		}
+	}
+
+	if (!pos->file)
+		fname = "<no-file>";
+	else if (!pos->file->name)
+		fname = "<no-filename>";
+	else if (full)
+		fname = pos->file->name;
+	else
+		fname = shorten_to_initial_path(pos->file->name);
+
+	if (full)
+		xasprintf(&first, "%s:%d:%d-%d:%d", fname,
+			  pos->first_line, pos->first_column,
+			  pos->last_line, pos->last_column);
+	else
+		xasprintf(&first, "%s:%d", fname,
+			  first_line ? pos->first_line : pos->last_line);
+
+	if (pos->next != NULL) {
+		rest = srcpos_string_comment(pos->next, first_line, full);
+		xasprintf(&pos_str, "%s, %s", first, rest);
+		free(first);
+		free(rest);
+	} else {
+		pos_str = first;
+	}
+
+	return pos_str;
+}
+
+char *srcpos_string_first(struct srcpos *pos, bool full)
+{
+	return srcpos_string_comment(pos, true, full);
+}
+
+char *srcpos_string_last(struct srcpos *pos, bool full)
+{
+	return srcpos_string_comment(pos, false, full);
+}
+
+
 void srcpos_verror(struct srcpos *pos, const char *prefix,
 		   const char *fmt, va_list va)
 {
@@ -379,4 +479,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);
+	}
 }
diff --git a/srcpos.h b/srcpos.h
index c22d6ba..a538c4e 100644
--- a/srcpos.h
+++ b/srcpos.h
@@ -111,6 +111,8 @@ extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
 extern struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
 				    struct srcpos *old_srcpos);
 extern char *srcpos_string(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 2461a3d..f454ba4 100644
--- a/treesource.c
+++ b/treesource.c
@@ -199,10 +199,20 @@ static void write_propval(FILE *f, struct property *prop)
 	struct marker *m = prop->val.markers;
 	int nnotstring = 0, nnul = 0;
 	int nnotstringlbl = 0, nnotcelllbl = 0;
+	char *srcstr;
 	int i;
 
 	if (len == 0) {
-		fprintf(f, ";\n");
+		fprintf(f, ";");
+		if (annotate) {
+			srcstr = srcpos_string_first(prop->srcpos,
+						     annotate_full);
+			if (srcstr) {
+				fprintf(f, " /* %s */", srcstr);
+				free(srcstr);
+			}
+		}
+		fprintf(f, "\n");
 		return;
 	}
 
@@ -230,7 +240,15 @@ static void write_propval(FILE *f, struct property *prop)
 		write_propval_bytes(f, prop->val);
 	}
 
-	fprintf(f, ";\n");
+	fprintf(f, ";");
+	if (annotate) {
+		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)
@@ -238,14 +256,24 @@ 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_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);
@@ -259,10 +287,17 @@ 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");
+	fprintf(f, "};");
+	if (annotate) {
+		srcstr = srcpos_string_last(tree->srcpos, annotate_full);
+		if (srcstr) {
+			fprintf(f, " /* %s */", srcstr);
+			free(srcstr);
+		}
+	}
+	fprintf(f, "\n");
 }
 
-
 void dt_to_source(FILE *f, struct dt_info *dti)
 {
 	struct reserve_info *re;
-- 
2.7.4

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

[David Gibson]

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

On Mon, Jan 15, 2018 at 07:24:09PM +0100, Julia Lawall wrote:
> The parser is extended to record positions.  For productions with a
> nodedef, this is the position of the nodedef, with preceding / if any.
> For other productions it is the position of the complete set of terms
> matched by the production.
> 
> srcpos structures are added to the property and node types, and to the
> parameter lists of various functions that construct these types.  Nodes
> and properties that are created by the compiler rather than from parsing
> source code typically have NULL as the srcpos value.
> 
> merge_nodes, defined in livetree.c uses srcpos_extend to combine
> multiple positions, resulting in a list of positions.
> 
> srcpos_extend is defined in srcpos.c and is somewhat complex.  It
> addresses two issues: the lists of positions to merge may share a common
> tail and the lists of positions to merge may contain duplicates.  In
> srcpos_extend, the nested loops detect a common tail (merge_point) if
> any.  The second list is then copied up to the merge point, and then the
> resulting copy is placed at the end of a copy of the first list.
> Duplicates are detected by srcpos_copy_list within the copying process.
> 
> srcpos_combine is also newly defined in srcpos.c.  It is only used to
> merge positions of adjacent terms matched by the parser, and thus
> doesn't have to be concerned with common tails and duplicates.
> 
> Another change to srcpos.c is to make srcpos_copy always do a full copy,
> including a copy of the file substructure.  This is required because
> when dtc is used on the output of cpp, the successive detected file
> names overwrite the file name in the file structure.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
> ---
> 
> v2: Allow lists of positions, make srcpos_copy always do a full copy, move
> annotation support into another patch.
> 
>  dtc-parser.y | 23 +++++++++++---------
>  dtc.h        | 13 +++++++----
>  flattree.c   |  2 +-
>  fstree.c     |  8 ++++---
>  livetree.c   | 44 ++++++++++++++++++++++++++-----------
>  srcpos.c     | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  srcpos.h     |  5 +++++
>  7 files changed, 135 insertions(+), 31 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, "", &@$);

Attaching the location at name_node time isn't necessarily wrong, but
it seems a bit odd.  Is there a reason not to do it at build_node()
time?  It means you wouldn't include the node name in the definition,
but I think we can live with that.  Putting the location tagging in
the nodedef production also means slightly fewer places need changing.

>  		}
>  	| 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);

Why does the location get attached in the add_orphan_node() rather
than the name_node() (or build_node()) within?  It seems odd to pass
NULL as a location to name_node(), then attach a real location almost
immediately afterwards.

>  		}
>  	| 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);

Passing a location to merge_nodes() also seems odd.  Shouldn't the
location information already be in the things you're merging?

>  			} 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.h b/dtc.h
> index 3b18a42..3a85058 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -149,6 +149,7 @@ struct property {
>  	struct property *next;
>  
>  	struct label *labels;
> +	struct srcpos *srcpos;
>  };
>  
>  struct node {
> @@ -168,6 +169,7 @@ struct node {
>  
>  	struct label *labels;
>  	const struct bus_type *bus;
> +	struct srcpos *srcpos;
>  };
>  
>  #define for_each_label_withdel(l0, l) \
> @@ -194,17 +196,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..9317739 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "dtc.h"
> +#include "srcpos.h"
>  
>  /*
>   * Tree building functions
> @@ -50,7 +51,8 @@ void delete_labels(struct label **labels)
>  		label->deleted = 1;
>  }
>  
> -struct property *build_property(char *name, struct data val)
> +struct property *build_property(char *name, struct data val,
> +				struct srcpos *srcpos)
>  {
>  	struct property *new = xmalloc(sizeof(*new));
>  
> @@ -58,6 +60,7 @@ struct property *build_property(char *name, struct data val)
>  
>  	new->name = name;
>  	new->val = val;
> +	new->srcpos = srcpos_copy(srcpos);
>  
>  	return new;
>  }
> @@ -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(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,9 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  			add_child(old_node, new_child);
>  	}
>  
> +	old_node->srcpos =
> +		srcpos_extend(new_node_begin_srcpos, old_node->srcpos);
> +
>  	/* The new node contents are now merged into the old node.  Free
>  	 * the new node. */
>  	free(new_node);
> @@ -216,7 +226,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 +238,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 +342,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 +599,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 +784,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 +843,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 9067ba3..2ed794b 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -207,6 +207,7 @@ struct srcpos srcpos_empty = {
>  	.last_line = 0,
>  	.last_column = 0,
>  	.file = NULL,
> +	.next = NULL,
>  };
>  
>  #define TAB_SIZE      8
> @@ -240,13 +241,83 @@ struct srcpos *
>  srcpos_copy(struct srcpos *pos)
>  {
>  	struct srcpos *pos_new;
> +	struct srcfile_state *srcfile_state;
> +
> +	if (!pos)
> +		return NULL;
>  
>  	pos_new = xmalloc(sizeof(struct srcpos));
>  	memcpy(pos_new, pos, sizeof(struct srcpos));
>  
> +	if (pos_new) {

No need to test this, xmalloc() will abort() on failure.

> +		/* allocate without free */

Ah, yeah.  Lifetime management is a bit of a mess in dtc.  I've
seriously considered just removing every free() and treating the
process lifetime as a poor man's pool allocator.

> +		srcfile_state = xmalloc(sizeof(struct srcfile_state));
> +		memcpy(srcfile_state, pos->file, sizeof(struct srcfile_state));
> +
> +		pos_new->file = srcfile_state;
> +	}

Since you've now added a list to srcpos, don't you need to deep copy
that as well?

<reads ahead in the patch>Hrm.  Maybe not.  I think conflation of a
"simple" srcpos with one location and a "complex" one which could
include a list into the same structure is causing some unnecessary
confusion.

> +
> +	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;
>  }
>  
> +static bool same_pos(struct srcpos *p1, struct srcpos *p2)
> +{
> +	return (p1->first_line == p2->first_line &&
> +		p1->first_column == p2->first_column &&
> +		p1->last_line == p2->last_line &&
> +		p1->last_column == p2->last_column &&
> +		!strcmp(p1->file->name, p2->file->name));

Use streq() here - it exists specifically because I always get
confused by the sense of strcmp() when treated as a boolean.

> +}
> +
> +static struct srcpos *srcpos_copy_list(struct srcpos *pos, struct srcpos *end,
> +				       struct srcpos *newtail)
> +{
> +	struct srcpos *next, *p;
> +
> +	if (pos == end)
> +		return newtail;
> +
> +	next = srcpos_copy_list(pos->next, end, newtail);
> +
> +	for (p = next; p != NULL; p = p->next)
> +		if (same_pos(pos, p))
> +			return next;
> +
> +	pos = srcpos_copy(pos);
> +	pos->next = next;
> +	return pos;
> +}
> +
> +struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
> +			     struct srcpos *old_srcpos)
> +{
> +	struct srcpos *pos, *p;
> +	struct srcpos *merge_point = NULL;
> +
> +	for (pos = new_srcpos; pos->next != NULL; pos = pos->next)
> +		for (p = old_srcpos; p != NULL; p = p->next)
> +			if (pos->next == p) {
> +				merge_point = p;
> +				goto after;
> +			}
> +after:
> +	old_srcpos = srcpos_copy_list(old_srcpos, merge_point, NULL);
> +	return srcpos_copy_list(new_srcpos, NULL, old_srcpos);
> +}
> +
>  char *
>  srcpos_string(struct srcpos *pos)
>  {
> diff --git a/srcpos.h b/srcpos.h
> index 9ded12a..c22d6ba 100644
> --- a/srcpos.h
> +++ b/srcpos.h
> @@ -74,6 +74,7 @@ struct srcpos {
>      int last_line;
>      int last_column;
>      struct srcfile_state *file;
> +    struct srcpos *next;
>  };
>  
>  #define YYLTYPE struct srcpos
> @@ -105,6 +106,10 @@ 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_combine(struct srcpos *left_srcpos,
> +				     struct srcpos *right_srcpos);
> +extern struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
> +				    struct srcpos *old_srcpos);
>  extern char *srcpos_string(struct srcpos *pos);
>  
>  extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3 v2] annotations: check for NULL position

[David Gibson]

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

On Mon, Jan 15, 2018 at 07:24:08PM +0100, Julia Lawall wrote:
> Check for NULL position and NULL filename.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

> ---
> 
> v2: Use xasprintf without result check.  Only use fname when it is defined.
> 
>  srcpos.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/srcpos.c b/srcpos.c
> index e8fced9..9067ba3 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -250,12 +250,20 @@ 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)
> -		fname = pos->file->name;
> +	if (!pos) {
> +		xasprintf(&pos_str, "<no-file>:<no-line>");
> +		return pos_str;
> +	}
>  
> +	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,

-- 
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/3 v2] annotations: add the annotation functionality

[David Gibson]

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

On Mon, Jan 15, 2018 at 07:24:10PM +0100, Julia Lawall wrote:
> This commit provides two new command-line options:
> 
> --annotate (abbreviated -T)
> --annotate-full (abbreviated -F)

What's the rationale for having two different versions of the
annotations?

> 
> --annotate provides one or more filenames and line numbers indicating
> the origin of a given line.  The filename is expressed relative the the
> filename provided on the command line.  Nothing is printed for overlays,
> etc.
> 
> --annotate-full provides one or more tuples of: filename, starting line,
> starting column, ending line ending column.  The full path is given for
> the file name.  Overlays, etc are annotated with <no-file>:<no-line>.
> 
> There are numerous changes in srcpos to provide the relative filenames
> (variables initial_path, initial_pathlen and initial_cpp, new functions
> set_initial_path and shorten_to_initial_path, and changes in
> srcfile_push and srcpos_set_line).  The change in srcpos_set_line takes
> care of the case where cpp is used as a preprocessor.  In that case the
> initial file name is not the one provided on the command line but the
> one found at the beginnning of the cpp output.
> 
> The new functions srcpos_string_comment, srcpos_string_first, and
> srcpos_string_last print the annotations.  srcpos_string_comment is
> recursive to print a list of source file positions.
> 
> Note that the column numbers in the --annotate-full case may be of
> limited use when cpp is first used, because the columns are those in the
> output generated by cpp, which are not the same as the ones in the
> source code.  It may be that cpp simply replaces tabs by spaces.
> 
> Various changes are sprinkled throughout treesource.c to print the
> annotations.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
> ---
> 
> v2: Squash all of the annotation support into a single patch, enable
> printing a list of positions.
> 
>  dtc.c        |  17 ++++++++-
>  dtc.h        |   2 ++
>  srcpos.c     | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  srcpos.h     |   2 ++
>  treesource.c |  47 +++++++++++++++++++++----
>  5 files changed, 169 insertions(+), 10 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index c36994e..0153b1a 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -35,6 +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_full; /* =false, annotate .dts with full input source location */
>  
>  static int is_power_of_2(int x)
>  {
> @@ -60,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'},
> @@ -81,6 +83,8 @@ 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'},
>  	{NULL,               no_argument, NULL, 0x0},
> @@ -114,6 +118,8 @@ static const char * const usage_opts_help[] = {
>  	"\n\tEnable/disable errors (prefix with \"no-\")",
>  	"\n\tEnable generation of symbols",
>  	"\n\tEnable auto-alias of labels",
> +	"\n\tAnnotate output .dts with input source file and line",
> +	"\n\tAnnotate output .dts with input source file (full path) line and column",
>  	"\n\tPrint this help and exit",
>  	"\n\tPrint version and exit",
>  	NULL,
> @@ -259,6 +265,11 @@ int main(int argc, char *argv[])
>  		case 'A':
>  			auto_label_aliases = 1;
>  			break;
> +		case 'F':
> +			annotate_full = true;

Uncommented fallthrough.  Please don't do that.

> +		case 'T':
> +			annotate = true;
> +			break;
>  
>  		case 'h':
>  			usage(NULL);
> @@ -297,6 +308,10 @@ int main(int argc, char *argv[])
>  				outform = "dts";
>  		}
>  	}
> +
> +	if (annotate && (!streq(inform, "dts") || !streq(outform, "dts")))
> +		die("--annotate and --annotate-full require -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 3a85058..0b8141b 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -58,6 +58,8 @@ 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 */
> +extern bool annotate_full;   /* annotate .dts with detailed source location */
>  
>  #define PHANDLE_LEGACY	0x1
>  #define PHANDLE_EPAPR	0x2
> diff --git a/srcpos.c b/srcpos.c
> index 2ed794b..04e78ba 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,51 @@ 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; /* = 0 */
> +static bool initial_cpp = true;
>  
> -/* Detect infinite include recursion. */
> -#define MAX_SRCFILE_DEPTH     (100)
> -static int srcfile_depth; /* = 0 */
> +static void set_initial_path(char *fname)
> +{
> +	int i, len = strlen(fname);
> +
> +	xasprintf(&initial_path, "%s", fname);
> +	initial_pathlen = 0;
> +	for (i = 0; i != len; i++)
> +		if (initial_path[i] == '/')
> +			initial_pathlen++;
> +}
>  
> +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 +200,9 @@ void srcfile_push(const char *fname)
>  	srcfile->colno = 0;
>  
>  	current_srcfile = srcfile;
> +
> +	if (srcfile_depth == 1)
> +		set_initial_path(srcfile->name);
>  }
>  
>  bool srcfile_pop(void)
> @@ -351,6 +397,60 @@ srcpos_string(struct srcpos *pos)
>  	return pos_str;
>  }
>  
> +static char *
> +srcpos_string_comment(struct srcpos *pos, bool first_line, bool full)
> +{
> +	char *pos_str, *fname, *first, *rest;
> +
> +	if (!pos) {
> +		if (full) {
> +			xasprintf(&pos_str, "<no-file>:<no-line>");
> +			return pos_str;
> +		} else {
> +			return NULL;
> +		}
> +	}
> +
> +	if (!pos->file)
> +		fname = "<no-file>";
> +	else if (!pos->file->name)
> +		fname = "<no-filename>";
> +	else if (full)
> +		fname = pos->file->name;
> +	else
> +		fname = shorten_to_initial_path(pos->file->name);
> +
> +	if (full)
> +		xasprintf(&first, "%s:%d:%d-%d:%d", fname,
> +			  pos->first_line, pos->first_column,
> +			  pos->last_line, pos->last_column);
> +	else
> +		xasprintf(&first, "%s:%d", fname,
> +			  first_line ? pos->first_line : pos->last_line);
> +
> +	if (pos->next != NULL) {
> +		rest = srcpos_string_comment(pos->next, first_line, full);
> +		xasprintf(&pos_str, "%s, %s", first, rest);
> +		free(first);
> +		free(rest);
> +	} else {
> +		pos_str = first;
> +	}
> +
> +	return pos_str;
> +}
> +
> +char *srcpos_string_first(struct srcpos *pos, bool full)
> +{
> +	return srcpos_string_comment(pos, true, full);
> +}
> +
> +char *srcpos_string_last(struct srcpos *pos, bool full)
> +{
> +	return srcpos_string_comment(pos, false, full);
> +}
> +
> +
>  void srcpos_verror(struct srcpos *pos, const char *prefix,
>  		   const char *fmt, va_list va)
>  {
> @@ -379,4 +479,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);
> +	}
>  }
> diff --git a/srcpos.h b/srcpos.h
> index c22d6ba..a538c4e 100644
> --- a/srcpos.h
> +++ b/srcpos.h
> @@ -111,6 +111,8 @@ extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
>  extern struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
>  				    struct srcpos *old_srcpos);
>  extern char *srcpos_string(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 2461a3d..f454ba4 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -199,10 +199,20 @@ static void write_propval(FILE *f, struct property *prop)
>  	struct marker *m = prop->val.markers;
>  	int nnotstring = 0, nnul = 0;
>  	int nnotstringlbl = 0, nnotcelllbl = 0;
> +	char *srcstr;
>  	int i;
>  
>  	if (len == 0) {
> -		fprintf(f, ";\n");
> +		fprintf(f, ";");
> +		if (annotate) {
> +			srcstr = srcpos_string_first(prop->srcpos,
> +						     annotate_full);
> +			if (srcstr) {
> +				fprintf(f, " /* %s */", srcstr);
> +				free(srcstr);
> +			}
> +		}
> +		fprintf(f, "\n");
>  		return;
>  	}
>  
> @@ -230,7 +240,15 @@ static void write_propval(FILE *f, struct property *prop)
>  		write_propval_bytes(f, prop->val);
>  	}
>  
> -	fprintf(f, ";\n");
> +	fprintf(f, ";");
> +	if (annotate) {
> +		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)
> @@ -238,14 +256,24 @@ 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_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);
> @@ -259,10 +287,17 @@ 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");
> +	fprintf(f, "};");
> +	if (annotate) {
> +		srcstr = srcpos_string_last(tree->srcpos, annotate_full);
> +		if (srcstr) {
> +			fprintf(f, " /* %s */", srcstr);
> +			free(srcstr);
> +		}
> +	}
> +	fprintf(f, "\n");
>  }
>  
> -
>  void dt_to_source(FILE *f, struct dt_info *dti)
>  {
>  	struct reserve_info *re;

-- 
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/3 v2] annotations: add the annotation functionality

[Frank Rowand]

On 01/16/18 17:13, David Gibson wrote:
> On Mon, Jan 15, 2018 at 07:24:10PM +0100, Julia Lawall wrote:
>> This commit provides two new command-line options:
>>
>> --annotate (abbreviated -T)
>> --annotate-full (abbreviated -F)
> 
> What's the rationale for having two different versions of the
> annotations?

I'll put an example to try to explain, at the end of this email.


>>
>> --annotate provides one or more filenames and line numbers indicating
>> the origin of a given line.  The filename is expressed relative the the
>> filename provided on the command line.  Nothing is printed for overlays,
>> etc.
>>
>> --annotate-full provides one or more tuples of: filename, starting line,
>> starting column, ending line ending column.  The full path is given for
>> the file name.  Overlays, etc are annotated with <no-file>:<no-line>.
>>
>> There are numerous changes in srcpos to provide the relative filenames
>> (variables initial_path, initial_pathlen and initial_cpp, new functions
>> set_initial_path and shorten_to_initial_path, and changes in
>> srcfile_push and srcpos_set_line).  The change in srcpos_set_line takes
>> care of the case where cpp is used as a preprocessor.  In that case the
>> initial file name is not the one provided on the command line but the
>> one found at the beginnning of the cpp output.
>>
>> The new functions srcpos_string_comment, srcpos_string_first, and
>> srcpos_string_last print the annotations.  srcpos_string_comment is
>> recursive to print a list of source file positions.
>>
>> Note that the column numbers in the --annotate-full case may be of
>> limited use when cpp is first used, because the columns are those in the
>> output generated by cpp, which are not the same as the ones in the
>> source code.  It may be that cpp simply replaces tabs by spaces.
>>
>> Various changes are sprinkled throughout treesource.c to print the
>> annotations.
>>
>> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
>> Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

< snip >


For a real world Linux devicetree source file, I often use
arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
In these examples, I am using the version in Linux v4.15-rc7.

My basic premise is that --annotate-full provides a lot of useful
information, but that information is usually not needed.  But the
extra information makes the output very dense and much harder for
a human eye to scan or read.

In the Linux sources (an argument _against_ my case :-)), the
full path of the source files normally provides no value.  Part
of this the way our source tree is structured.

I will use the Linux script scripts/dtc/dtx_diff to generate my
examples of annotated files.  This script takes care of all of
the include paths, invoking cpp, and other details of the linux
use of dtc. When provided a single argument, dtx_diff simply does
the compile with '-O dts'.  I have modified the normal dtx_diff
to pass on --annotate and --annotate-full to dtc. 

First, I create two annotated files, one with --annotate, the other
with --annotate-full:

$ scripts/dtc/dtx_diff --annotate arch/arm/boot/dts/qcom-apq8074-dragonboard.dts >qcom-apq8074-dragonboard--annotate.dts

$ scripts/dtc/dtx_diff --annotate-full arch/arm/boot/dts/qcom-apq8074-dragonboard.dts >qcom-apq8074-dragonboard--annotate-full.dts


Then just look at the first few lines of each of the two examples:

$ head qcom-apq8074-dragonboard--annotate.dts
/dts-v1/;

/ { /* qcom-apq8074-dragonboard.dts:6, qcom-msm8974.dtsi:11, skeleton.dtsi:12 */
	#address-cells = <0x1>; /* skeleton.dtsi:13 */
	#size-cells = <0x1>; /* skeleton.dtsi:14 */
	compatible = "qcom,apq8074-dragonboard", "qcom,apq8074"; /* qcom-apq8074-dragonboard.dts:8 */
	interrupt-parent = <0x1>; /* qcom-msm8974.dtsi:14 */
	model = "Qualcomm APQ8074 Dragonboard"; /* qcom-apq8074-dragonboard.dts:7 */

	adsp-pil { /* qcom-msm8974.dtsi:243 */

$ head qcom-apq8074-dragonboard--annotate-full.dts
/dts-v1/;

/ { /* arch/arm/boot/dts/qcom-apq8074-dragonboard.dts:6:0-346:2, arch/arm/boot/dts/qcom-msm8974.dtsi:11:0-1148:2, arch/arm/boot/dts/skeleton.dtsi:12:0-18:2 */
	#address-cells = <0x1>; /* arch/arm/boot/dts/skeleton.dtsi:13:1-13:22 */
	#size-cells = <0x1>; /* arch/arm/boot/dts/skeleton.dtsi:14:1-14:19 */
	compatible = "qcom,apq8074-dragonboard", "qcom,apq8074"; /* arch/arm/boot/dts/qcom-apq8074-dragonboard.dts:8:1-8:57 */
	interrupt-parent = <0x1>; /* arch/arm/boot/dts/qcom-msm8974.dtsi:14:1-14:28 */
	model = "Qualcomm APQ8074 Dragonboard"; /* arch/arm/boot/dts/qcom-apq8074-dragonboard.dts:7:1-7:40 */

	adsp-pil { /* arch/arm/boot/dts/qcom-msm8974.dtsi:243:1-262:3 */


The --annotate-full version is harder for _me_ to scan.

The --annotate-full version results in a much longer line (and this effect gets even
worse when there are nodes nested several levels).

One can make the argument that the long path is the fault of the way that
Linux is structured, and it should be my problem to resolve that.  And to
a certain extent I can.  I could package a simple sed command in a script.
The simplistic hard coded version of the sed command (since I know the base
path of the devicetree source file that I am compiling) is:

$ sed -e 's|arch/arm/boot/dts/||g' -e 's|:[0-9]*-[0-9]*:[0-9]*||g' -e 's| /\* <no-file>:<no-line> \*/||' qcom-apq8074-dragonboard--annotate-full.dts > qcom-apq8074-dragonboard--annotate-full--then-sed.dts

The second "-e" substitution is not needed to strip the path off, but I was
going for the full simplification.  Looking at the first few lines of that
transformation, I see that it is basically the same as using --annotate.
Looking deeper into the file, which I don't show here, some of the row
numbers are different, due to my simplistic sed regular expression.

$ head qcom-apq8074-dragonboard--annotate-full--then-sed.dts
/dts-v1/;

/ { /* qcom-apq8074-dragonboard.dts:6, qcom-msm8974.dtsi:11, skeleton.dtsi:12 */
	#address-cells = <0x1>; /* skeleton.dtsi:13 */
	#size-cells = <0x1>; /* skeleton.dtsi:14 */
	compatible = "qcom,apq8074-dragonboard", "qcom,apq8074"; /* qcom-apq8074-dragonboard.dts:8 */
	interrupt-parent = <0x1>; /* qcom-msm8974.dtsi:14 */
	model = "Qualcomm APQ8074 Dragonboard"; /* qcom-apq8074-dragonboard.dts:7 */

	adsp-pil { /* qcom-msm8974.dtsi:243 */

So I could achieve the equivalent of --annotate by post-processing, possible
with the result of slightly different line numbers if I'm sloppy.  But if
dtc can do the simplification, I don't need to add the additional external processing.

The question of whether to include just the starting line number, or the
starting line/column and ending line/column was the trade off between
more information vs less dense information being easier to scan and
read.

Having both --annotate and --annotate-full lets the user make the trade
off as needed in any particular case.

-Frank

Re: [PATCH 3/3 v2] annotations: add the annotation functionality

[Frank Rowand]

Hi Julia,

When I was creating the sample output for this reply to David,
I noticed that the --annotate-full annotation for the dtc
generated content (eg the phandle properties) are reporting
"/* <no-file>:<no-line> */".  Did you intend to remove this,
as you did for the --annotate-full case?

-Frank


On 01/16/18 20:13, Frank Rowand wrote:
> On 01/16/18 17:13, David Gibson wrote:
>> On Mon, Jan 15, 2018 at 07:24:10PM +0100, Julia Lawall wrote:
>>> This commit provides two new command-line options:
>>>
>>> --annotate (abbreviated -T)
>>> --annotate-full (abbreviated -F)
>>
>> What's the rationale for having two different versions of the
>> annotations?
> 
> I'll put an example to try to explain, at the end of this email.
> 

< snip >

Re: [PATCH 3/3 v2] annotations: add the annotation functionality

[Julia Lawall]

On Tue, 16 Jan 2018, Frank Rowand wrote:

> Hi Julia,
>
> When I was creating the sample output for this reply to David,
> I noticed that the --annotate-full annotation for the dtc
> generated content (eg the phandle properties) are reporting
> "/* <no-file>:<no-line> */".  Did you intend to remove this,
> as you did for the --annotate-full case?

From what you say, it is done backwards.  --annotate-full was intended to
say no file no line and --annotate was supposed to put nothing at all.

julia

> -Frank
>
>
> On 01/16/18 20:13, Frank Rowand wrote:
> > On 01/16/18 17:13, David Gibson wrote:
> >> On Mon, Jan 15, 2018 at 07:24:10PM +0100, Julia Lawall wrote:
> >>> This commit provides two new command-line options:
> >>>
> >>> --annotate (abbreviated -T)
> >>> --annotate-full (abbreviated -F)
> >>
> >> What's the rationale for having two different versions of the
> >> annotations?
> >
> > I'll put an example to try to explain, at the end of this email.
> >
>
> < snip >
>

Re: [PATCH 3/3 v2] annotations: add the annotation functionality

[Frank Rowand]

On 01/16/18 21:50, Julia Lawall wrote:
> 
> 
> On Tue, 16 Jan 2018, Frank Rowand wrote:
> 
>> Hi Julia,
>>
>> When I was creating the sample output for this reply to David,
>> I noticed that the --annotate-full annotation for the dtc
>> generated content (eg the phandle properties) are reporting
>> "/* <no-file>:<no-line> */".  Did you intend to remove this,
>> as you did for the --annotate-full case?
> 
>>From what you say, it is done backwards.  --annotate-full was intended to
> say no file no line and --annotate was supposed to put nothing at all.

I think it is working as intended then:

  --annotate-full says: /* <no-file>:<no-line> */

  --annotate says: nothing

Thanks,

-Frank

> 
> julia
> 
>> -Frank
>>
>>
>> On 01/16/18 20:13, Frank Rowand wrote:
>>> On 01/16/18 17:13, David Gibson wrote:
>>>> On Mon, Jan 15, 2018 at 07:24:10PM +0100, Julia Lawall wrote:
>>>>> This commit provides two new command-line options:
>>>>>
>>>>> --annotate (abbreviated -T)
>>>>> --annotate-full (abbreviated -F)
>>>>
>>>> What's the rationale for having two different versions of the
>>>> annotations?
>>>
>>> I'll put an example to try to explain, at the end of this email.
>>>
>>
>> < snip >
>>
> 

Re: [PATCH 3/3 v2] annotations: add the annotation functionality

[Julia Lawall]

On Tue, 16 Jan 2018, Frank Rowand wrote:

> On 01/16/18 21:50, Julia Lawall wrote:
> >
> >
> > On Tue, 16 Jan 2018, Frank Rowand wrote:
> >
> >> Hi Julia,
> >>
> >> When I was creating the sample output for this reply to David,
> >> I noticed that the --annotate-full annotation for the dtc
> >> generated content (eg the phandle properties) are reporting
> >> "/* <no-file>:<no-line> */".  Did you intend to remove this,
> >> as you did for the --annotate-full case?
> >
> >>From what you say, it is done backwards.  --annotate-full was intended to
> > say no file no line and --annotate was supposed to put nothing at all.
>
> I think it is working as intended then:
>
>   --annotate-full says: /* <no-file>:<no-line> */
>
>   --annotate says: nothing

Yes, that's the intent.

thanks,
julia

>
> Thanks,
>
> -Frank
>
> >
> > julia
> >
> >> -Frank
> >>
> >>
> >> On 01/16/18 20:13, Frank Rowand wrote:
> >>> On 01/16/18 17:13, David Gibson wrote:
> >>>> On Mon, Jan 15, 2018 at 07:24:10PM +0100, Julia Lawall wrote:
> >>>>> This commit provides two new command-line options:
> >>>>>
> >>>>> --annotate (abbreviated -T)
> >>>>> --annotate-full (abbreviated -F)
> >>>>
> >>>> What's the rationale for having two different versions of the
> >>>> annotations?
> >>>
> >>> I'll put an example to try to explain, at the end of this email.
> >>>
> >>
> >> < snip >
> >>
> >
>
> --
> 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
>