[PATCH 0/8] avoid duplicated warnings

[PATCH 0/8] avoid duplicated warnings

[Luc Van Oostenryck]

The goal of this series is to eliminate some duplicated
warnings in order to reduce noise and allow to better
focus on the origin of the problem.

The series is also available in the Git repository at:
  git://github.com/lucvoo/sparse-dev.git expr-bad-twice

----------------------------------------------------------------
Luc Van Oostenryck (8):
  add testcases for duplicated warning about invalid types
  fix error in bad conditional
  early return if null ctype in evaluate_conditional()
  add helper: valid_type()
  use valid_type to avoid to warn twice on conditionals
  add helpers: valid_expr_type() & valid_subexpr_type()
  do not report bad types twice
  always evaluate both operands

 evaluate.c                   | 66 +++++++++++++++++++++++++++-----------------
 symbol.h                     |  5 ++++
 validation/bad-type-twice0.c | 13 +++++++++
 validation/bad-type-twice1.c | 16 +++++++++++
 validation/bad-type-twice2.c | 18 ++++++++++++
 5 files changed, 93 insertions(+), 25 deletions(-)
 create mode 100644 validation/bad-type-twice0.c
 create mode 100644 validation/bad-type-twice1.c
 create mode 100644 validation/bad-type-twice2.c

-- 
2.16.0

[PATCH 1/8] add testcases for duplicated warning about invalid types

[Luc Van Oostenryck]

Once a invalid type is present in some sub-expression,
every upper level of the expression has an invalid type
but we should only warn about the innermost/root error.

However, this is not currently the case and invalid types
can create duplicated warnings, sometimes even a succession
of such warning.

Add some testcases to catch such situations.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/bad-type-twice0.c | 14 ++++++++++++++
 validation/bad-type-twice1.c | 17 +++++++++++++++++
 validation/bad-type-twice2.c | 19 +++++++++++++++++++
 3 files changed, 50 insertions(+)
 create mode 100644 validation/bad-type-twice0.c
 create mode 100644 validation/bad-type-twice1.c
 create mode 100644 validation/bad-type-twice2.c

diff --git a/validation/bad-type-twice0.c b/validation/bad-type-twice0.c
new file mode 100644
index 000000000..2dbc91b03
--- /dev/null
+++ b/validation/bad-type-twice0.c
@@ -0,0 +1,14 @@
+static int foo(a)
+{
+	return a ? : 1;
+}
+
+/*
+ * check-name: bad-type-twice0
+ * check-known-to-fail
+ *
+ * check-error-start
+bad-type-twice0.c:3:16: error: incorrect type in conditional
+bad-type-twice0.c:3:16:    got incomplete type a
+ * check-error-end
+ */
diff --git a/validation/bad-type-twice1.c b/validation/bad-type-twice1.c
new file mode 100644
index 000000000..2f4e2838f
--- /dev/null
+++ b/validation/bad-type-twice1.c
@@ -0,0 +1,17 @@
+static unsigned long foo(unsigned long val, void *ref)
+{
+	if (val >= ref)
+		val = 0;
+	return val;
+}
+
+/*
+ * check-name: bad-type-twice1
+ * check-known-to-fail
+ *
+ * check-error-start
+bad-type-twice1.c:3:17: error: incompatible types for operation (>=)
+bad-type-twice1.c:3:17:    left side has type unsigned long [unsigned] val
+bad-type-twice1.c:3:17:    right side has type void *ref
+ * check-error-end
+ */
diff --git a/validation/bad-type-twice2.c b/validation/bad-type-twice2.c
new file mode 100644
index 000000000..916e82028
--- /dev/null
+++ b/validation/bad-type-twice2.c
@@ -0,0 +1,19 @@
+extern type_t fun(int);
+
+int foo(int x, int y)
+{
+	return ((int)fun(y)) + x;
+}
+
+/*
+ * check-name: bad-type-twice2
+ * check-known-to-fail
+ *
+ * check-error-start
+bad-type-twice2.c:1:8: warning: 'type_t' has implicit type
+bad-type-twice2.c:1:15: error: Expected ; at end of declaration
+bad-type-twice2.c:1:15: error: got fun
+bad-type-twice2.c:5:22: error: undefined identifier 'fun'
+bad-type-twice2.c:5:18: error: cast from unknown type
+ * check-error-end
+ */
-- 
2.16.0

