On 9/20/2015 11:31 PM, David Gibson wrote:
> On Tue, Sep 15, 2015 at 09:22:18PM -0700, Frank Rowand wrote:
>>
>> From: Frank Rowand <frank.rowand@xxxxxxxxxxxxxx>
>>
>> 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.
>>
>> Not-signed-off-by: Frank Rowand <frank.rowand@xxxxxxxxxxxxxx>
>
> A like the concept, but I have some queries about the implementation.
>
>> ---
>> dtc-lexer.l | 2 +
>> dtc.c | 9 ++++++++
>> dtc.h | 14 +++++++++++++
>> livetree.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> srcpos.h | 6 +++++
>> treesource.c | 27 ++++++++++++++++++++-----
>> 6 files changed, 115 insertions(+), 5 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
>> @@ -126,6 +127,16 @@ bool data_is_one_string(struct data d);
>> #define MAX_NODENAME_LEN 31
>>
>> /* Live trees */
>> +struct src {
>> + char *name;
>> + int lineno;
>> +};
>
> I'd prefer to see the existing struct srcpos used, rather than
> creating a new structure.
My next version of the patch will still have struct src, but I
will explain why it exists. If your comment still stands with
me additional explanation, please repeat it.
>
>> +struct prop_src {
>> + struct prop_src *prev;
>> + struct src src;
>> +};
>> +
>> struct label {
>> bool deleted;
>> char *label;
>> @@ -140,6 +151,7 @@ struct property {
>> struct property *next;
>>
>> struct label *labels;
>> + struct src src;
>> };
>>
>> struct node {
>> @@ -158,6 +170,8 @@ struct node {
>> int addr_cells, size_cells;
>>
>> struct label *labels;
>> + struct src begin_src;
>> + struct src end_src;
>> };
>>
>> #define for_each_label_withdel(l0, l) \
>> Index: b/livetree.c
>> ===================================================================
>> --- a/livetree.c
>> +++ b/livetree.c
>> @@ -19,6 +19,9 @@
>> */
>>
>> #include "dtc.h"
>> +#include "srcpos.h"
>> +
>> +struct prop_src *prev_prop_src = NULL;
>
> So you've built a new global stack here, and I don't really understand
> what it's for. The parser already tracks source positions for each
> construct, so you should be able to just use that. You'd need to pass
> the srcpos from dtc-parser.y into the tree building calls in more
> places, but that should be it.
This is the part that I struggled with most. You have given me the clue
that I was overlooking. I'll send out a new version without the global
stack later today.
>
> Or have I missed something?
>
>> /*
>> * Tree building functions
>> @@ -58,6 +61,13 @@ struct property *build_property(char *na
>>
>> new->name = name;
>> new->val = val;
>> + if (current_srcfile) {
>> + new->src.name = current_srcfile->name;
>> + new->src.lineno = current_srcfile->lineno;
>> + } else {
>> + new->src.name = "__builtin__";
>> + new->src.lineno = 0;
>
> A comment explaining when this will happen in practice would be nice.
I'm not sure what you are asking. When current_srcfile will be null?
I'll add a comment for that.
>
> Incidentally.. what happens if you use dtb input and try to annotate?
There is no source information, which means the output would be of no use.
I chose to address this by the check in dtc.c to only allow --annotate
for inform and outform of "dts".
Thanks for the comments.
-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
