On Tue, Jan 9, 2018 at 11:30 PM, David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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
--
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
