[PATCH 1/5] sparse: Show expected vs. actual output on test failure

[PATCH 1/5] sparse: Show expected vs. actual output on test failure

[Pekka Enberg]

This patch changes 'make check' output to show sparse output compared to
expected results upon unexpected test failure. For example,
static-forward-decl.c output would look like this if it would not be tagged as
"known to fail":

       TEST     static forward declaration (static-forward-decl.c)
  error: actual error text does not match expected error text.
  --- static-forward-decl.c.error.expected	2011-08-22 06:29:40.000000000 +0000
  +++ static-forward-decl.c.error.got	2011-08-22 06:29:40.000000000 +0000
  @@ -0,0 +1 @@
  +static-forward-decl.c:3:5: warning: symbol 'f' was not declared. Should it be static?
  error: see static-forward-decl.c.error.* for further investigation.
  info: test 'static-forward-decl.c' is known to fail

This makes it easier to detect and analyze test breakage.

Cc: Christopher Li <sparse@chrisli.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 validation/test-suite |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/validation/test-suite b/validation/test-suite
index 42f7bd7..7549fd2 100755
--- a/validation/test-suite
+++ b/validation/test-suite
@@ -146,6 +146,8 @@ do_test()
 		if [ "$?" -eq "0" ]; then
 			echo "info: test '$file' is known to fail"
 			known_ko_tests=`expr $known_ko_tests + 1`
+		else
+			cat "$file".$stream.diff
 		fi
 		return 1
 	else
-- 
1.7.4.1

[PATCH 2/5] sparse: Enable unhandled validation tests

[Pekka Enberg]

This patch enables unhandled tests that did not have "check-name" specified.
It's pointless not to run them.

Cc: Christopher Li <sparse@chrisli.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 validation/badtype1.c                 |    5 +++++
 validation/badtype2.c                 |   14 ++++++++++++++
 validation/badtype3.c                 |   17 +++++++++++++++++
 validation/bitfields.c                |    3 +++
 validation/builtin_safe1.c            |   13 +++++++++++++
 validation/choose_expr.c              |   13 +++++++++++++
 validation/field-overlap.c            |    4 ++++
 validation/foul-bitwise.c             |   10 ++++++++++
 validation/inline_compound_literals.c |    4 ++++
 validation/struct-ns2.c               |    5 +++++
 validation/struct-size1.c             |    4 ++++
 validation/test-be.c                  |    3 +++
 validation/type1.c                    |    4 ++++
 13 files changed, 99 insertions(+), 0 deletions(-)

diff --git a/validation/badtype1.c b/validation/badtype1.c
index 4366d8d..ced7f9f 100644
--- a/validation/badtype1.c
+++ b/validation/badtype1.c
@@ -1 +1,6 @@
 static void foo(enum bar baz);
+
+/*
+ * check-name: enum not in scope
+ * check-known-to-fail
+ */
diff --git a/validation/badtype2.c b/validation/badtype2.c
index aad725d..90a5fa1 100644
--- a/validation/badtype2.c
+++ b/validation/badtype2.c
@@ -8,3 +8,17 @@ static undef foo(char *c)
     return bar();
   }
 }
+
+/*
+ * check-name: missing type
+ * check-error-start
+badtype2.c:2:14: error: Expected ; at end of declaration
+badtype2.c:2:14: error: got bar
+badtype2.c:3:14: error: Expected ; at end of declaration
+badtype2.c:3:14: error: got foo
+badtype2.c:6:3: error: Trying to use reserved word 'switch' as identifier
+badtype2.c:7:3: error: not in switch scope
+badtype2.c:10:1: error: Expected ; at the end of type declaration
+badtype2.c:10:1: error: got }
+ * check-error-end
+ */
diff --git a/validation/badtype3.c b/validation/badtype3.c
index 198ef87..20f346c 100644
--- a/validation/badtype3.c
+++ b/validation/badtype3.c
@@ -8,3 +8,20 @@ foo (int (*func) (undef, void *), void *data)
   }
   return err;
 }
+
+/*
+ * check-name: missing type in argument list
+ * check-error-start
+badtype3.c:2:18: warning: identifier list not in definition
+badtype3.c:2:24: error: Expected ) in function declarator
+badtype3.c:2:24: error: got ,
+badtype3.c:5:3: error: Trying to use reserved word 'while' as identifier
+badtype3.c:7:7: error: break/continue not in iterator scope
+badtype3.c:9:3: error: Trying to use reserved word 'return' as identifier
+badtype3.c:9:10: error: Expected ; at end of declaration
+badtype3.c:9:10: error: got err
+badtype3.c:10:1: error: Expected ; at the end of type declaration
+badtype3.c:10:1: error: got }
+badtype3.c:6:11: error: undefined identifier 'func'
+ * check-error-end
+ */
diff --git a/validation/bitfields.c b/validation/bitfields.c
index 16aa16d..ea24841 100644
--- a/validation/bitfields.c
+++ b/validation/bitfields.c
@@ -16,3 +16,6 @@ static int b(void)
 	return a[y.x];
 }
 
