[RFC PATCH v6 0/2] dtc: dts source location annotation

[RFC PATCH v6 0/2] dtc: dts source location annotation

[Frank Rowand]

Proof of concept patch.

Annotates input source file and line number of nodes and properties
as comments in output .dts file when --annotate flag is supplied.


A common dts source file convention is for a system .dts file
to include default SOC and/or device .dtsi files and then add
additional system specific properties or over-ride property values
from the .dtsi files.  It can be a time consuming and error prone
exercise to determine exactly what nodes, properties, and property
values are in the final .dtb binary blob and where they originated.

Modify the dtc compiler to read a (possibly cpp pre-processed) .dts
file and for the output .dts annotate each node and property with
the corresponding source location.

As an example, one device tree node for the dragonboard in the
Linux kernel source tree (hand edited to remove leading path
components) is:

  -----  long format  -----

  sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14.3-18.5 */
        compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240.4-37 */
        reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241.4-49 */
        reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242.4-37 */
        interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243.4-38 */
        interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244.4-42 */
        clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245.4-36 */
        clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246.4-34 */
        status = "ok"; /* qcom-apq8074-dragonboard.dts:17.4-18 */
        bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15.4-20 */
        non-removable; /* qcom-apq8074-dragonboard.dts:16.4-18 */
  }; /* qcom-apq8074-dragonboard.dts:14.3-18.5 */


  ----- short format  -----

  sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14 */
        compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240 */
        reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241 */
        reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242 */
        interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243 */
        interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244 */
        clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245 */
        clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246 */
        status = "ok"; /* qcom-apq8074-dragonboard.dts:17 */
        bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15 */
        non-removable; /* qcom-apq8074-dragonboard.dts:16 */
  }; /* qcom-apq8074-dragonboard.dts:18 */


qcom-apq8074-dragonboard.dts:
   - last referenced the sdhci node
   - changed the value of the "status" property from "disabled" to "ok"
   - added two properties, "bus-width" and "non-removable"

qcom-msm8974.dtsi:
   - initially set the value the "status" property to "disabled"
     (not visible in the annotated .dts)
   - provided all of the other property values


When the dtc compiler is run within the Linux kernel build system,
the path of the source files will be the full absolute path, just
as seen for gcc warnings and errors.  I always trim away the path
leading up to the Linux kernel source tree by passing the kernel
build output through a sed pipe.  I have done the same to the
above example to remove the excessive verbosity in the source paths.

Implementation notes:

  - Added new function srcpos_string_short() which is similar to
    srcpos_string() but limits the location information to one
    line number.  The fuller output of srcpos_string() adds noise
    to the annotation which (in my opinion) is not especially
    useful in this specific context.

    The one downside to this choice is that the column numbers for
    multiple properties on the same input line will not be reported.
    This is unlikely to be an issue unless a .dts contains all of
    the properties on a single line (which might be the case for
    a machine generated .dts).  I do not think that the extra
    noise for the common case justifies handling this case.

  - The source location of each node and property is saved in the
    respective node or property during the parse phase because
    the source location information from current_srcfile is no longer
    available when the final values are written out from dt_to_source()
    and the functions that it calls.

  - A check is added to dtc.c to ensure that input and output format
    are both device tree source.  An alternate choice would be to
    turn off the --annotate flag if either the input file or the
    output file is not device tree source.  In the alternate case,
    the disabling of --annotate could be silent or a warning could
    be issued.


TODO:
  - Update "make check" tests to reflect review comments.
  - Test against a wider set of .dts files.  There are some rules
    that I did not test extensively (and some rules, such as delete
    that I did not test at all in this version).
  - Change the --annotate option to choose either long or short format.
  - Fix location for:  devicetree: '/' nodedef


Changes from v5:
  - Add more pointer checking to patch 1.
  - Change from two to one srcpos fields in struct node.  Move the
    logic to choose begin or end to the annotation print logic for
    the case of short location format.
  - Move the setting of srcpos back to name_node() and build_prop()
    instead of adding new functions to save that information.
  - Patch 2 reports the location in long format (beginning and end
    of the object).  Adding patch 3 changes for format to short (the
    current source line only).

[RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string()

[Frank Rowand]

From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

Check for NULL pos before dereferencing it in srcpos_string().

Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 srcpos.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/srcpos.c
===================================================================
--- a/srcpos.c
+++ b/srcpos.c
@@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
 	char *pos_str;
 	int rc;
 
-	if (pos)
+	if (pos && pos->file)
 		fname = pos->file->name;
 
 
-	if (pos->first_line != pos->last_line)
+	if (!pos)
+		rc = asprintf(&pos_str, "%s:<no-line>", fname);
+	else if (pos->first_line != pos->last_line)
 		rc = asprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
 			      pos->first_line, pos->first_column,
 			      pos->last_line, pos->last_column);

