* [RFC PATCH v6 0/2] dtc: dts source location annotation @ 2015-10-03 4:44 Frank Rowand [not found] ` <560F5D15.9060606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [not found] ` <560F5FB5.3020602@gmail.com> 0 siblings, 2 replies; 14+ messages in thread From: Frank Rowand @ 2015-10-03 4:44 UTC (permalink / raw) To: david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+, jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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). ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <560F5D15.9060606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string() [not found] ` <560F5D15.9060606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-03 4:49 ` Frank Rowand [not found] ` <560F5E44.9080006-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-03 4:52 ` [RFC PATCH v6 2/3] dtc: dts source location annotation Frank Rowand 1 sibling, 1 reply; 14+ messages in thread From: Frank Rowand @ 2015-10-03 4:49 UTC (permalink / raw) To: david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+, jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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); ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <560F5E44.9080006-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string() [not found] ` <560F5E44.9080006-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-06 4:10 ` David Gibson [not found] ` <20151006041000.GI3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: David Gibson @ 2015-10-06 4:10 UTC (permalink / raw) To: Frank Rowand; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20151006041000.GI3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>]
* Re: [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string() [not found] ` <20151006041000.GI3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> @ 2015-10-06 7:32 ` Frank Rowand [not found] ` <56137904.9080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: Frank Rowand @ 2015-10-06 7:32 UTC (permalink / raw) To: David Gibson; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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; } ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <56137904.9080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string() [not found] ` <56137904.9080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-06 10:46 ` David Gibson 0 siblings, 0 replies; 14+ messages in thread From: David Gibson @ 2015-10-06 10:46 UTC (permalink / raw) To: Frank Rowand; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH v6 2/3] dtc: dts source location annotation [not found] ` <560F5D15.9060606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-03 4:49 ` [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string() Frank Rowand @ 2015-10-03 4:52 ` Frank Rowand [not found] ` <560F5F20.30709-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 1 sibling, 1 reply; 14+ messages in thread From: Frank Rowand @ 2015-10-03 4:52 UTC (permalink / raw) To: frowand.list-Re5JQEeQqe8AvxtiuMwx3w Cc: david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+, jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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)); } ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <560F5F20.30709-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC PATCH v6 2/3] dtc: dts source location annotation [not found] ` <560F5F20.30709-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-06 4:56 ` David Gibson [not found] ` <20151006045607.GK3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: David Gibson @ 2015-10-06 4:56 UTC (permalink / raw) To: Frank Rowand; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20151006045607.GK3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>]
* Re: [RFC PATCH v6 2/3] dtc: dts source location annotation [not found] ` <20151006045607.GK3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> @ 2015-10-06 7:38 ` Frank Rowand [not found] ` <56137A6B.6080805-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-06 7:45 ` Frank Rowand 1 sibling, 1 reply; 14+ messages in thread From: Frank Rowand @ 2015-10-06 7:38 UTC (permalink / raw) To: David Gibson; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <56137A6B.6080805-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC PATCH v6 2/3] dtc: dts source location annotation [not found] ` <56137A6B.6080805-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-07 5:27 ` David Gibson 0 siblings, 0 replies; 14+ messages in thread From: David Gibson @ 2015-10-07 5:27 UTC (permalink / raw) To: Frank Rowand; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v6 2/3] dtc: dts source location annotation [not found] ` <20151006045607.GK3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 2015-10-06 7:38 ` Frank Rowand @ 2015-10-06 7:45 ` Frank Rowand [not found] ` <56137C1E.8060005-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 1 sibling, 1 reply; 14+ messages in thread From: Frank Rowand @ 2015-10-06 7:45 UTC (permalink / raw) To: David Gibson; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <56137C1E.8060005-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC PATCH v6 2/3] dtc: dts source location annotation [not found] ` <56137C1E.8060005-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-07 5:32 ` David Gibson [not found] ` <20151007053246.GS3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: David Gibson @ 2015-10-07 5:32 UTC (permalink / raw) To: Frank Rowand; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20151007053246.GS3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>]
* Re: [RFC PATCH v6 2/3] dtc: dts source location annotation [not found] ` <20151007053246.GS3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> @ 2015-10-07 6:58 ` Frank Rowand 0 siblings, 0 replies; 14+ messages in thread From: Frank Rowand @ 2015-10-07 6:58 UTC (permalink / raw) To: David Gibson; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <560F5FB5.3020602@gmail.com>]
[parent not found: <560F5FB5.3020602-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [RFC PATCH v6 3/3] dtc: dts source location annotation, short location format [not found] ` <560F5FB5.3020602-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2015-10-06 5:01 ` David Gibson [not found] ` <20151006050114.GL3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 0 siblings, 1 reply; 14+ messages in thread From: David Gibson @ 2015-10-06 5:01 UTC (permalink / raw) To: Frank Rowand; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20151006050114.GL3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>]
* Re: [RFC PATCH v6 3/3] dtc: dts source location annotation, short location format [not found] ` <20151006050114.GL3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> @ 2015-10-06 7:32 ` Frank Rowand 0 siblings, 0 replies; 14+ messages in thread From: Frank Rowand @ 2015-10-06 7:32 UTC (permalink / raw) To: David Gibson; +Cc: jdl-CYoMK+44s/E, devicetree-compiler-u79uwXL29TY76Z2rM5mHXA 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, ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-10-07 6:58 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-03 4:44 [RFC PATCH v6 0/2] dtc: dts source location annotation Frank Rowand [not found] ` <560F5D15.9060606-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-03 4:49 ` [RFC PATCH v6 1/3] dtc: protect against null pointer dereference in srcpos_string() Frank Rowand [not found] ` <560F5E44.9080006-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-06 4:10 ` David Gibson [not found] ` <20151006041000.GI3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 2015-10-06 7:32 ` Frank Rowand [not found] ` <56137904.9080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-06 10:46 ` David Gibson 2015-10-03 4:52 ` [RFC PATCH v6 2/3] dtc: dts source location annotation Frank Rowand [not found] ` <560F5F20.30709-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-06 4:56 ` David Gibson [not found] ` <20151006045607.GK3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 2015-10-06 7:38 ` Frank Rowand [not found] ` <56137A6B.6080805-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-07 5:27 ` David Gibson 2015-10-06 7:45 ` Frank Rowand [not found] ` <56137C1E.8060005-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-07 5:32 ` David Gibson [not found] ` <20151007053246.GS3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 2015-10-07 6:58 ` Frank Rowand [not found] ` <560F5FB5.3020602@gmail.com> [not found] ` <560F5FB5.3020602-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2015-10-06 5:01 ` [RFC PATCH v6 3/3] dtc: dts source location annotation, short location format David Gibson [not found] ` <20151006050114.GL3861-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org> 2015-10-06 7:32 ` Frank Rowand
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.