+/*
+ * check-name: bitfield to integer promotion
+ */
diff --git a/validation/builtin_safe1.c b/validation/builtin_safe1.c
index 8a8b979..2f6c9d2 100644
--- a/validation/builtin_safe1.c
+++ b/validation/builtin_safe1.c
@@ -24,3 +24,16 @@ static int foo(int x, int y)
   return x;
 }
 
+/*
+ * check-name: __builtin_safe
+ * check-known-to-fail
+ * check-error-start
+builtin_safe1.c:13:3: warning: Macro argument with side effects: x++
+builtin_safe1.c:14:3: warning: Macro argument with side effects: x+=1
+builtin_safe1.c:15:3: warning: Macro argument with side effects: x=x+1
+builtin_safe1.c:16:3: warning: Macro argument with side effects: x%=y
+builtin_safe1.c:17:3: warning: Macro argument with side effects: x=y
+builtin_safe1.c:18:3: warning: Macro argument with side effects: g(x)
+builtin_safe1.c:19:3: warning: Macro argument with side effects: (y,g(x))
+ * check-error-end
+ */
diff --git a/validation/choose_expr.c b/validation/choose_expr.c
index 55bfa0c..f6fd84c 100644
--- a/validation/choose_expr.c
+++ b/validation/choose_expr.c
@@ -2,3 +2,16 @@ static int x = __builtin_choose_expr(0,(char *)0,(void)0);
 static int y = __builtin_choose_expr(1,(char *)0,(void)0);
 static char s[42];
 static int z = 1/(sizeof(__builtin_choose_expr(1,s,0)) - 42);
+
+/*
+ * check-name: choose expr builtin
+ * check-error-start
+choose_expr.c:1:51: warning: incorrect type in initializer (different base types)
+choose_expr.c:1:51:    expected int static [signed] [toplevel] x
+choose_expr.c:1:51:    got void <noident>
+choose_expr.c:2:41: warning: incorrect type in initializer (different base types)
+choose_expr.c:2:41:    expected int static [signed] [toplevel] y
+choose_expr.c:2:41:    got char *<noident>
+choose_expr.c:4:17: warning: division by zero
+ * check-error-end
+ */
diff --git a/validation/field-overlap.c b/validation/field-overlap.c
index 15b974a..a6abab2 100644
--- a/validation/field-overlap.c
+++ b/validation/field-overlap.c
@@ -10,3 +10,7 @@ static struct {int x, y, z;} w[2] = {
 	{.x = 1, .y = 2, .z = 3},
 	{.x = 1, .y = 2, .z = 3}
 };
+
+/*
+ * check-name: field overlap
+ */
diff --git a/validation/foul-bitwise.c b/validation/foul-bitwise.c
index ca84be6..9e21eab 100644
--- a/validation/foul-bitwise.c
+++ b/validation/foul-bitwise.c
@@ -18,3 +18,13 @@ static __le16 bar(__le16 a)
 {
 	return -a;
 }
+
+/*
+ * check-name: foul bitwise
+ * check-error-start
+foul-bitwise.c:9:16: warning: restricted __le16 degrades to integer
+foul-bitwise.c:9:22: warning: restricted __le16 degrades to integer
+foul-bitwise.c:19:16: error: incompatible types for operation (-)
+foul-bitwise.c:19:16:    argument has type restricted __le16 [usertype] a
+ * check-error-end
+ */
diff --git a/validation/inline_compound_literals.c b/validation/inline_compound_literals.c
index 649d42a..fc223ff 100644
--- a/validation/inline_compound_literals.c
+++ b/validation/inline_compound_literals.c
@@ -16,3 +16,7 @@ static void foo(void)
 {
 	baz();
 }
+
+/*
+ * check-name: inline compound literals
+ */
diff --git a/validation/struct-ns2.c b/validation/struct-ns2.c
index b38af0a..4dd2c3b 100644
--- a/validation/struct-ns2.c
+++ b/validation/struct-ns2.c
@@ -12,3 +12,8 @@ h (void)
   struct Bar y;
   y.i = 1;
 }
+
+/*
+ * check-name: struct not in scope
+ * check-known-to-fail
+ */
diff --git a/validation/struct-size1.c b/validation/struct-size1.c
index 4748cd3..cf956a4 100644
--- a/validation/struct-size1.c
+++ b/validation/struct-size1.c
@@ -15,3 +15,7 @@ static const struct { int x; } foo[] = {{ 1 }};
 struct C {
   int bar[(sizeof foo/sizeof foo[0])];
 };
+
+/*
+ * check-name: struct size
+ */
diff --git a/validation/test-be.c b/validation/test-be.c
index 6b74555..deda3cc 100644
--- a/validation/test-be.c
+++ b/validation/test-be.c
@@ -41,3 +41,6 @@ int main (int argc, char *argv[])
 	return 0;
 }
 
