checks: Use source position information for check failures

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

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

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

checks.c +4 -1

index 7845472742e0..2078c0fe75eb 100644
@@ -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;

checks.c +9 -2

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

checks.c +1 -1

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

Quoted 6 lines · 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.

Quoted 6 lines

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

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

Quoted 2 lines

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

Quoted 37 lines

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

checks.c +10 -1

index 7845472742e0..2078c0fe75eb 100644
@@ -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): ",

checks.c static inline void PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti, · line 94

Quoted 3 lines

checks.c +1 -1

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

Quoted 31 lines

checks.c +9 -2

              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
      }
 

checks.c static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d · line 1069

Quoted 4 lines

checks.c +1 -1

      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?

Quoted 14 lines · David Gibson

Quoted 6 lines · 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.

Quoted 6 lines

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

Quoted 7 lines

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

Quoted 2 lines

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

Quoted 37 lines

Quoted 37 lines

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

checks.c +10 -1

index 7845472742e0..2078c0fe75eb 100644
@@ -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): ",

Quoted 8 lines

checks.c static inline void PRINTF(3, 4) check_msg(struct check *c, struct dt_info *dti, · line 94

Quoted 3 lines

checks.c +1 -1

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

Quoted 31 lines

Quoted 31 lines

checks.c +9 -2

              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
      }
 

Quoted 7 lines

checks.c static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d · line 1069

Quoted 4 lines

checks.c +1 -1

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

Quoted 13 lines · Rob Herring

Quoted 11 lines · David Gibson

Quoted 10 lines · 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.

Quoted 11 lines

Quoted 10 lines

Review static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d · line 1069

Quoted 7 lines

(quoted diff) +1 -1

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

Quoted 2 lines

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

Basically, yes.

Quoted 5 lines

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

Quoted 14 lines · David Gibson

Quoted 13 lines · Rob Herring

Quoted 11 lines · David Gibson

Quoted 10 lines · 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.

[...]

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Review static void check_avoid_unnecessary_addr_size(struct check *c, struct dt_info *d · line 1069

Quoted 7 lines

(quoted diff) +1 -1

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 5 lines

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