[RFC PATCH v6 2/3] dtc: dts source location annotation

[Frank Rowand]

From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

Proof of concept patch.

Annotates input source file and line number of nodes and properties
as comments in output .dts file when --annotate flag is supplied.


A common dts source file convention is for a system .dts file
to include default SOC and/or device .dtsi files and then add
additional system specific properties or over-ride property values
from the .dtsi files.  It can be a time consuming and error prone
exercise to determine exactly what nodes, properties, and property
values are in the final .dtb binary blob and where they originated.

Modify the dtc compiler to read a (possibly cpp pre-processed) .dts
file and for the output .dts annotate each node and property with
the corresponding source location.

As an example, one device tree node for the dragonboard in the
Linux kernel source tree is: 

  -----  long format  -----

  sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14.3-18.5 */
        compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240.4-37 */
        reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241.4-49 */
        reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242.4-37 */
        interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243.4-38 */
        interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244.4-42 */
        clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245.4-36 */
        clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246.4-34 */
        status = "ok"; /* qcom-apq8074-dragonboard.dts:17.4-18 */
        bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15.4-20 */
        non-removable; /* qcom-apq8074-dragonboard.dts:16.4-18 */
  }; /* qcom-apq8074-dragonboard.dts:14.3-18.5 */


  -----  short format (requires patch 3)  -----

  sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14 */
        compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240 */
        reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241 */
        reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242 */
        interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243 */
        interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244 */
        clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245 */
        clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246 */
        status = "ok"; /* qcom-apq8074-dragonboard.dts:17 */
        bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15 */
        non-removable; /* qcom-apq8074-dragonboard.dts:16 */
  }; /* qcom-apq8074-dragonboard.dts:18 */


qcom-apq8074-dragonboard.dts:
   - last referenced the sdhci node
   - changed the value of the "status" property from "disabled" to "ok"
   - added two properties, "bus-width" and "non-removable"

qcom-msm8974.dtsi:
   - initially set the value the "status" property to "disabled"
     (not visible in the annotated .dts)
   - provided all of the other property values


When the dtc compiler is run within the Linux kernel build system,
the path of the source files will be the full absolute path, just
as seen for gcc warnings and errors.  I always trim away the path
leading up to the Linux kernel source tree by passing the kernel
build output through a sed pipe.  I have done the same to the
above example to remove the excessive verbosity in the source paths.

Implementation notes:

  - The source location of each node and property is saved in the
    respective node or property during the parse phase because
    the source location information no longer available when the
    final values are written out from dt_to_source() and the
    functions that it calls.

  - A check is added to dtc.c to ensure that input and output format
    are both device tree source.  An alternate choice would be to
    turn off the --annotate flag if either the input file or the
    output file is not device tree source.  In the alternate case,
    the disabling of --annotate could be silent or a warning could
    be issued.

TODO:

  - More testing.

Not-signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 dtc-parser.y |   16 ++++++++--------
 dtc.c        |    9 +++++++++
 dtc.h        |    9 ++++++---
 flattree.c   |    2 +-
 fstree.c     |    7 ++++---
 livetree.c   |   20 ++++++++++++++------
 srcpos.c     |   35 +++++++++++++++++++++++++++++++++++
 srcpos.h     |    2 ++
 treesource.c |   38 +++++++++++++++++++++++++++++++++-----
 9 files changed, 112 insertions(+), 26 deletions(-)

Index: b/dtc.h
===================================================================
--- a/dtc.h
+++ b/dtc.h
@@ -54,6 +54,7 @@ extern int reservenum;		/* Number of mem
 extern int minsize;		/* Minimum blob size */
 extern int padsize;		/* Additional padding to blob */
 extern int phandle_format;	/* Use linux,phandle or phandle properties */