+/*
+ * check-name: binary operations
+ */
diff --git a/validation/type1.c b/validation/type1.c
index 4f08f88..2a55f2a 100644
--- a/validation/type1.c
+++ b/validation/type1.c
@@ -21,3 +21,7 @@ static int test(struct hello *arg)
 {
 	return deref(arg->array);
 }
+
+/*
+ * check-name: "char []" to "char *" demotion
+ */
-- 
1.7.4.1

Re: [PATCH 2/5] sparse: Enable unhandled validation tests

[Josh Triplett]

On Mon, Aug 22, 2011 at 04:57:28PM +0300, Pekka Enberg wrote:
> This patch enables unhandled tests that did not have "check-name" specified.
> It's pointless not to run them.
[...]
> --- a/validation/badtype1.c
> +++ b/validation/badtype1.c
> @@ -1 +1,6 @@
>  static void foo(enum bar baz);
> +
> +/*
> + * check-name: enum not in scope
> + * check-known-to-fail
> + */
[...]
> --- a/validation/struct-ns2.c
> +++ b/validation/struct-ns2.c
> @@ -12,3 +12,8 @@ h (void)
>    struct Bar y;
>    y.i = 1;
>  }
> +
> +/*
> + * check-name: struct not in scope
> + * check-known-to-fail
> + */

Ouch, case in point; I didn't know these two tests had broken.

- Josh Triplett

Re: [PATCH 2/5] sparse: Enable unhandled validation tests

[Christopher Li]

On Mon, Aug 22, 2011 at 6:57 AM, Pekka Enberg <penberg@kernel.org> wrote:
> This patch enables unhandled tests that did not have "check-name" specified.
> It's pointless not to run them.

Good catch. Applied.

Chris

Re: [PATCH 2/5] sparse: Enable unhandled validation tests

[Pekka Enberg]

On 8/25/11 12:05 AM, Christopher Li wrote:
> On Mon, Aug 22, 2011 at 6:57 AM, Pekka Enberg<penberg@kernel.org>  wrote:
>> This patch enables unhandled tests that did not have "check-name" specified.
>> It's pointless not to run them.
> Good catch. Applied.
>
Is there something wrong with the following patches?

[PATCH 3/5] sparse: Fix __builtin_safe_p for pure and const functions
[PATCH 4/5] sparse, i386: Fix boolean bit size

Or haven't you gotten to them yet?

Re: [PATCH 2/5] sparse: Enable unhandled validation tests

[Christopher Li]

On Thu, Aug 25, 2011 at 3:30 AM, Pekka Enberg <penberg@kernel.org> wrote:
> [PATCH 3/5] sparse: Fix __builtin_safe_p for pure and const functions

My fault. I have been travelling with limited Internet access.

Nothing serious wrong in this patch. This patch is much harder than the
other minor fix ups. So good progress on your sparse hacking.

There are two things I am considering and I am try some simple modifications,
which did not go very far yet.
1) It would be better not adding the attribute bits into the modifiers.
   We have been moving the attribute bits out of the modifiers.
   For very simple reason, it the modifier has very limited number of bits.
   We need better infrastructure support from storing attribute in general
   any way.

   On the other hand, there are a few other attribute bits are done it the
   same way so that is not your fault. Not adding to the modifier bits is
   a much bigger change.

2) The expand_symbol_call(), it return early if the pure bit is set.
    That might have unwanted side effect if the pure attribute is combine
    with the symbol that require expand. e.g. If some one apply pure attribute
    to symbol __builtin_const_p (I agree it is silly BTW), then
__buildin_const_p
    will not expand properly. I would move the return statement a few
lines below

Over all I am tempting to just apply it and fix the rest when we clean up
the attribute bits.

Actually, I just did that. Push to the chrisl repository, you can check if that
works for you or not.

> [PATCH 4/5] sparse, i386: Fix boolean bit size

I will reply in the original email.

Chris

[PATCH 3/5] sparse: Fix __builtin_safe_p for pure and const functions

[Pekka Enberg]

This patch fixes __builtin_safe_p() to work properly for calls to pure
functions.

Cc: Christopher Li <sparse@chrisli.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 expand.c                   |    3 +++
 parse.c                    |   10 +++++-----
 show-parse.c               |    1 +
 symbol.h                   |    1 +
 validation/builtin_safe1.c |    1 -
 5 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/expand.c b/expand.c
index b965dc3..91e14f3 100644
--- a/expand.c
+++ b/expand.c
@@ -785,6 +785,9 @@ static int expand_symbol_call(struct expression *expr, int cost)
 	struct expression *fn = expr->fn;
 	struct symbol *ctype = fn->ctype;
 
+	if (ctype->ctype.modifiers & MOD_PURE)
+		return 0;
+
 	if (fn->type != EXPR_PREOP)
 		return SIDE_EFFECTS;
 
