[RFC PATCH] checks: Use source position information for check failures

[RFC PATCH] checks: Use source position information for check failures

[Rob Herring]

Now that we retain source position information of nodes and properties,
make that the preferred file name (and position) to print out in check
failures. This will greatly simplify finding and fixing check errors
because most errors are in included source .dtsi files and they get
duplicated every time the source file is included.

For now, only converting a few locations and using a new macro name. I
will convert all FAIL occurences once we agree on the new syntax. Also,
after this, some checks may need some rework to have more specific
line numbers of properties rather than nodes.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
This is based on Julia's annotation series.

 checks.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/checks.c b/checks.c
index 7845472742e0..2078c0fe75eb 100644
--- a/checks.c
+++ b/checks.c
@@ -19,6 +19,7 @@
  */
 
 #include "dtc.h"
+#include "srcpos.h"
 
 #ifdef TRACE_CHECKS
 #define TRACE(c, ...) \
@@ -72,7 +73,8 @@ struct check {
 #define CHECK(nm_, fn_, d_, ...) \
 	CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
 
-static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
+static inline void  PRINTF(4, 5) check_msg(struct check *c, struct dt_info *dti,
+					   struct srcpos *pos,
 					   const char *fmt, ...)
 {
 	va_list ap;
@@ -80,8 +82,15 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
 
 	if ((c->warn && (quiet < 1))
 	    || (c->error && (quiet < 2))) {
+		const char *file_str;
+		if (pos)
+			file_str = srcpos_string(pos);
+		else if (streq(dti->outname, "-"))
+			file_str = "<stdout>";
+		else
+			file_str = dti->outname;
 		fprintf(stderr, "%s: %s (%s): ",
-			strcmp(dti->outname, "-") ? dti->outname : "<stdout>",
+			file_str,
 			(c->error) ? "ERROR" : "Warning", c->name);
 		vfprintf(stderr, fmt, ap);
 		fprintf(stderr, "\n");
@@ -93,7 +102,14 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
 	do {								\
 		TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);	\
 		(c)->status = FAILED;					\
-		check_msg((c), dti, __VA_ARGS__);			\
+		check_msg((c), dti, NULL, __VA_ARGS__);			\
+	} while (0)
+
+#define FAIL_POS(c, dti, p, ...)						\
+	do {								\
+		TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);	\
+		(c)->status = FAILED;					\
+		check_msg((c), dti, p, __VA_ARGS__);			\
 	} while (0)
 
 static void check_nodes_props(struct check *c, struct dt_info *dti, struct node *node)
@@ -126,7 +142,7 @@ static bool run_check(struct check *c, struct dt_info *dti)
 		error = error || run_check(prq, dti);
 		if (prq->status != PASSED) {
 			c->status = PREREQ;
-			check_msg(c, dti, "Failed prerequisite '%s'",
+			check_msg(c, dti, NULL, "Failed prerequisite '%s'",
 				  c->prereq[i]->name);
 		}
 	}
@@ -1049,7 +1065,7 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
 	}
 
 	if (!has_reg)
-		FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
+		FAIL_POS(c, dti, node->srcpos, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
 		     node->fullpath);
 }
 WARNING(avoid_unnecessary_addr_size, check_avoid_unnecessary_addr_size, NULL, &avoid_default_addr_size);