+extern bool annotate;		/* annotate .dts with input source location */
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
@@ -140,6 +141,7 @@ struct property {
 	struct property *next;
 
 	struct label *labels;
+	struct srcpos *srcpos;
 };
 
 struct node {
@@ -158,6 +160,7 @@ struct node {
 	int addr_cells, size_cells;
 
 	struct label *labels;
+	struct srcpos *srcpos;
 };
 
 #define for_each_label_withdel(l0, l) \
@@ -184,16 +187,16 @@ 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 *merge_nodes(struct node *old_node, struct node *new_node, struct srcpos *srcpos);
 
 void add_property(struct node *node, struct property *prop);
 void delete_property_by_name(struct node *node, char *name);
Index: b/livetree.c
===================================================================
--- a/livetree.c
+++ b/livetree.c
@@ -19,6 +19,7 @@
  */
 
 #include "dtc.h"
+#include "srcpos.h"
 
 /*
  * Tree building functions
@@ -50,7 +51,7 @@ 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 +59,7 @@ struct property *build_property(char *na
 
 	new->name = name;
 	new->val = val;
+	new->srcpos = srcpos_copy_all(srcpos);
 
 	return new;
 }
@@ -125,16 +127,17 @@ 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 +172,7 @@ struct node *merge_nodes(struct node *ol
 
 				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 +202,7 @@ struct node *merge_nodes(struct node *ol
 		/* 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 +213,8 @@ struct node *merge_nodes(struct node *ol
 			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);
@@ -535,13 +541,15 @@ cell_t get_node_phandle(struct node *roo
 	    && (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
Index: b/treesource.c
===================================================================
--- a/treesource.c
+++ b/treesource.c
@@ -200,9 +200,16 @@ static void write_propval(FILE *f, struc
 	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, struc
 		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
 	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
 		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");
+	}
 }
 
 
Index: b/dtc.c
===================================================================
--- a/dtc.c
+++ b/dtc.c
@@ -29,6 +29,7 @@ int reservenum;		/* Number of memory res
 int minsize;		/* Minimum blob size */
 int padsize;		/* Additional padding to blob */
 int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
+bool annotate = false;	/* annotate .dts with input source location */
 
 static void fill_fullpaths(struct node *tree, const char *prefix)
 {
@@ -71,6 +72,7 @@ static struct option const usage_long_op
 	{"error",             a_argument, NULL, 'E'},
 	{"help",             no_argument, NULL, 'h'},
 	{"version",          no_argument, NULL, 'v'},
+	{"annotate",         no_argument, NULL, 'A'},
 	{NULL,               no_argument, NULL, 0x0},
 };
 static const char * const usage_opts_help[] = {
@@ -101,6 +103,7 @@ static const char * const usage_opts_hel
 	"\n\tEnable/disable errors (prefix with \"no-\")",
 	"\n\tPrint this help and exit",
 	"\n\tPrint version and exit",
+	"\n\tAnnotate output .dts with input source file and line",
 	NULL,
 };
 
@@ -125,6 +128,9 @@ int main(int argc, char *argv[])
 
 	while ((opt = util_getopt_long()) != EOF) {
 		switch (opt) {
+		case 'A':
+			annotate = true;
+			break;
 		case 'I':
 			inform = optarg;
 			break;
@@ -213,6 +219,9 @@ int main(int argc, char *argv[])
 		fprintf(depfile, "%s:", outname);
 	}
 
+	if (annotate && (!streq(inform, "dts") || !streq(outform, "dts")))
+		die("--annotate requires -I dts -O dts\n");
+
 	if (streq(inform, "dts"))
 		bi = dt_from_source(arg);
 	else if (streq(inform, "fs"))
Index: b/dtc-parser.y
===================================================================
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -134,11 +134,11 @@ memreserve:
 devicetree:
 	  '/' nodedef
 		{
-			$$ = name_node($2, "");
+			$$ = name_node($2, "", &@1);
 		}
 	| devicetree '/' nodedef
 		{
-			$$ = merge_nodes($1, $3);
+			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
 		}
 
 	| devicetree DT_LABEL DT_REF nodedef
@@ -147,7 +147,7 @@ devicetree:
 
 			add_label(&target->labels, $2);
 			if (target)
-				merge_nodes(target, $4);
+				merge_nodes(target, $4, &@4);
 			else
 				ERROR(&@3, "Label or path %s not found", $3);
 			$$ = $1;
@@ -157,7 +157,7 @@ devicetree:
 			struct node *target = get_node_by_ref($1, $2);
 
 			if (target)
-				merge_nodes(target, $3);
+				merge_nodes(target, $3, &@3);
 			else
 				ERROR(&@2, "Label or path %s not found", $2);
 			$$ = $1;
@@ -197,11 +197,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 ';'
 		{
@@ -456,11 +456,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
 		{
Index: b/srcpos.c
===================================================================
--- 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;
+}
+
 
 
 void
Index: b/srcpos.h
===================================================================
--- a/srcpos.h
+++ b/srcpos.h
@@ -104,6 +104,8 @@ extern struct srcpos srcpos_empty;
 
 extern void srcpos_update(struct srcpos *pos, const char *text, int len);
 extern struct srcpos *srcpos_copy(struct srcpos *pos);
+extern struct srcpos *srcpos_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 srcpos_dump(struct srcpos *pos);
 
Index: b/flattree.c
===================================================================
--- a/flattree.c
+++ b/flattree.c
@@ -692,7 +692,7 @@ static struct property *flat_read_proper
 
 	val = flat_read_data(dtbuf, proplen);
 
-	return build_property(name, val);
+	return build_property(name, val, NULL);
 }
 
 
Index: b/fstree.c
===================================================================
--- a/fstree.c
+++ b/fstree.c
@@ -60,7 +60,8 @@ static struct node *read_fstree(const ch
 			} 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,7 @@ static struct node *read_fstree(const ch
 			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 +85,7 @@ struct boot_info *dt_from_fs(const char
 	struct node *tree;
 
 	tree = read_fstree(dirname);
-	tree = name_node(tree, "");
+	tree = name_node(tree, "", NULL);
 
 	return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
 }

Re: [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string()

[David Gibson]

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

On Fri, Oct 02, 2015 at 09:49:08PM -0700, Frank Rowand wrote:
> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> 
> Check for NULL pos before dereferencing it in srcpos_string().
> 
> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> ---
>  srcpos.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: b/srcpos.c
> ===================================================================
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
>  	char *pos_str;
>  	int rc;
>  
> -	if (pos)
> +	if (pos && pos->file)
>  		fname = pos->file->name;
>  
>  
> -	if (pos->first_line != pos->last_line)
> +	if (!pos)
> +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
> +	else if (pos->first_line != pos->last_line)

This logic still seems backwards to me.  I'd really prefer the !pos
check to go first, then !pos->file, then the normal case.

>  		rc = asprintf(&pos_str, "%s:%d.%d-%d.%d", fname,
>  			      pos->first_line, pos->first_column,
>  			      pos->last_line, pos->last_column);

-- 
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: 819 bytes --]

Re: [RFC PATCH v6 2/3] dtc: dts source location annotation

[David Gibson]

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

On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> 
> Proof of concept patch.
> 
> Annotates input source file and line number of nodes and properties
> as comments in output .dts file when --annotate flag is supplied.
> 
> 
> A common dts source file convention is for a system .dts file
> to include default SOC and/or device .dtsi files and then add
> additional system specific properties or over-ride property values
> from the .dtsi files.  It can be a time consuming and error prone
> exercise to determine exactly what nodes, properties, and property
> values are in the final .dtb binary blob and where they originated.
> 
> Modify the dtc compiler to read a (possibly cpp pre-processed) .dts
> file and for the output .dts annotate each node and property with
> the corresponding source location.
> 
> As an example, one device tree node for the dragonboard in the
> Linux kernel source tree is: 
> 
>   -----  long format  -----
> 
>   sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14.3-18.5 */
>         compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240.4-37 */
>         reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241.4-49 */
>         reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242.4-37 */
>         interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243.4-38 */
>         interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244.4-42 */
>         clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245.4-36 */
>         clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246.4-34 */
>         status = "ok"; /* qcom-apq8074-dragonboard.dts:17.4-18 */
>         bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15.4-20 */
>         non-removable; /* qcom-apq8074-dragonboard.dts:16.4-18 */
>   }; /* qcom-apq8074-dragonboard.dts:14.3-18.5 */
> 
> 
>   -----  short format (requires patch 3)  -----
> 
>   sdhci@f9824900 { /* qcom-apq8074-dragonboard.dts:14 */
>         compatible = "qcom,sdhci-msm-v4"; /* qcom-msm8974.dtsi:240 */
>         reg = <0xf9824900 0x11c 0xf9824000 0x800>; /* qcom-msm8974.dtsi:241 */
>         reg-names = "hc_mem", "core_mem"; /* qcom-msm8974.dtsi:242 */
>         interrupts = <0x0 0x7b 0x0 0x0 0x8a 0x0>; /* qcom-msm8974.dtsi:243 */
>         interrupt-names = "hc_irq", "pwr_irq"; /* qcom-msm8974.dtsi:244 */
>         clocks = <0xd 0xd8 0xd 0xd7>; /* qcom-msm8974.dtsi:245 */
>         clock-names = "core", "iface"; /* qcom-msm8974.dtsi:246 */
>         status = "ok"; /* qcom-apq8074-dragonboard.dts:17 */
>         bus-width = <0x8>; /* qcom-apq8074-dragonboard.dts:15 */
>         non-removable; /* qcom-apq8074-dragonboard.dts:16 */
>   }; /* qcom-apq8074-dragonboard.dts:18 */
> 
> 
> qcom-apq8074-dragonboard.dts:
>    - last referenced the sdhci node
>    - changed the value of the "status" property from "disabled" to "ok"
>    - added two properties, "bus-width" and "non-removable"
> 
> qcom-msm8974.dtsi:
>    - initially set the value the "status" property to "disabled"
>      (not visible in the annotated .dts)
>    - provided all of the other property values
> 
> 
> When the dtc compiler is run within the Linux kernel build system,
> the path of the source files will be the full absolute path, just
> as seen for gcc warnings and errors.  I always trim away the path
> leading up to the Linux kernel source tree by passing the kernel
> build output through a sed pipe.  I have done the same to the
> above example to remove the excessive verbosity in the source paths.
> 
> Implementation notes:
> 
>   - The source location of each node and property is saved in the
>     respective node or property during the parse phase because
>     the source location information no longer available when the
>     final values are written out from dt_to_source() and the
>     functions that it calls.
> 
>   - A check is added to dtc.c to ensure that input and output format
>     are both device tree source.  An alternate choice would be to
>     turn off the --annotate flag if either the input file or the
>     output file is not device tree source.  In the alternate case,
>     the disabling of --annotate could be silent or a warning could
>     be issued.
> 
> TODO:
> 
>   - More testing.
> 
> Not-signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> ---
>  dtc-parser.y |   16 ++++++++--------
>  dtc.c        |    9 +++++++++
>  dtc.h        |    9 ++++++---
>  flattree.c   |    2 +-
>  fstree.c     |    7 ++++---
>  livetree.c   |   20 ++++++++++++++------
>  srcpos.c     |   35 +++++++++++++++++++++++++++++++++++
>  srcpos.h     |    2 ++
>  treesource.c |   38 +++++++++++++++++++++++++++++++++-----
>  9 files changed, 112 insertions(+), 26 deletions(-)
> 
> Index: b/dtc.h
> ===================================================================
[snip]
> Index: b/dtc-parser.y
> ===================================================================
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -134,11 +134,11 @@ memreserve:
>  devicetree:
>  	  '/' nodedef
>  		{
> -			$$ = name_node($2, "");
> +			$$ = name_node($2, "", &@1);
>  		}
>  	| devicetree '/' nodedef
>  		{
> -			$$ = merge_nodes($1, $3);
> +			$$ = merge_nodes($1, $3, srcpos_combine(&@2, &@3));
>  		}

The two branches here aren't quite consistent - the first doesn't
include the '/', the second does.  You could either change the second
to just &@3, or use &@$ for the first.

>  
>  	| devicetree DT_LABEL DT_REF nodedef
> @@ -147,7 +147,7 @@ devicetree:
>  
>  			add_label(&target->labels, $2);
>  			if (target)
> -				merge_nodes(target, $4);
> +				merge_nodes(target, $4, &@4);

This one doesn't include the name/label again

>  			else
>  				ERROR(&@3, "Label or path %s not found", $3);
>  			$$ = $1;
> @@ -157,7 +157,7 @@ devicetree:
>  			struct node *target = get_node_by_ref($1, $2);
>  
>  			if (target)
> -				merge_nodes(target, $3);
> +				merge_nodes(target, $3, &@3);
>  			else
>  				ERROR(&@2, "Label or path %s not found", $2);
>  			$$ = $1;
> @@ -197,11 +197,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 ';'
>  		{
> @@ -456,11 +456,11 @@ subnodes:
>  subnode:
>  	  DT_PROPNODENAME nodedef
>  		{
> -			$$ = name_node($2, $1);
> +			$$ = name_node($2, $1, &@$);

.. and this one does.

Looking at all of these it's probably going to be simplest not to
include the label/name (i.e. just use the srcpos from the nodedef).

>  		}
>  	| DT_DEL_NODE DT_PROPNODENAME ';'
>  		{
> -			$$ = name_node(build_node_delete(), $2);
> +			$$ = name_node(build_node_delete(), $2, &@$);
>  		}
>  	| DT_LABEL subnode
>  		{
> Index: b/srcpos.c
> ===================================================================
> --- 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;
> +}

You still don't need this function.  srcfile_states have unlimited
lifetime.

> +
> +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;
> +}
> +
>  
>  
>  void
> Index: b/srcpos.h
> ===================================================================
> --- a/srcpos.h
> +++ b/srcpos.h
> @@ -104,6 +104,8 @@ extern struct srcpos srcpos_empty;
>  
>  extern void srcpos_update(struct srcpos *pos, const char *text, int len);
>  extern struct srcpos *srcpos_copy(struct srcpos *pos);
> +extern struct srcpos *srcpos_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 srcpos_dump(struct srcpos *pos);
>  
> Index: b/flattree.c
> ===================================================================
> --- a/flattree.c
> +++ b/flattree.c
> @@ -692,7 +692,7 @@ static struct property *flat_read_proper
>  
>  	val = flat_read_data(dtbuf, proplen);
>  
> -	return build_property(name, val);
> +	return build_property(name, val, NULL);
>  }
>  
>  
> Index: b/fstree.c
> ===================================================================
> --- a/fstree.c
> +++ b/fstree.c
> @@ -60,7 +60,8 @@ static struct node *read_fstree(const ch
>  			} 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,7 @@ static struct node *read_fstree(const ch
>  			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 +85,7 @@ struct boot_info *dt_from_fs(const char
>  	struct node *tree;
>  
>  	tree = read_fstree(dirname);
> -	tree = name_node(tree, "");
> +	tree = name_node(tree, "", NULL);
>  
>  	return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
>  }

-- 
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: 819 bytes --]

Re: [RFC PATCH v6 3/3] dtc: dts source location annotation, short location format

[David Gibson]

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

On Fri, Oct 02, 2015 at 09:55:17PM -0700, Frank Rowand wrote:

Uh.. the actual patch seems to be missing.

-- 
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: 819 bytes --]