diff --git a/parse.c b/parse.c
index 082c2c4..6d8678e 100644
--- a/parse.c
+++ b/parse.c
@@ -463,6 +463,11 @@ static struct init_keyword {
 	{ "__transparent_union__",	NS_KEYWORD,	.op = &transparent_union_op },
 	{ "noreturn",	NS_KEYWORD,	MOD_NORETURN,	.op = &attr_mod_op },
 	{ "__noreturn__",	NS_KEYWORD,	MOD_NORETURN,	.op = &attr_mod_op },
+	{ "pure",	NS_KEYWORD,	MOD_PURE,	.op = &attr_mod_op },
+	{"__pure__",	NS_KEYWORD,	MOD_PURE,	.op = &attr_mod_op },
+	{"const",	NS_KEYWORD,	MOD_PURE,	.op = &attr_mod_op },
+	{"__const",	NS_KEYWORD,	MOD_PURE,	.op = &attr_mod_op },
+	{"__const__",	NS_KEYWORD,	MOD_PURE,	.op = &attr_mod_op },
 
 	{ "__mode__",	NS_KEYWORD,	.op = &mode_op },
 	{ "QI",		NS_KEYWORD,	MOD_CHAR,	.op = &mode_QI_op },
@@ -494,9 +499,6 @@ const char *ignored_attributes[] = {
 	"__cdecl__",
 	"cold",
 	"__cold__",
-	"const",
-	"__const",
-	"__const__",
 	"constructor",
 	"__constructor__",
 	"deprecated",
@@ -545,8 +547,6 @@ const char *ignored_attributes[] = {
 	"nothrow",
 	"__nothrow",
 	"__nothrow__",
-	"pure",
-	"__pure__",
 	"regparm",
 	"__regparm__",
 	"section",
diff --git a/show-parse.c b/show-parse.c
index a5beafe..1333e30 100644
--- a/show-parse.c
+++ b/show-parse.c
@@ -131,6 +131,7 @@ const char *modifier_string(unsigned long mod)
 		{MOD_NORETURN,		"[noreturn]"},
 		{MOD_EXPLICITLY_SIGNED,	"[explicitly-signed]"},
 		{MOD_BITWISE,		"[bitwise]"},
+		{MOD_PURE,		"[pure]"},
 	};
 
 	for (i = 0; i < ARRAY_SIZE(mod_names); i++) {
diff --git a/symbol.h b/symbol.h
index 2b8f20e..1e74579 100644
--- a/symbol.h
+++ b/symbol.h
@@ -198,6 +198,7 @@ struct symbol {
 #define MOD_LONG	0x0400
 #define MOD_LONGLONG	0x0800
 #define MOD_LONGLONGLONG	0x1000
+#define MOD_PURE	0x2000
 
 #define MOD_TYPEDEF	0x10000
 
diff --git a/validation/builtin_safe1.c b/validation/builtin_safe1.c
index 2f6c9d2..eeddcc8 100644
--- a/validation/builtin_safe1.c
+++ b/validation/builtin_safe1.c
@@ -26,7 +26,6 @@ static int foo(int x, int y)
 
 /*
  * check-name: __builtin_safe
- * check-known-to-fail
  * check-error-start
 builtin_safe1.c:13:3: warning: Macro argument with side effects: x++
 builtin_safe1.c:14:3: warning: Macro argument with side effects: x+=1
-- 
1.7.4.1

[PATCH 4/5] sparse, i386: Fix boolean bit size

[Pekka Enberg]

The value of 'ctype->bit_size' is set to 1 for booleans which confuses the i386
backend:

  ./compile allocate.c
  compile: compile-i386.c:1406: emit_binop: Assertion `0' failed.
  Aborted

Looking at the code, we assume that "bit_size / 8" gives a sane result on
various places. This patch fixes the problem by bumping bit_size to 8 for
booleans. This also makes sizeof(_Bool) return 1 which is consistent with what
GCC 4.4.3, for example, does.

Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 target.c                 |    2 +-
 validation/sizeof-bool.c |    3 ---
 2 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/target.c b/target.c
index 17b228a..6a535bc 100644
--- a/target.c
+++ b/target.c
@@ -14,7 +14,7 @@ int max_alignment = 16;
 /*
  * Integer data types
  */
-int bits_in_bool = 1;
+int bits_in_bool = 8;
 int bits_in_char = 8;
 int bits_in_short = 16;
 int bits_in_int = 32;
diff --git a/validation/sizeof-bool.c b/validation/sizeof-bool.c
index 6c68748..31b0585 100644
--- a/validation/sizeof-bool.c
+++ b/validation/sizeof-bool.c
@@ -4,9 +4,6 @@ static int a(void)
 }
 /*
  * check-name: sizeof(_Bool) is valid
- * check-description: sizeof(_Bool) was rejected because _Bool is not an even
- * number of bytes
  * check-error-start
-sizeof-bool.c:3:16: warning: expression using sizeof bool
  * check-error-end
  */
-- 
1.7.4.1

Re: [PATCH 4/5] sparse, i386: Fix boolean bit size

[Josh Triplett]

On Mon, Aug 22, 2011 at 04:57:30PM +0300, Pekka Enberg wrote:
> The value of 'ctype->bit_size' is set to 1 for booleans which confuses the i386
> backend:
> 
>   ./compile allocate.c
>   compile: compile-i386.c:1406: emit_binop: Assertion `0' failed.
>   Aborted
> 
> Looking at the code, we assume that "bit_size / 8" gives a sane result on
> various places. This patch fixes the problem by bumping bit_size to 8 for
> booleans. This also makes sizeof(_Bool) return 1 which is consistent with what
> GCC 4.4.3, for example, does.

Seems reasonable; that would make it consistent with the behavior of
structs or arrays of bool, at least in GCC.  As far as I know, GCC
doesn't have any logic to attempt to compress a bool down to a single
bit of storage.

- Josh Triplett

Re: [PATCH 4/5] sparse, i386: Fix boolean bit size

[Christopher Li]

On Mon, Aug 22, 2011 at 6:57 AM, Pekka Enberg <penberg@kernel.org> wrote:
> The value of 'ctype->bit_size' is set to 1 for booleans which confuses the i386
> backend:
>
>  ./compile allocate.c
>  compile: compile-i386.c:1406: emit_binop: Assertion `0' failed.
>  Aborted
>
> Looking at the code, we assume that "bit_size / 8" gives a sane result on
> various places. This patch fixes the problem by bumping bit_size to 8 for
> booleans. This also makes sizeof(_Bool) return 1 which is consistent with what
> GCC 4.4.3, for example, does.
>
> diff --git a/target.c b/target.c
> index 17b228a..6a535bc 100644
> --- a/target.c
> +++ b/target.c
> @@ -14,7 +14,7 @@ int max_alignment = 16;
>  /*
>  * Integer data types
>  */
> -int bits_in_bool = 1;
> +int bits_in_bool = 8;

I object this part. I consider the sizeof(_Bool) == 1 as external behaviour.
But internally we should know that the real useful part of bool is in just one
bit, not any bit of that  1 byte storage.

Allowing _Bool to be addressable is a separate issue. I consider those
rest of 7 bit as paddings. Not the useful part of _Bool.

>  int bits_in_char = 8;
>  int bits_in_short = 16;
>  int bits_in_int = 32;
> diff --git a/validation/sizeof-bool.c b/validation/sizeof-bool.c
> index 6c68748..31b0585 100644
> --- a/validation/sizeof-bool.c
> +++ b/validation/sizeof-bool.c
> @@ -4,9 +4,6 @@ static int a(void)
>  }
>  /*
>  * check-name: sizeof(_Bool) is valid
> - * check-description: sizeof(_Bool) was rejected because _Bool is not an even
> - * number of bytes
>  * check-error-start
> -sizeof-bool.c:3:16: warning: expression using sizeof bool
>  * check-error-end
>  */

We should just fix the validation.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] sparse, i386: Fix boolean bit size

[Pekka Enberg]

On Fri, Aug 26, 2011 at 6:59 AM, Christopher Li <sparse@chrisli.org> wrote:
> On Mon, Aug 22, 2011 at 6:57 AM, Pekka Enberg <penberg@kernel.org> wrote:
>> The value of 'ctype->bit_size' is set to 1 for booleans which confuses the i386
>> backend:
>>
>>  ./compile allocate.c
>>  compile: compile-i386.c:1406: emit_binop: Assertion `0' failed.
>>  Aborted
>>
>> Looking at the code, we assume that "bit_size / 8" gives a sane result on
>> various places. This patch fixes the problem by bumping bit_size to 8 for
>> booleans. This also makes sizeof(_Bool) return 1 which is consistent with what
>> GCC 4.4.3, for example, does.
>>
>> diff --git a/target.c b/target.c
>> index 17b228a..6a535bc 100644
>> --- a/target.c
>> +++ b/target.c
>> @@ -14,7 +14,7 @@ int max_alignment = 16;
>>  /*
>>  * Integer data types
>>  */
>> -int bits_in_bool = 1;
>> +int bits_in_bool = 8;
>
> I object this part. I consider the sizeof(_Bool) == 1 as external behaviour.
> But internally we should know that the real useful part of bool is in just one
> bit, not any bit of that  1 byte storage.

You missed the most important part of my reasoning: sparse code
already expects "bit_size / 8" to return a non-zero number and it's
not just compile-i386.c! So while I don't disagree with you that we
should internally know that a bool is just one bit, I don't consider
that to be relevant for this particular patch.

                        Pekka
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] sparse, i386: Fix boolean bit size

[Pekka Enberg]

On Fri, Aug 26, 2011 at 8:28 AM, Pekka Enberg <penberg@kernel.org> wrote:
> On Fri, Aug 26, 2011 at 6:59 AM, Christopher Li <sparse@chrisli.org> wrote:
>> On Mon, Aug 22, 2011 at 6:57 AM, Pekka Enberg <penberg@kernel.org> wrote:
>>> The value of 'ctype->bit_size' is set to 1 for booleans which confuses the i386
>>> backend:
>>>
>>>  ./compile allocate.c
>>>  compile: compile-i386.c:1406: emit_binop: Assertion `0' failed.
>>>  Aborted
>>>
>>> Looking at the code, we assume that "bit_size / 8" gives a sane result on
>>> various places. This patch fixes the problem by bumping bit_size to 8 for
>>> booleans. This also makes sizeof(_Bool) return 1 which is consistent with what
>>> GCC 4.4.3, for example, does.
>>>
>>> diff --git a/target.c b/target.c
>>> index 17b228a..6a535bc 100644
>>> --- a/target.c
>>> +++ b/target.c
>>> @@ -14,7 +14,7 @@ int max_alignment = 16;
>>>  /*
>>>  * Integer data types
>>>  */
>>> -int bits_in_bool = 1;
>>> +int bits_in_bool = 8;
>>
>> I object this part. I consider the sizeof(_Bool) == 1 as external behaviour.
>> But internally we should know that the real useful part of bool is in just one
>> bit, not any bit of that  1 byte storage.
>
> You missed the most important part of my reasoning: sparse code
> already expects "bit_size / 8" to return a non-zero number and it's
> not just compile-i386.c! So while I don't disagree with you that we
> should internally know that a bool is just one bit, I don't consider
> that to be relevant for this particular patch.

Oh, I see the same problem with sparse-llvm when generating code for OP_CAST.

This little C function:

  int sete(int x, int y)
  {
    return x == y;
  }

is compiled by GCC to:

00000170 <setne>:
 170:	55                   	push   %ebp
 171:	89 e5                	mov    %esp,%ebp
 173:	8b 45 0c             	mov    0xc(%ebp),%eax
 176:	39 45 08             	cmp    %eax,0x8(%ebp)
 179:	5d                   	pop    %ebp
 17a:	0f 95 c0             	setne  %al
 17d:	0f b6 c0             	movzbl %al,%eax
 180:	c3                   	ret

Sparse-llvm compiles the code to this which seems wrong:

00000140 <sete>:
 140:	31 c9                	xor    %ecx,%ecx
 142:	8b 44 24 08          	mov    0x8(%esp),%eax
 146:	39 44 24 04          	cmp    %eax,0x4(%esp)
 14a:	b8 ff ff ff ff       	mov    $0xffffffff,%eax
 14f:	0f 45 c1             	cmovne %ecx,%eax
 152:	c3                   	ret

However, if I bump up 'bits_in_bool' to 32:

diff --git a/target.c b/target.c
index 17b228a..2b83498 100644
--- a/target.c
+++ b/target.c
@@ -14,7 +14,7 @@ int max_alignment = 16;
 /*
  * Integer data types
  */
-int bits_in_bool = 1;
+int bits_in_bool = 32;
 int bits_in_char = 8;
 int bits_in_short = 16;
 int bits_in_int = 32;

I get this from sparse-llvm:

00000140 <sete>:
 140:	8b 44 24 08          	mov    0x8(%esp),%eax
 144:	39 44 24 04          	cmp    %eax,0x4(%esp)
 148:	0f 94 c0             	sete   %al
 14b:	c3                   	ret

I don't know sparse well enough to know what's the right thing to do
here. Linus, Jeff, Chris, someone, anyone, help!!!

                        Pekka
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] sparse: Add end-to-end compiler shell script