@@ -1437,7 +1453,7 @@ static void check_graph_child_address(struct check *c, struct dt_info *dti,
 		cnt++;
 
 	if (cnt == 1 && node->addr_cells != -1)
-		FAIL(c, dti, "graph node '%s' has single child node, unit address is not necessary",
+		FAIL_POS(c, dti, node->srcpos, "graph node '%s' has single child node, unit address is not necessary",
 		     node->fullpath);
 }
 WARNING(graph_child_address, check_graph_child_address, NULL, &graph_nodes);
-- 
2.14.1

Re: [RFC PATCH] checks: Use source position information for check failures

[David Gibson]

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

On Mon, Jan 08, 2018 at 04:49:27PM -0600, Rob Herring wrote:
> Now that we retain source position information of nodes and properties,
> make that the preferred file name (and position) to print out in check
> failures. This will greatly simplify finding and fixing check errors
> because most errors are in included source .dtsi files and they get
> duplicated every time the source file is included.
> 
> For now, only converting a few locations and using a new macro name. I
> will convert all FAIL occurences once we agree on the new syntax. Also,
> after this, some checks may need some rework to have more specific
> line numbers of properties rather than nodes.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

So, I like the basic idea here - something I've kind of wanted for a while.

> ---
> This is based on Julia's annotation series.

But that series will need a new spin, which will require a new spin of
this at the last.

> 
>  checks.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 7845472742e0..2078c0fe75eb 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "dtc.h"
> +#include "srcpos.h"
>  
>  #ifdef TRACE_CHECKS
>  #define TRACE(c, ...) \
> @@ -72,7 +73,8 @@ struct check {
>  #define CHECK(nm_, fn_, d_, ...) \
>  	CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
>  
> -static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
> +static inline void  PRINTF(4, 5) check_msg(struct check *c, struct dt_info *dti,
> +					   struct srcpos *pos,
>  					   const char *fmt, ...)
>  {
>  	va_list ap;
> @@ -80,8 +82,15 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
>  
>  	if ((c->warn && (quiet < 1))
>  	    || (c->error && (quiet < 2))) {
> +		const char *file_str;
> +		if (pos)
> +			file_str = srcpos_string(pos);
> +		else if (streq(dti->outname, "-"))
> +			file_str = "<stdout>";
> +		else
> +			file_str = dti->outname;
>  		fprintf(stderr, "%s: %s (%s): ",
> -			strcmp(dti->outname, "-") ? dti->outname : "<stdout>",
> +			file_str,
>  			(c->error) ? "ERROR" : "Warning", c->name);

Incidentally, the reason it currently shows the output name, which
seems weird at first glance, is so that if you have a whole batch of
dtc commands running from a script or Makefile, you can at least tell
which command caused the check failure.

>  		vfprintf(stderr, fmt, ap);
>  		fprintf(stderr, "\n");
> @@ -93,7 +102,14 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
>  	do {								\
>  		TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);	\
>  		(c)->status = FAILED;					\
> -		check_msg((c), dti, __VA_ARGS__);			\
> +		check_msg((c), dti, NULL, __VA_ARGS__);			\
> +	} while (0)
> +
> +#define FAIL_POS(c, dti, p, ...)						\
> +	do {								\
> +		TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);	\
> +		(c)->status = FAILED;					\
> +		check_msg((c), dti, p, __VA_ARGS__);			\
>  	} while (0)
>  
>  static void check_nodes_props(struct check *c, struct dt_info *dti, struct node *node)
> @@ -126,7 +142,7 @@ static bool run_check(struct check *c, struct dt_info *dti)
>  		error = error || run_check(prq, dti);
>  		if (prq->status != PASSED) {
>  			c->status = PREREQ;
> -			check_msg(c, dti, "Failed prerequisite '%s'",
> +			check_msg(c, dti, NULL, "Failed prerequisite '%s'",
>  				  c->prereq[i]->name);
>  		}
>  	}
> @@ -1049,7 +1065,7 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
>  	}
>  
>  	if (!has_reg)
> -		FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
> +		FAIL_POS(c, dti, node->srcpos, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
>  		     node->fullpath);

Checks are already associated with a node, would it make more sense to
print the position information from the general code?

>  }
>  WARNING(avoid_unnecessary_addr_size, check_avoid_unnecessary_addr_size, NULL, &avoid_default_addr_size);
> @@ -1437,7 +1453,7 @@ static void check_graph_child_address(struct check *c, struct dt_info *dti,
>  		cnt++;
>  
>  	if (cnt == 1 && node->addr_cells != -1)
> -		FAIL(c, dti, "graph node '%s' has single child node, unit address is not necessary",
> +		FAIL_POS(c, dti, node->srcpos, "graph node '%s' has single child node, unit address is not necessary",
>  		     node->fullpath);
>  }
>  WARNING(graph_child_address, check_graph_child_address, NULL, &graph_nodes);

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

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

Re: [RFC PATCH] checks: Use source position information for check failures

[Rob Herring]

On Tue, Jan 9, 2018 at 11:30 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Jan 08, 2018 at 04:49:27PM -0600, Rob Herring wrote:
>> Now that we retain source position information of nodes and properties,
>> make that the preferred file name (and position) to print out in check
>> failures. This will greatly simplify finding and fixing check errors
>> because most errors are in included source .dtsi files and they get
>> duplicated every time the source file is included.
>>
>> For now, only converting a few locations and using a new macro name. I
>> will convert all FAIL occurences once we agree on the new syntax. Also,
>> after this, some checks may need some rework to have more specific
>> line numbers of properties rather than nodes.
>>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> So, I like the basic idea here - something I've kind of wanted for a while.
>
>> ---
>> This is based on Julia's annotation series.
>
> But that series will need a new spin, which will require a new spin of
> this at the last.

Yes, I expected that.

>>  checks.c | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/checks.c b/checks.c
>> index 7845472742e0..2078c0fe75eb 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -19,6 +19,7 @@
>>   */
>>
>>  #include "dtc.h"
>> +#include "srcpos.h"
>>
>>  #ifdef TRACE_CHECKS
>>  #define TRACE(c, ...) \
>> @@ -72,7 +73,8 @@ struct check {
>>  #define CHECK(nm_, fn_, d_, ...) \
>>       CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
>>
>> -static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
>> +static inline void  PRINTF(4, 5) check_msg(struct check *c, struct dt_info *dti,
>> +                                        struct srcpos *pos,
>>                                          const char *fmt, ...)
>>  {
>>       va_list ap;
>> @@ -80,8 +82,15 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
>>
>>       if ((c->warn && (quiet < 1))
>>           || (c->error && (quiet < 2))) {
>> +             const char *file_str;
>> +             if (pos)
>> +                     file_str = srcpos_string(pos);
>> +             else if (streq(dti->outname, "-"))
>> +                     file_str = "<stdout>";
>> +             else
>> +                     file_str = dti->outname;
>>               fprintf(stderr, "%s: %s (%s): ",
>> -                     strcmp(dti->outname, "-") ? dti->outname : "<stdout>",
>> +                     file_str,
>>                       (c->error) ? "ERROR" : "Warning", c->name);
>
> Incidentally, the reason it currently shows the output name, which
> seems weird at first glance, is so that if you have a whole batch of
> dtc commands running from a script or Makefile, you can at least tell
> which command caused the check failure.

Yes, better than nothing...

>
>>               vfprintf(stderr, fmt, ap);
>>               fprintf(stderr, "\n");
>> @@ -93,7 +102,14 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
>>       do {                                                            \
>>               TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);  \
>>               (c)->status = FAILED;                                   \
>> -             check_msg((c), dti, __VA_ARGS__);                       \
>> +             check_msg((c), dti, NULL, __VA_ARGS__);                 \
>> +     } while (0)
>> +
>> +#define FAIL_POS(c, dti, p, ...)                                             \
>> +     do {                                                            \
>> +             TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);  \
>> +             (c)->status = FAILED;                                   \
>> +             check_msg((c), dti, p, __VA_ARGS__);                    \
>>       } while (0)
>>
>>  static void check_nodes_props(struct check *c, struct dt_info *dti, struct node *node)
>> @@ -126,7 +142,7 @@ static bool run_check(struct check *c, struct dt_info *dti)
>>               error = error || run_check(prq, dti);
>>               if (prq->status != PASSED) {
>>                       c->status = PREREQ;
>> -                     check_msg(c, dti, "Failed prerequisite '%s'",
>> +                     check_msg(c, dti, NULL, "Failed prerequisite '%s'",
>>                                 c->prereq[i]->name);
>>               }
>>       }
>> @@ -1049,7 +1065,7 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
>>       }
>>
>>       if (!has_reg)
>> -             FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
>> +             FAIL_POS(c, dti, node->srcpos, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
>>                    node->fullpath);
>
> Checks are already associated with a node, would it make more sense to
> print the position information from the general code?

Not sure I follow the question. You mean pass in the struct node and
get the srcpos and fullpath inside check_msg?

While checks are associated with nodes, specific error messages may be
associated with properties. This is one example where we could make
the error message be the exact line (of the #address-cells or
#size-cells), but that would require re-working the check a bit to get
the property structs (and srcpos).

Rob

Re: [RFC PATCH] checks: Use source position information for check failures

[David Gibson]

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

On Wed, Jan 10, 2018 at 10:41:54AM -0600, Rob Herring wrote:
> On Tue, Jan 9, 2018 at 11:30 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Jan 08, 2018 at 04:49:27PM -0600, Rob Herring wrote:
> >> Now that we retain source position information of nodes and properties,
> >> make that the preferred file name (and position) to print out in check
> >> failures. This will greatly simplify finding and fixing check errors
> >> because most errors are in included source .dtsi files and they get
> >> duplicated every time the source file is included.
> >>
> >> For now, only converting a few locations and using a new macro name. I
> >> will convert all FAIL occurences once we agree on the new syntax. Also,
> >> after this, some checks may need some rework to have more specific
> >> line numbers of properties rather than nodes.
> >>
> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> > So, I like the basic idea here - something I've kind of wanted for a while.
> >
> >> ---
> >> This is based on Julia's annotation series.
> >
> > But that series will need a new spin, which will require a new spin of
> > this at the last.
> 
> Yes, I expected that.
> 
> >>  checks.c | 28 ++++++++++++++++++++++------
> >>  1 file changed, 22 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/checks.c b/checks.c
> >> index 7845472742e0..2078c0fe75eb 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -19,6 +19,7 @@
> >>   */
> >>
> >>  #include "dtc.h"
> >> +#include "srcpos.h"
> >>
> >>  #ifdef TRACE_CHECKS
> >>  #define TRACE(c, ...) \
> >> @@ -72,7 +73,8 @@ struct check {
> >>  #define CHECK(nm_, fn_, d_, ...) \
> >>       CHECK_ENTRY(nm_, fn_, d_, false, false, __VA_ARGS__)
> >>
> >> -static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
> >> +static inline void  PRINTF(4, 5) check_msg(struct check *c, struct dt_info *dti,
> >> +                                        struct srcpos *pos,
> >>                                          const char *fmt, ...)
> >>  {
> >>       va_list ap;
> >> @@ -80,8 +82,15 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
> >>
> >>       if ((c->warn && (quiet < 1))
> >>           || (c->error && (quiet < 2))) {
> >> +             const char *file_str;
> >> +             if (pos)
> >> +                     file_str = srcpos_string(pos);
> >> +             else if (streq(dti->outname, "-"))
> >> +                     file_str = "<stdout>";
> >> +             else
> >> +                     file_str = dti->outname;
> >>               fprintf(stderr, "%s: %s (%s): ",
> >> -                     strcmp(dti->outname, "-") ? dti->outname : "<stdout>",
> >> +                     file_str,
> >>                       (c->error) ? "ERROR" : "Warning", c->name);
> >
> > Incidentally, the reason it currently shows the output name, which
> > seems weird at first glance, is so that if you have a whole batch of
> > dtc commands running from a script or Makefile, you can at least tell
> > which command caused the check failure.
> 
> Yes, better than nothing...
> 
> >
> >>               vfprintf(stderr, fmt, ap);
> >>               fprintf(stderr, "\n");
> >> @@ -93,7 +102,14 @@ static inline void  PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti,
> >>       do {                                                            \
> >>               TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);  \
> >>               (c)->status = FAILED;                                   \
> >> -             check_msg((c), dti, __VA_ARGS__);                       \
> >> +             check_msg((c), dti, NULL, __VA_ARGS__);                 \
> >> +     } while (0)
> >> +
> >> +#define FAIL_POS(c, dti, p, ...)                                             \
> >> +     do {                                                            \
> >> +             TRACE((c), "\t\tFAILED at %s:%d", __FILE__, __LINE__);  \
> >> +             (c)->status = FAILED;                                   \
> >> +             check_msg((c), dti, p, __VA_ARGS__);                    \
> >>       } while (0)
> >>
> >>  static void check_nodes_props(struct check *c, struct dt_info *dti, struct node *node)
> >> @@ -126,7 +142,7 @@ static bool run_check(struct check *c, struct dt_info *dti)
> >>               error = error || run_check(prq, dti);
> >>               if (prq->status != PASSED) {
> >>                       c->status = PREREQ;
> >> -                     check_msg(c, dti, "Failed prerequisite '%s'",
> >> +                     check_msg(c, dti, NULL, "Failed prerequisite '%s'",
> >>                                 c->prereq[i]->name);
> >>               }
> >>       }
> >> @@ -1049,7 +1065,7 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
> >>       }
> >>
> >>       if (!has_reg)
> >> -             FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
> >> +             FAIL_POS(c, dti, node->srcpos, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
> >>                    node->fullpath);
> >
> > Checks are already associated with a node, would it make more sense to
> > print the position information from the general code?
> 
> Not sure I follow the question. You mean pass in the struct node and
> get the srcpos and fullpath inside check_msg?

Basically, yes.

> While checks are associated with nodes, specific error messages may be
> associated with properties. This is one example where we could make
> the error message be the exact line (of the #address-cells or
> #size-cells), but that would require re-working the check a bit to get
> the property structs (and srcpos).

Ah, good point.

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

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

Re: [RFC PATCH] checks: Use source position information for check failures

[Rob Herring]

On Wed, Jan 10, 2018 at 5:43 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Jan 10, 2018 at 10:41:54AM -0600, Rob Herring wrote:
>> On Tue, Jan 9, 2018 at 11:30 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Mon, Jan 08, 2018 at 04:49:27PM -0600, Rob Herring wrote:
>> >> Now that we retain source position information of nodes and properties,
>> >> make that the preferred file name (and position) to print out in check
>> >> failures. This will greatly simplify finding and fixing check errors
>> >> because most errors are in included source .dtsi files and they get
>> >> duplicated every time the source file is included.
>> >>
>> >> For now, only converting a few locations and using a new macro name. I
>> >> will convert all FAIL occurences once we agree on the new syntax. Also,
>> >> after this, some checks may need some rework to have more specific
>> >> line numbers of properties rather than nodes.

[...]

>> >> @@ -1049,7 +1065,7 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
>> >>       }
>> >>
>> >>       if (!has_reg)
>> >> -             FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
>> >> +             FAIL_POS(c, dti, node->srcpos, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
>> >>                    node->fullpath);
>> >
>> > Checks are already associated with a node, would it make more sense to
>> > print the position information from the general code?
>>
>> Not sure I follow the question. You mean pass in the struct node and
>> get the srcpos and fullpath inside check_msg?
>
> Basically, yes.

That would help getting the messages to be a more consistent form
like: 'source/file.dts:123: (ERROR|WARNING): /full/path/of/node: bad
news'

That had been something I wanted to do. The downside is it makes for
really long lines, but many messages already have the full path in
them. I guess we could go to 2 line messages where the 1st line is the
error and the 2nd line is the node path. The downside to that is I
typically do 'sort -u' (stripping the the dtb name) to de-duplicate
the errors as 10 boards including 1 SoC dtsi file gives me 10 of the
same error. Of course, printing the dts filename instead fixes that
problem.

It probably makes sense to do that in one step rather than reword
error messages twice.

>> While checks are associated with nodes, specific error messages may be
>> associated with properties. This is one example where we could make
>> the error message be the exact line (of the #address-cells or
>> #size-cells), but that would require re-working the check a bit to get
>> the property structs (and srcpos).

Re: [RFC PATCH] checks: Use source position information for check failures

[David Gibson]

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

On Thu, Jan 11, 2018 at 10:19:06AM -0600, Rob Herring wrote:
> On Wed, Jan 10, 2018 at 5:43 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Jan 10, 2018 at 10:41:54AM -0600, Rob Herring wrote:
> >> On Tue, Jan 9, 2018 at 11:30 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Mon, Jan 08, 2018 at 04:49:27PM -0600, Rob Herring wrote:
> >> >> Now that we retain source position information of nodes and properties,
> >> >> make that the preferred file name (and position) to print out in check
> >> >> failures. This will greatly simplify finding and fixing check errors
> >> >> because most errors are in included source .dtsi files and they get
> >> >> duplicated every time the source file is included.
> >> >>
> >> >> For now, only converting a few locations and using a new macro name. I
> >> >> will convert all FAIL occurences once we agree on the new syntax. Also,
> >> >> after this, some checks may need some rework to have more specific
> >> >> line numbers of properties rather than nodes.
> 
> [...]
> 
> >> >> @@ -1049,7 +1065,7 @@ static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d
> >> >>       }
> >> >>
> >> >>       if (!has_reg)
> >> >> -             FAIL(c, dti, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
> >> >> +             FAIL_POS(c, dti, node->srcpos, "unnecessary #address-cells/#size-cells without \"ranges\" or child \"reg\" property in %s",
> >> >>                    node->fullpath);
> >> >
> >> > Checks are already associated with a node, would it make more sense to
> >> > print the position information from the general code?
> >>
> >> Not sure I follow the question. You mean pass in the struct node and
> >> get the srcpos and fullpath inside check_msg?
> >
> > Basically, yes.
> 
> That would help getting the messages to be a more consistent form
> like: 'source/file.dts:123: (ERROR|WARNING): /full/path/of/node: bad
> news'
> 
> That had been something I wanted to do. The downside is it makes for
> really long lines, but many messages already have the full path in
> them. I guess we could go to 2 line messages where the 1st line is the
> error and the 2nd line is the node path. The downside to that is I
> typically do 'sort -u' (stripping the the dtb name) to de-duplicate
> the errors as 10 boards including 1 SoC dtsi file gives me 10 of the
> same error. Of course, printing the dts filename instead fixes that
> problem.

I suspect it might also make a bunch of existing testcases a bit of a
pain in the arse, too.

> It probably makes sense to do that in one step rather than reword
> error messages twice.
> 
> >> While checks are associated with nodes, specific error messages may be
> >> associated with properties. This is one example where we could make
> >> the error message be the exact line (of the #address-cells or
> >> #size-cells), but that would require re-working the check a bit to get
> >> the property structs (and srcpos).

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

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