Re: [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string()

[Frank Rowand]

On 10/5/2015 9:10 PM, David Gibson wrote:
> On Fri, Oct 02, 2015 at 09:49:08PM -0700, Frank Rowand wrote:
>> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>>
>> Check for NULL pos before dereferencing it in srcpos_string().
>>
>> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>> ---
>>  srcpos.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Index: b/srcpos.c
>> ===================================================================
>> --- a/srcpos.c
>> +++ b/srcpos.c
>> @@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
>>  	char *pos_str;
>>  	int rc;
>>  
>> -	if (pos)
>> +	if (pos && pos->file)
>>  		fname = pos->file->name;
>>  
>>  
>> -	if (pos->first_line != pos->last_line)
>> +	if (!pos)
>> +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
>> +	else if (pos->first_line != pos->last_line)
> 
> This logic still seems backwards to me.  I'd really prefer the !pos
> check to go first, then !pos->file, then the normal case.
> 

Checking !pos first results in either an early return, a goto,
or more deeply nesting the

   if (pos->first_line != pos->last_line)
      asprintf(...)
   else if (...)
      asprintf(...)
   else
      rc = asprintf(...)

all of which seemed worse.  In the next version I'll change it to:

char *
srcpos_string(struct srcpos *pos)
{
        const char *fname = "<no-file>";
        char *pos_str;
        int rc;

        if (!pos) {
                rc = asprintf(&pos_str, "%s:<no-line>", fname);
                goto out;
        } else if (pos->file) {
                fname = pos->file->name;
        }

        if (pos->first_line != pos->last_line)
                rc = asprintf(&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)
                rc = asprintf(&pos_str, "%s:%d.%d-%d", fname,
                              pos->first_line, pos->first_column,
                              pos->last_column);
        else
                rc = asprintf(&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: [RFC PATCH v6 3/3] dtc: dts source location annotation, short location format

[Frank Rowand]

On 10/5/2015 10:01 PM, David Gibson wrote:
> On Fri, Oct 02, 2015 at 09:55:17PM -0700, Frank Rowand wrote:
> 
> Uh.. the actual patch seems to be missing.
> 

Ah, the fun of broken email tools.  I'll be changing my email tools
in the near future.

This version of the patch is intended to show what is involved
in creating the short input, given the changes to the other
patches.  If it looks reasonable, I will add the modifications
to --annotate to allow a choice of either short or long format.

This version also has a totally awkward:

   srcpost_string_short()
   {
     if (pos)
        ...

     if (!pos)
        ...
     else
        ...

which I will fix in the next version.

-Frank




From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

Show only the current line number in each source annotation instead
of the full range of the object.

TODO:
  - modify "--annotate" command line option to choose short or long annotation


Not-signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 srcpos.c     |   34 ++++++++++++++++++++++++++++++++++
 srcpos.h     |    4 ++++
 treesource.c |    8 ++++----
 3 files changed, 42 insertions(+), 4 deletions(-)

Index: b/srcpos.c
===================================================================
--- a/srcpos.c
+++ b/srcpos.c
@@ -327,6 +327,40 @@ srcpos_string(struct srcpos *pos)
 	return pos_str;
 }
 
+char *
+srcpos_string_short(struct srcpos *pos, int first_line)
+{
+	const char *fname = "<no-file>";
+	char *pos_str;
+	int rc;
+
+	if (pos)
+		fname = pos->file->name;
+
+	if (!pos)
+		rc = asprintf(&pos_str, "%s:<no-line>", fname);
+	else
+		rc = asprintf(&pos_str, "%s:%d", fname,
+			      (first_line) ? pos->first_line: pos->last_line);
+
+	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, 1);
+}
+
+char *
+srcpos_string_last(struct srcpos *pos)
+{
+	return srcpos_string_short(pos, 0);
+}
+
 void srcpos_verror(struct srcpos *pos, const char *prefix,
 		   const char *fmt, va_list va)
 {
Index: b/treesource.c
===================================================================
--- a/treesource.c
+++ b/treesource.c
@@ -204,7 +204,7 @@ static void write_propval(FILE *f, struc
 
 	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, struc
 	}
 
 	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
 	else
 		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
 	}
 	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 {
Index: b/srcpos.h
===================================================================
--- a/srcpos.h
+++ b/srcpos.h
@@ -107,6 +107,10 @@ extern struct srcpos *srcpos_copy(struct
 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_short(struct srcpos *pos, int line);
+extern char *srcpos_string_first(struct srcpos *pos);
+extern char *srcpos_string_last(struct srcpos *pos);
+
 extern void srcpos_dump(struct srcpos *pos);
 
 extern void srcpos_verror(struct srcpos *pos, const char *prefix,

Re: [RFC PATCH v6 2/3] dtc: dts source location annotation

[Frank Rowand]

On 10/5/2015 9:56 PM, David Gibson wrote:
> On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
>> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>>
>> Proof of concept patch.
>>
>> Annotates input source file and line number of nodes and properties
>> as comments in output .dts file when --annotate flag is supplied.

< snip >

> The two branches here aren't quite consistent - the first doesn't
> include the '/', the second does.  You could either change the second
> to just &@3, or use &@$ for the first.

< snip >

> This one doesn't include the name/label again

< snip>

> 
> .. and this one does.
> 
> Looking at all of these it's probably going to be simplest not to
> include the label/name (i.e. just use the srcpos from the nodedef).

This also shows how much I need to create a better "make check" test
suite to exercise all of the relevant paths (as is on my todo list).

I've been trying to capture the line that the name is on, since it
might be different than the line that the "{" is on, but maybe I'm
just trying too hard and should accept the simpler solution.  The
result will be obvious to the person using the output.

I'll fix this all up after I get back from the conference.

Thanks for checking the patch over at that level of detail!

-frank

Re: [RFC PATCH v6 2/3] dtc: dts source location annotation

[Frank Rowand]

On 10/5/2015 9:56 PM, David Gibson wrote:
> On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
>> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>>
>> Proof of concept patch.
>>
>> Annotates input source file and line number of nodes and properties
>> as comments in output .dts file when --annotate flag is supplied.

< snip >


>> Index: b/srcpos.c
>> ===================================================================
>> --- 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;
>> +}
> 
> You still don't need this function.  srcfile_states have unlimited
> lifetime.

My observation about this is buried in a late reply to v5, so
copying here:

   If I don't allocate a new copy of pos->file, then the file names are
   incorrect.  I'm not sure why.  I can dig into this deeper to try to
   understand what is going on if you want me to.

It sounds like I do need to debug this.

-Frank

Re: [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string()

[David Gibson]

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

On Tue, Oct 06, 2015 at 12:32:20AM -0700, Frank Rowand wrote:
> On 10/5/2015 9:10 PM, David Gibson wrote:
> > On Fri, Oct 02, 2015 at 09:49:08PM -0700, Frank Rowand wrote:
> >> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> >>
> >> Check for NULL pos before dereferencing it in srcpos_string().
> >>
> >> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> >> ---
> >>  srcpos.c |    6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> Index: b/srcpos.c
> >> ===================================================================
> >> --- a/srcpos.c
> >> +++ b/srcpos.c
> >> @@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
> >>  	char *pos_str;
> >>  	int rc;
> >>  
> >> -	if (pos)
> >> +	if (pos && pos->file)
> >>  		fname = pos->file->name;
> >>  
> >>  
> >> -	if (pos->first_line != pos->last_line)
> >> +	if (!pos)
> >> +		rc = asprintf(&pos_str, "%s:<no-line>", fname);
> >> +	else if (pos->first_line != pos->last_line)
> > 
> > This logic still seems backwards to me.  I'd really prefer the !pos
> > check to go first, then !pos->file, then the normal case.
> > 
> 
> Checking !pos first results in either an early return, a goto,
> or more deeply nesting the

Early return is fine by me.

-- 
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: 819 bytes --]

Re: [RFC PATCH v6 2/3] dtc: dts source location annotation

[David Gibson]

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

On Tue, Oct 06, 2015 at 12:38:19AM -0700, Frank Rowand wrote:
> On 10/5/2015 9:56 PM, David Gibson wrote:
> > On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
> >> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> >>
> >> Proof of concept patch.
> >>
> >> Annotates input source file and line number of nodes and properties
> >> as comments in output .dts file when --annotate flag is supplied.
> 
> < snip >
> 
> > The two branches here aren't quite consistent - the first doesn't
> > include the '/', the second does.  You could either change the second
> > to just &@3, or use &@$ for the first.
> 
> < snip >
> 
> > This one doesn't include the name/label again
> 
> < snip>
> 
> > 
> > .. and this one does.
> > 
> > Looking at all of these it's probably going to be simplest not to
> > include the label/name (i.e. just use the srcpos from the nodedef).
> 
> This also shows how much I need to create a better "make check" test
> suite to exercise all of the relevant paths (as is on my todo list).
> 
> I've been trying to capture the line that the name is on, since it
> might be different than the line that the "{" is on, but maybe I'm
> just trying too hard and should accept the simpler solution.  The
> result will be obvious to the person using the output.

Hm, yes I see your point, it probably is slightly better to include
the name.  I'm fine with that, as long as it's done consistently.

> I'll fix this all up after I get back from the conference.
> 
> Thanks for checking the patch over at that level of detail!
> 
> -frank

-- 
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: 819 bytes --]

Re: [RFC PATCH v6 2/3] dtc: dts source location annotation

[David Gibson]

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

On Tue, Oct 06, 2015 at 12:45:34AM -0700, Frank Rowand wrote:
> On 10/5/2015 9:56 PM, David Gibson wrote:
> > On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
> >> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
> >>
> >> Proof of concept patch.
> >>
> >> Annotates input source file and line number of nodes and properties
> >> as comments in output .dts file when --annotate flag is supplied.
> 
> < snip >
> 
> 
> >> Index: b/srcpos.c
> >> ===================================================================
> >> --- 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;
> >> +}
> > 
> > You still don't need this function.  srcfile_states have unlimited
> > lifetime.
> 
> My observation about this is buried in a late reply to v5, so
> copying here:
> 
>    If I don't allocate a new copy of pos->file, then the file names are
>    incorrect.  I'm not sure why.  I can dig into this deeper to try to
>    understand what is going on if you want me to.
> 
> It sounds like I do need to debug this.

That's weird.  Yeah, we need to debug this.

Btw, I was thinking about the filenames - in particular the way the
current draft will give you very unwield full paths.  I was thinking
about a different approach which should shorten those without
requiring different invocation or hand hacking:
   * For the "base" dts file (the one passed on the command line),
     always use just the basename (i.e. final piece of the file path)
   * For all other files, print the relative path from the directory
     of the base file

/include/ directives already open files relative to the directory of
the file including the /include/ so we have some of the mechanisms for
this already.

I think this will require a moderate amount of extra work, so I
wouldn't suggest it for the first cut, but does it sound like a good
approach in principle to you?

-- 
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: 819 bytes --]

Re: [RFC PATCH v6 2/3] dtc: dts source location annotation

[Frank Rowand]

On 10/6/2015 10:32 PM, David Gibson wrote:
> On Tue, Oct 06, 2015 at 12:45:34AM -0700, Frank Rowand wrote:
>> On 10/5/2015 9:56 PM, David Gibson wrote:
>>> On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
>>>> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>>>>
>>>> Proof of concept patch.
>>>>
>>>> Annotates input source file and line number of nodes and properties
>>>> as comments in output .dts file when --annotate flag is supplied.
>>
>> < snip >
>>
>>
>>>> Index: b/srcpos.c
>>>> ===================================================================
>>>> --- 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;
>>>> +}
>>>
>>> You still don't need this function.  srcfile_states have unlimited
>>> lifetime.
>>
>> My observation about this is buried in a late reply to v5, so
>> copying here:
>>
>>    If I don't allocate a new copy of pos->file, then the file names are
>>    incorrect.  I'm not sure why.  I can dig into this deeper to try to
>>    understand what is going on if you want me to.
>>
>> It sounds like I do need to debug this.
> 
> That's weird.  Yeah, we need to debug this.

OK, I will dig into this.


> Btw, I was thinking about the filenames - in particular the way the
> current draft will give you very unwield full paths.  I was thinking
> about a different approach which should shorten those without
> requiring different invocation or hand hacking:
>    * For the "base" dts file (the one passed on the command line),
>      always use just the basename (i.e. final piece of the file path)
>    * For all other files, print the relative path from the directory
>      of the base file
> 
> /include/ directives already open files relative to the directory of
> the file including the /include/ so we have some of the mechanisms for
> this already.
> 
> I think this will require a moderate amount of extra work, so I
> wouldn't suggest it for the first cut, but does it sound like a good
> approach in principle to you?

Yes, I think that is a great way to approach it.  I'll take a look at
how much work this is, and if small enough I will add another patch
to the end of the series to do this.

-Frank