[Pekka Enberg]

This patch adds a 'kcc' shell script that combines the sparse's i386 backend
with GCC and GNU assembler to make it easier for people to find bugs in sparse.
You can, for example, attempt to build sparse with itself and see it crash and
burn:

  make && find . -name "*.o" | xargs rm
  make CC=./kcc
       CC       test-lexing.o
  FIXME! no value for symbol preprocess_only.  creating pseudo 1 (stack offset 4)
  {standard input}: Assembler messages:
  {standard input}:79: Error: operand type mismatch for `mov'
  make: *** [test-lexing.o] Error 1

Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 kcc |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)
 create mode 100755 kcc

diff --git a/kcc b/kcc
new file mode 100755
index 0000000..7f275a9
--- /dev/null
+++ b/kcc
@@ -0,0 +1,31 @@
+#!/bin/sh
+#
+# GCC compatible C compiler based on Sparse
+
+SPARSEOPTS=""
+ASOPTS=""
+DIRNAME=`dirname $0`
+
+use_gcc=1
+
+while [ $# -gt 0 ]; do
+	case $1 in
+	'-o')
+		ASOPTS=$ASOPTS"-o "$2" "
+		shift
+		;;
+	'-c')
+		use_gcc=0
+		;;
+	*)
+		SPARSEOPTS="$SPARSEOPTS $1 " ;;
+	esac
+	shift
+done
+
+if [ $use_gcc -eq 1 ]; then
+	gcc $ASOPTS $SPARSEOPTS
+
+else
+	$DIRNAME/compile $SPARSEOPTS | as $ASOPTS
+fi
-- 
1.7.4.1

Re: [PATCH 5/5] sparse: Add end-to-end compiler shell script

[Jeff Garzik]

On 08/22/2011 09:57 AM, Pekka Enberg wrote:
> This patch adds a 'kcc' shell script that combines the sparse's i386 backend
> with GCC and GNU assembler to make it easier for people to find bugs in sparse.
> You can, for example, attempt to build sparse with itself and see it crash and
> burn:
>
>    make&&  find . -name "*.o" | xargs rm
>    make CC=./kcc
>         CC       test-lexing.o
>    FIXME! no value for symbol preprocess_only.  creating pseudo 1 (stack offset 4)
>    {standard input}: Assembler messages:
>    {standard input}:79: Error: operand type mismatch for `mov'
>    make: *** [test-lexing.o] Error 1
>
> Cc: Christopher Li<sparse@chrisli.org>
> Cc: Jeff Garzik<jgarzik@redhat.com>
> Cc: Linus Torvalds<torvalds@linux-foundation.org>
> Signed-off-by: Pekka Enberg<penberg@kernel.org>

