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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- 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 --]