dtc: dts source location annotation

From: Frank Rowand <frank.rowand-/[email protected]>

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

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>

---
 srcpos.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/srcpos.c
===================================================================

srcpos.c +0 -0

@@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
 	char *pos_str;
 	int rc;

srcpos.c +0 -0

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

From: Frank Rowand <frank.rowand-/[email protected]>

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-/[email protected]>

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

Index: b/dtc-parser.y
===================================================================

dtc-parser.y +0 -0

@@ -134,11 +134,11 @@ memreserve:
 devicetree:

dtc-parser.y +3 -3

 			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 ';'
 		{

dtc-parser.y +1 -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
===================================================================

srcpos.c +11 -0

@@ -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);
+

srcpos.c +14 -0

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

srcpos.h +2 -0

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

flattree.c +1 -1

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

fstree.c +4 -3

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

Quoted 22 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

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

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>

---
 srcpos.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/srcpos.c
===================================================================

srcpos.c +1 -1

@@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
 	char *pos_str;
 	int rc;
 
-	if (pos)
+	if (pos && pos->file)
 		fname = pos->file->name;
 
 

srcpos.c srcpos_string(struct srcpos *pos) · line 277

Quoted 4 lines

srcpos.c +3 -1

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

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

Uh.. the actual patch seems to be missing.

Quoted 6 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

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.

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.

This one doesn't include the name/label again

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

Quoted 6 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

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.

Quoted 24 lines

Index: b/srcpos.c
===================================================================

srcpos.c +16 -0

@@ -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));
+

srcpos.c srcpos_copy(struct srcpos *pos) · line 269

Quoted 5 lines

srcpos.c +5 -0

+		pos_new->file = srcfile_state;
+	}
+
+	return pos_new;
+}

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

Quoted 23 lines · David Gibson

Quoted 22 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

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

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>

---
 srcpos.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/srcpos.c
===================================================================

srcpos.c +1 -1

@@ -268,11 +268,13 @@ srcpos_string(struct srcpos *pos)
 	char *pos_str;
 	int rc;
 
-	if (pos)
+	if (pos && pos->file)
 		fname = pos->file->name;
 
 

Quoted 8 lines

srcpos.c srcpos_string(struct srcpos *pos) · line 277

Quoted 4 lines

srcpos.c +3 -1

-	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

Quoted 7 lines · David Gibson

Quoted 6 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

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 >

Quoted 3 lines

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 >

Quoted 1 line

This one doesn't include the name/label again

< snip>

Quoted 5 lines

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

Quoted 7 lines · David Gibson

Quoted 6 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

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 >

Quoted 24 lines

Quoted 24 lines

Index: b/srcpos.c
===================================================================

srcpos.c +16 -0

@@ -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));
+

Quoted 8 lines

srcpos.c srcpos_copy(struct srcpos *pos) · line 269

Quoted 5 lines

srcpos.c +5 -0

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

Quoted 48 lines · Frank Rowand

Quoted 7 lines · David Gibson

Quoted 6 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

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 >

Quoted 24 lines

Quoted 24 lines

Index: b/srcpos.c
===================================================================

srcpos.c +16 -0

@@ -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));
+

Quoted 8 lines

srcpos.c srcpos_copy(struct srcpos *pos) · line 269

Quoted 5 lines

srcpos.c +5 -0

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

Quoted 5 lines

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?