For the record, the i386 backend is quite incomplete and awful.

Ideally, an sparse compiler would work from linearized output, not from 
walking the tree as compile-i386 does.

	Jeff

Re: [PATCH 5/5] sparse: Add end-to-end compiler shell script

[Pekka Enberg]

On 8/22/11 5:51 PM, Jeff Garzik wrote:
> On 08/22/2011 09:57 AM, Pekka Enberg wrote:
>> This patch adds a 'kcc' shell script that combines the sparse's i386 
>> backend
>> with GCC and GNU assembler to make it easier for people to find bugs 
>> in sparse.
>> You can, for example, attempt to build sparse with itself and see it 
>> crash and
>> burn:
>>
>>    make&&  find . -name "*.o" | xargs rm
>>    make CC=./kcc
>>         CC       test-lexing.o
>>    FIXME! no value for symbol preprocess_only.  creating pseudo 1 
>> (stack offset 4)
>>    {standard input}: Assembler messages:
>>    {standard input}:79: Error: operand type mismatch for `mov'
>>    make: *** [test-lexing.o] Error 1
>>
>> Cc: Christopher Li<sparse@chrisli.org>
>> Cc: Jeff Garzik<jgarzik@redhat.com>
>> Cc: Linus Torvalds<torvalds@linux-foundation.org>
>> Signed-off-by: Pekka Enberg<penberg@kernel.org>
>
> For the record, the i386 backend is quite incomplete and awful.
>
> Ideally, an sparse compiler would work from linearized output, not 
> from walking the tree as compile-i386 does.

Christopher, please drop this patch. I'm not planning to continue
hacking on compile-i386.c.

                                     Pekka

Re: [PATCH 1/5] sparse: Show expected vs. actual output on test failure

[Christopher Li]

On Mon, Aug 22, 2011 at 6:57 AM, Pekka Enberg <penberg@kernel.org> wrote:
> This patch changes 'make check' output to show sparse output compared to
> expected results upon unexpected test failure. For example,
> static-forward-decl.c output would look like this if it would not be tagged as
> "known to fail":

I like that. Some minor suggestion:

There are two files to check for,  "output" and "error".
"$stream" is the loop variable. You only print the last one in the loop.
The "output" diff file does not show.

I think we might just give the same error message for
"known to fail" case as well. Yes, it is annoying to see them,
but that is the point, we want to fix them.

Chris

Re: [PATCH 1/5] sparse: Show expected vs. actual output on test failure

[Pekka Enberg]

On Tue, 23 Aug 2011, Christopher Li wrote:
> I think we might just give the same error message for
> "known to fail" case as well. Yes, it is annoying to see them,
> but that is the point, we want to fix them.

No, people who did not break them should not need to see the errors! If 
you're going to have a "known to fail" flag, then it really should be 
silent *by default*. When I run make check, I want to know I didn't break 
anything. If someone flagged a test as "known to fail", I really don't 
need to care about it.

Alternatively, we could get rid of the "known to fail" tag completely and 
just make sure the tests pass all the time. I actually wanted to do that 
but didn't have the skills to fix the remaining issues ;-)

 			Pekka

Re: [PATCH 1/5] sparse: Show expected vs. actual output on test failure

[Pekka Enberg]

On 8/22/11 4:57 PM, Pekka Enberg wrote:
> This patch changes 'make check' output to show sparse output compared to
> expected results upon unexpected test failure. For example,
> static-forward-decl.c output would look like this if it would not be tagged as
> "known to fail":
>
>         TEST     static forward declaration (static-forward-decl.c)
>    error: actual error text does not match expected error text.
>    --- static-forward-decl.c.error.expected	2011-08-22 06:29:40.000000000 +0000
>    +++ static-forward-decl.c.error.got	2011-08-22 06:29:40.000000000 +0000
>    @@ -0,0 +1 @@
>    +static-forward-decl.c:3:5: warning: symbol 'f' was not declared. Should it be static?
>    error: see static-forward-decl.c.error.* for further investigation.
>    info: test 'static-forward-decl.c' is known to fail
>
> This makes it easier to detect and analyze test breakage.
>
> Cc: Christopher Li<sparse@chrisli.org>
> Cc: Linus Torvalds<torvalds@linux-foundation.org>
> Signed-off-by: Pekka Enberg<penberg@kernel.org>
> ---
>   validation/test-suite |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/validation/test-suite b/validation/test-suite
> index 42f7bd7..7549fd2 100755
> --- a/validation/test-suite
> +++ b/validation/test-suite
> @@ -146,6 +146,8 @@ do_test()
>   		if [ "$?" -eq "0" ]; then
>   			echo "info: test '$file' is known to fail"
>   			known_ko_tests=`expr $known_ko_tests + 1`
> +		else
> +			cat "$file".$stream.diff
>   		fi
>   		return 1
>   	else

Chris, the patch you committed is different from mine:

http://git.kernel.org/?p=devel/sparse/chrisl/sparse.git;a=commitdiff;h=a7a00d5108c36b8baaf54814aa1f42583dabc754

Your patch now makes the runner verbose for "known to fail" tests
which is definitely not something we should do. If someone tagged
the test as "known to fail", we should treat it just like we treat
passed test cases.

The whole point of my patch was to make "make check" pinpoint
*unexpected* breakage so that anyone who bothers to do "make check"
on their patches can never cause regressions.

                         Pekka

Re: [PATCH 1/5] sparse: Show expected vs. actual output on test failure

[Christopher Li]

On Fri, Aug 26, 2011 at 2:10 AM, Pekka Enberg <penberg@kernel.org> wrote:
> Chris, the patch you committed is different from mine:
>
> http://git.kernel.org/?p=devel/sparse/chrisl/sparse.git;a=commitdiff;h=a7a00d5108c36b8baaf54814aa1f42583dabc754
>
> Your patch now makes the runner verbose for "known to fail" tests
> which is definitely not something we should do. If someone tagged
> the test as "known to fail", we should treat it just like we treat
> passed test cases.

That is intentional. I haven't seen you reply on my comment so I take
the liberty to change it. I do want to see the "known to fail" test
case.

>
> The whole point of my patch was to make "make check" pinpoint
> *unexpected* breakage so that anyone who bothers to do "make check"
> on their patches can never cause regressions.

How to you know your new patch did not break "known to fail" test
case in a different way than originally? It could be regression as well.

You can diff the aggregate output of "make check" before and after
the new patch to see if the new patch affect any test case at all.
That is better than blindly skip the "known to fail" test case.

Yes, I have a different usage case in mind. I want to look at what currently
breaks. In the long run, hopefully we fix all the known issues so this
wouldn't be any issue at all.

You can submit a patch to add a config for it if you are so insist on
the "I don't care about already broken test cases" usage case.
To me, diff the "make check" output is straightly better.

Chris

Re: [PATCH 1/5] sparse: Show expected vs. actual output on test failure

[Pekka Enberg]

On Fri, 26 Aug 2011, Christopher Li wrote:
>> Your patch now makes the runner verbose for "known to fail" tests
>> which is definitely not something we should do. If someone tagged
>> the test as "known to fail", we should treat it just like we treat
>> passed test cases.
>
> That is intentional. I haven't seen you reply on my comment so I take
> the liberty to change it. I do want to see the "known to fail" test
> case.

Sorry about that. I somehow missed the email.

>> The whole point of my patch was to make "make check" pinpoint
>> *unexpected* breakage so that anyone who bothers to do "make check"
>> on their patches can never cause regressions.
>
> How to you know your new patch did not break "known to fail" test
> case in a different way than originally? It could be regression as well.

I think that's irrelevant, really. I happen to think it's completely 
pointless to even run test cases that are "known to fail" because it 
creates confusion and adds little value.

And if we really care that a "known to fail" test case fails in a certain 
way, we can turn it into a "known _not_ to fail" and assert the exact 
failure case.

> You can diff the aggregate output of "make check" before and after
> the new patch to see if the new patch affect any test case at all.
> That is better than blindly skip the "known to fail" test case.

No, that doesn't scale at all if you run "make check" every few minutes 
like I do.

> Yes, I have a different usage case in mind. I want to look at what currently
> breaks. In the long run, hopefully we fix all the known issues so this
> wouldn't be any issue at all.

Agreed.

> You can submit a patch to add a config for it if you are so insist on
> the "I don't care about already broken test cases" usage case.
> To me, diff the "make check" output is straightly better.

OK, a flag would definitely work for me.

 			Pekka