[PATCH 2/8] fix error in bad conditional

[Luc Van Oostenryck]

Commit "b5867a33b (fix evaluation of a function or array symbol in conditionals)"
added a missing call to degenerate(epxr) but this must not be done
if the expression is erroneous.

fix this by bypassing the call to degenerate() if the ctype is NULL.

Fixes: b5867a33b62c04811784c6fc233c601a4f2b0841
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                   | 4 +++-
 validation/bad-type-twice0.c | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 402fa04af..027993983 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -894,7 +894,9 @@ static struct symbol *evaluate_conditional(struct expression *expr, int iterator
 			ctype = NULL;
 		}
 	}
-	ctype = degenerate(expr);
+
+	if (ctype)
+		ctype = degenerate(expr);
 
 	return ctype;
 }
diff --git a/validation/bad-type-twice0.c b/validation/bad-type-twice0.c
index 2dbc91b03..7a9073c52 100644
--- a/validation/bad-type-twice0.c
+++ b/validation/bad-type-twice0.c
@@ -5,7 +5,6 @@ static int foo(a)
 
 /*
  * check-name: bad-type-twice0
- * check-known-to-fail
  *
  * check-error-start
 bad-type-twice0.c:3:16: error: incorrect type in conditional
-- 
2.16.0

[PATCH 3/8] early return if null ctype in evaluate_conditional()

[Luc Van Oostenryck]

This function contains a few tests which must only be done
if the type is valid, same for the final call to degenerate().

Currently this is done by an "if (ctype) { ... }" but this
cause multiple NULL tests, take screen real estate and is
somehow error prone.

Change this by returning as soon as we know the ctype is
invalid since nothing good can be done after it.
This also makes, IMO, the code clearer.

NB: no functional changes here.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 027993983..1fc6111e4 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -879,25 +879,23 @@ static struct symbol *evaluate_conditional(struct expression *expr, int iterator
 		warning(expr->pos, "assignment expression in conditional");
 
 	ctype = evaluate_expression(expr);
-	if (ctype) {
-		if (is_safe_type(ctype))
-			warning(expr->pos, "testing a 'safe expression'");
-		if (is_func_type(ctype)) {
-			if (Waddress)
-				warning(expr->pos, "the address of %s will always evaluate as true", "a function");
-		} else if (is_array_type(ctype)) {
-			if (Waddress)
-				warning(expr->pos, "the address of %s will always evaluate as true", "an array");
-		} else if (!is_scalar_type(ctype)) {
-			sparse_error(expr->pos, "incorrect type in conditional");
-			info(expr->pos, "   got %s", show_typename(ctype));
-			ctype = NULL;
-		}
+	if (!ctype)
+		return NULL;
+	if (is_safe_type(ctype))
+		warning(expr->pos, "testing a 'safe expression'");
+	if (is_func_type(ctype)) {
+		if (Waddress)
+			warning(expr->pos, "the address of %s will always evaluate as true", "a function");
+	} else if (is_array_type(ctype)) {
+		if (Waddress)
+			warning(expr->pos, "the address of %s will always evaluate as true", "an array");
+	} else if (!is_scalar_type(ctype)) {
+		sparse_error(expr->pos, "incorrect type in conditional");
+		info(expr->pos, "   got %s", show_typename(ctype));
+		return NULL;
 	}
 
-	if (ctype)
-		ctype = degenerate(expr);

Re: [PATCH 3/8] early return if null ctype in evaluate_conditional()

[Christopher Li]

On Fri, Feb 2, 2018 at 4:17 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> NB: no functional changes here.
>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  evaluate.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/evaluate.c b/evaluate.c
> index 027993983..1fc6111e4 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -879,25 +879,23 @@ static struct symbol *evaluate_conditional(struct expression *expr, int iterator
>                 warning(expr->pos, "assignment expression in conditional");
>
>         ctype = evaluate_expression(expr);
> -       if (ctype) {
> -               if (is_safe_type(ctype))
> -                       warning(expr->pos, "testing a 'safe expression'");
> -               if (is_func_type(ctype)) {
> -                       if (Waddress)
> -                               warning(expr->pos, "the address of %s will always evaluate as true", "a function");
> -               } else if (is_array_type(ctype)) {
> -                       if (Waddress)
> -                               warning(expr->pos, "the address of %s will always evaluate as true", "an array");
> -               } else if (!is_scalar_type(ctype)) {
> -                       sparse_error(expr->pos, "incorrect type in conditional");
> -                       info(expr->pos, "   got %s", show_typename(ctype));
> -                       ctype = NULL;
> -               }
> +       if (!ctype)
> +               return NULL;
> +       if (is_safe_type(ctype))
> +               warning(expr->pos, "testing a 'safe expression'");
> +       if (is_func_type(ctype)) {
> +               if (Waddress)
> +                       warning(expr->pos, "the address of %s will always evaluate as true", "a function");
> +       } else if (is_array_type(ctype)) {
> +               if (Waddress)
> +                       warning(expr->pos, "the address of %s will always evaluate as true", "an array");
> +       } else if (!is_scalar_type(ctype)) {
> +               sparse_error(expr->pos, "incorrect type in conditional");
> +               info(expr->pos, "   got %s", show_typename(ctype));
> +               return NULL;

This is not straightly necessary to re-indent a section of the code.
It mess up the git annotation. For example, you can keep the "return NULL"
there and just move the

            ctype = degenerate(expr);

Inside if (ctype) statement block.

Any way, as you said, the function is the same.

Chris

Re: [PATCH 3/8] early return if null ctype in evaluate_conditional()

[Luc Van Oostenryck]

On Sat, Feb 10, 2018 at 11:28:44AM -0800, Christopher Li wrote:
> On Fri, Feb 2, 2018 at 4:17 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > NB: no functional changes here.
> >
> This is not straightly necessary to re-indent a section of the code.

Not needed but it was the prupose of the patch so that one of the following
patches become very clear about the functional change it made.
It's a very good things to separate functional from non-functional changes.

-- Luc

[PATCH 4/8] add helper: valid_type()

[Luc Van Oostenryck]

'bad_type' is used for ... bad types instead of NULL
in order to avoid later lots of null-checking.

Alas NULL is also often used and so we have to check
for NULL and 'bad_ctype' when checking a type's validity.

Add an helper: valid_type() to make the code a bit simpler.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 symbol.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/symbol.h b/symbol.h
index fd5746429..b4d1c2acc 100644
--- a/symbol.h
+++ b/symbol.h
@@ -309,6 +309,11 @@ extern void debug_symbol(struct symbol *);
 extern void merge_type(struct symbol *sym, struct symbol *base_type);
 extern void check_declaration(struct symbol *sym);
 
+static inline int valid_type(const struct symbol *ctype)
+{
+	return ctype && ctype != &bad_ctype;
+}
+
 static inline struct symbol *get_base_type(const struct symbol *sym)
 {
 	return examine_symbol_type(sym->ctype.base_type);
-- 
2.16.0

Re: [PATCH 4/8] add helper: valid_type()

[Christopher Li]

On Fri, Feb 2, 2018 at 4:17 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> 'bad_type' is used for ... bad types instead of NULL
> in order to avoid later lots of null-checking.

Is there a good reason to keep both NULL and "bad_type"
to represent bad types?

May be we should keep it simple, make NULL as the
haven't been evaluated. After evaluate if there is type
error, replace the NULL as bad type. So that we can tell
this expression is evaluated or not.

In other word, replace all usage of NULL as bad type into
"bad_type". That would be more consistent.

Chris

Re: [PATCH 4/8] add helper: valid_type()

[Luc Van Oostenryck]

On Sat, Feb 10, 2018 at 11:38:11AM -0800, Christopher Li wrote:
> On Fri, Feb 2, 2018 at 4:17 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > 'bad_type' is used for ... bad types instead of NULL
> > in order to avoid later lots of null-checking.
> 
> Is there a good reason to keep both NULL and "bad_type"
> to represent bad types?
> 
> May be we should keep it simple, make NULL as the
> haven't been evaluated. After evaluate if there is type
> error, replace the NULL as bad type. So that we can tell
> this expression is evaluated or not.
> 
> In other word, replace all usage of NULL as bad type into
> "bad_type". That would be more consistent.

This is indded one of the longer term goal (or at least to
make things much cleaner).
For the moment, it's far from being the case though, because
some of the functions return NULL to indicate that they
failed and then this return value is used and assigned.
It's quite a bit messy.

-- Luc

[PATCH 5/8] use valid_type to avoid to warn twice on conditionals

[Luc Van Oostenryck]

When evaluating a conditional, the expression is first evaluated
and some further verifications and processing are done if
the returned type is not NULL.

However, the returned type can also be 'bad_ctype' and if it is
the case, the additional verifications will just give meaningless
additional warnings.

Fix this by using the new helper valid_type() instead of just
testing for a null ctype.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                   | 2 +-
 validation/bad-type-twice1.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 1fc6111e4..3c4a25325 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -879,7 +879,7 @@ static struct symbol *evaluate_conditional(struct expression *expr, int iterator
 		warning(expr->pos, "assignment expression in conditional");
 
 	ctype = evaluate_expression(expr);
-	if (!ctype)
+	if (!valid_type(ctype))
 		return NULL;
 	if (is_safe_type(ctype))
 		warning(expr->pos, "testing a 'safe expression'");
diff --git a/validation/bad-type-twice1.c b/validation/bad-type-twice1.c
index 2f4e2838f..95cfd9e01 100644
--- a/validation/bad-type-twice1.c
+++ b/validation/bad-type-twice1.c
@@ -7,7 +7,6 @@ static unsigned long foo(unsigned long val, void *ref)
 
 /*
  * check-name: bad-type-twice1
- * check-known-to-fail
  *
  * check-error-start
 bad-type-twice1.c:3:17: error: incompatible types for operation (>=)
-- 
2.16.0

[PATCH 6/8] add helpers: valid_expr_type() & valid_subexpr_type()

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/evaluate.c b/evaluate.c
index 3c4a25325..14922017b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -47,6 +47,17 @@ struct symbol *current_fn;
 static struct symbol *degenerate(struct expression *expr);
 static struct symbol *evaluate_symbol(struct symbol *sym);
 
+static inline int valid_expr_type(struct expression *expr)
+{
+	return expr && valid_type(expr->ctype);
+}
+
+static inline int valid_subexpr_type(struct expression *expr)
+{
+	return valid_expr_type(expr->left)
+	    && valid_expr_type(expr->right);
+}
+
 static struct symbol *evaluate_symbol_expression(struct expression *expr)
 {
 	struct expression *addr;
-- 
2.16.0

[PATCH 7/8] do not report bad types twice

[Luc Van Oostenryck]

The type 'bad_ctype' is only used after an error has been detected.
Since this error has also been reported, there is no reasons
to issue more errors when a 'bad_ctype' is involved. This allow
to focus on the root cause of the error.

Fix this by checking in bad_expr_type() if one of the operands
is already a 'bad_ctype' and do not issue an diagnostic message
in this case.

Note: the kernel has a bunch of these situations where the
      exact same warning is given several times in a row,
      sometimes as much as a dozen time.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                   | 7 ++++++-
 validation/bad-type-twice2.c | 1 -
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 14922017b..f63c0344d 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -404,15 +404,20 @@ static inline int is_string_type(struct symbol *type)
 
 static struct symbol *bad_expr_type(struct expression *expr)
 {
-	sparse_error(expr->pos, "incompatible types for operation (%s)", show_special(expr->op));
 	switch (expr->type) {
 	case EXPR_BINOP:
 	case EXPR_COMPARE:
+		if (!valid_subexpr_type(expr))
+			break;
+		sparse_error(expr->pos, "incompatible types for operation (%s)", show_special(expr->op));
 		info(expr->pos, "   left side has type %s", show_typename(expr->left->ctype));
 		info(expr->pos, "   right side has type %s", show_typename(expr->right->ctype));
 		break;
 	case EXPR_PREOP:
 	case EXPR_POSTOP:
+		if (!valid_expr_type(expr->unop))
+			break;
+		sparse_error(expr->pos, "incompatible types for operation (%s)", show_special(expr->op));
 		info(expr->pos, "   argument has type %s", show_typename(expr->unop->ctype));
 		break;
 	default:
diff --git a/validation/bad-type-twice2.c b/validation/bad-type-twice2.c
index 916e82028..0aadd7a30 100644
--- a/validation/bad-type-twice2.c
+++ b/validation/bad-type-twice2.c
@@ -7,7 +7,6 @@ int foo(int x, int y)
 
 /*
  * check-name: bad-type-twice2
- * check-known-to-fail
  *
  * check-error-start
 bad-type-twice2.c:1:8: warning: 'type_t' has implicit type
-- 
2.16.0

[PATCH 8/8] always evaluate both operands

[Luc Van Oostenryck]

When evaluating a binary expression, the evaluation
already stop if the left operand is found erroneous.
The right one is thus not evaluated but it may contain
another error which will only be diagnosticated after the
first one is corrected and the file rechecked.
This is especially annoying when there are several independent
errors in some complex expression since it will need several
cycles of check-edit-recheck to get all errrors out.

Fix this by always evaluating both left & right operands
(and returning NULL if one of them is erroneous).

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index f63c0344d..bd10c6d9b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -3205,9 +3205,9 @@ struct symbol *evaluate_expression(struct expression *expr)
 	case EXPR_SYMBOL:
 		return evaluate_symbol_expression(expr);
 	case EXPR_BINOP:
-		if (!evaluate_expression(expr->left))
-			return NULL;
-		if (!evaluate_expression(expr->right))
+		evaluate_expression(expr->left);
+		evaluate_expression(expr->right);
+		if (!valid_subexpr_type(expr))
 			return NULL;
 		return evaluate_binop(expr);
 	case EXPR_LOGICAL:
@@ -3218,15 +3218,15 @@ struct symbol *evaluate_expression(struct expression *expr)
 			return NULL;
 		return evaluate_comma(expr);
 	case EXPR_COMPARE:
-		if (!evaluate_expression(expr->left))
-			return NULL;
-		if (!evaluate_expression(expr->right))
+		evaluate_expression(expr->left);
+		evaluate_expression(expr->right);
+		if (!valid_subexpr_type(expr))
 			return NULL;
 		return evaluate_compare(expr);
 	case EXPR_ASSIGNMENT:
-		if (!evaluate_expression(expr->left))
-			return NULL;
-		if (!evaluate_expression(expr->right))
+		evaluate_expression(expr->left);
+		evaluate_expression(expr->right);
+		if (!valid_subexpr_type(expr))
 			return NULL;
 		return evaluate_assignment(expr);
 	case EXPR_PREOP:
-- 
2.16.0