Re: four sparse patches

Re: four sparse patches

[Geoff Johnstone]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

> > I've attached four patches that I've written for sparse to
> > use it for a userland project. Please feel free to bin/
> > change/apply them as you see fit.
> >
> > I made them individually against the vanilla 0.4.1 tree
> > but they can be applied sequentially, albeit with a bit
> > of fuzz. They apply against the 20080330 git tree too.

> I need to get a "Signed-off-by" from you.

Attached. (The email address is real, but removing the
animals will get a less spam-filtered one.)


> Also, I'd prefer to let people on the linux-sparse mailing list
> <linux-sparse@vger.kernel.org> review patches as well,
> rather than relying solely on my own review. In the future,
> please CC patches to that list.

Done.

Thanks,
Geoff.

[-- Attachment #2: incomplete-struct.diff --]
[-- Type: application/octet-stream, Size: 1406 bytes --]

If I put the following in a public header:
 
   struct foo;
   typedef struct foo *Foo;
 
   void func (Foo f);
 
and the definition of struct foo in a private header:
 
   struct foo { int bar; }
 
then I get a sparse warning (different base type for argument 1) when
I compile the implementation file:
 
   #include "public.h"
   #include "private.h"
 
   void func (Foo f) { ... }
 
i.e. sparse doesn't realise that the incomplete structure definition
in the function  prototype refers to the same type as the complete
structure definition in the function definition.
 
*I think* that the patch fixes this - it silences the error - but I
don't know enough about sparse to know whether it's correct (or
whether it silences other legitimate errors, for example).
 

Signed-off-by: Geoff Johnstone <geoffSHEEP.johnstoneFROG@googlemail.com>

diff -ur sparse-0.4.1-vanilla/evaluate.c sparse-0.4.1/evaluate.c
--- sparse-0.4.1-vanilla/evaluate.c	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/evaluate.c	2008-03-30 14:53:34.656331005 +0100
@@ -696,8 +696,12 @@
 				     type);
 			return "bad types";
 		case SYM_RESTRICT:
+			return "different base types";
 		case SYM_UNION:
 		case SYM_STRUCT:
+			/* allow definition of incomplete structs and unions */
+			if (t1->ident == t2->ident)
+			  return NULL;
 			return "different base types";
 		case SYM_ARRAY:
 			/* XXX: we ought to compare sizes */

[-- Attachment #3: Wmix-decl-code.diff --]
[-- Type: application/octet-stream, Size: 3412 bytes --]

This adds -W[no-]mix-decl-code, which makes warnings about declarations
after statements a command-line option. (The code to implement the
warning was already in there via a #define; the patch just exposes it
at runtime.) Rationale: C99 allows them, C89 doesn't.

Signed-off-by: Geoff Johnstone <geoffSHEEP.johnstoneFROG@googlemail.com>


diff -ur sparse-0.4.1-vanilla/cgcc sparse-0.4.1/cgcc
--- sparse-0.4.1-vanilla/cgcc	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/cgcc	2008-03-30 15:12:43.758028336 +0100
@@ -69,7 +69,7 @@
 
 sub check_only_option {
     my ($arg) = @_;
-    return 1 if $arg =~ /^-W(no-?)?(default-bitfield-sign|one-bit-signed-bitfield|cast-truncate|bitwise|typesign|context|undef|ptr-subtraction-blows|cast-to-as|decl|transparent-union|address-space|enum-mismatch|do-while|old-initializer|non-pointer-null|paren-string|return-void)$/;
+    return 1 if $arg =~ /^-W(no-?)?(default-bitfield-sign|one-bit-signed-bitfield|cast-truncate|bitwise|typesign|context|undef|ptr-subtraction-blows|cast-to-as|decl|transparent-union|address-space|enum-mismatch|do-while|old-initializer|non-pointer-null|paren-string|return-void|mix-decl-code)$/;
     return 1 if $arg =~ /^-v(no-?)?(entry|dead)$/;
     return 0;
 }
diff -ur sparse-0.4.1-vanilla/lib.c sparse-0.4.1/lib.c
--- sparse-0.4.1-vanilla/lib.c	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/lib.c	2008-03-30 15:12:43.759028184 +0100
@@ -208,6 +208,7 @@
 int Wtypesign = 0;
 int Wundef = 0;
 int Wuninitialized = 1;
+int Wmixdeclcode = 1;
 
 int dbg_entry = 0;
 int dbg_dead = 0;
@@ -366,6 +367,7 @@
 	{ "typesign", &Wtypesign },
 	{ "undef", &Wundef },
 	{ "uninitialized", &Wuninitialized },
+	{ "mix-decl-code", &Wmixdeclcode },
 };
 
 enum {
diff -ur sparse-0.4.1-vanilla/lib.h sparse-0.4.1/lib.h
--- sparse-0.4.1-vanilla/lib.h	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/lib.h	2008-03-30 15:12:43.759028184 +0100
@@ -108,6 +108,7 @@
 extern int Wtypesign;
 extern int Wundef;
 extern int Wuninitialized;
+extern int Wmixdeclcode;
 
 extern int dbg_entry;
 extern int dbg_dead;
diff -ur sparse-0.4.1-vanilla/parse.c sparse-0.4.1/parse.c
--- sparse-0.4.1-vanilla/parse.c	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/parse.c	2008-03-30 15:12:43.761027879 +0100
@@ -28,8 +28,6 @@
 #include "expression.h"
 #include "target.h"
 
-#define warn_on_mixed (1)
-
 static struct symbol_list **function_symbol_list;
 struct symbol_list *function_computed_target_list;
 struct statement_list *function_computed_goto_list;
@@ -1810,7 +1808,7 @@
 			stmt = alloc_statement(token->pos, STMT_DECLARATION);
 			token = external_declaration(token, &stmt->declaration);
 		} else {
-			seen_statement = warn_on_mixed;
+			seen_statement = Wmixdeclcode;
 			token = statement(token, &stmt);
 		}
 		add_statement(list, stmt);
diff -ur sparse-0.4.1-vanilla/sparse.1 sparse-0.4.1/sparse.1
--- sparse-0.4.1-vanilla/sparse.1	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/sparse.1	2008-03-30 15:13:13.614473548 +0100
@@ -148,6 +148,15 @@
 \fB\-Wno\-non\-pointer\-null\fR.
 .
 .TP
+.B \-Wmix-decl-code
+Warn about declarations that are not at the start of a block.
+
+These declarations are permitted in C99 but not in C89.
+
+Sparse issues these warnings by default.  To turn them off, use
+\fB\-Wno\-mix\-decl\-code\fR.
+.
+.TP
 .B \-Wold\-initializer
 Warn about the use of the pre-C99 GCC syntax for designated initializers.
 

[-- Attachment #4: standard-c.diff --]
[-- Type: application/octet-stream, Size: 3323 bytes --]

This adds support for GCC's -std=... and -ansi command line options,
i.e. defining __STDC_VERSION__ and __STRICT_ANSI__ appropriately.
 
This means that code that invokes #error if __STDC_VERSION__ < 199901L
will work. It will probably also affect the userland header files too.
 
(Wmix-decl-code's default should probably depend on this, but I've
defaulted that to on for compatibility with vanilla 0.4.1.)

Signed-off-by: Geoff Johnstone <geoffSHEEP.johnstoneFROG@googlemail.com>

diff -ur sparse-0.4.1-vanilla/lib.c sparse-0.4.1/lib.c
--- sparse-0.4.1-vanilla/lib.c	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/lib.c	2008-03-30 15:19:09.695151208 +0100
@@ -214,6 +214,12 @@
 
 int preprocess_only;
 
+static enum { STANDARD_C89,
+              STANDARD_C94,
+              STANDARD_C99,
+              STANDARD_GNU89,
+              STANDARD_GNU99, } standard = STANDARD_GNU89;
+
 #define CMDLINE_INCLUDE 20
 int cmdline_include_nr = 0;
 struct cmdline_include cmdline_include[CMDLINE_INCLUDE];
@@ -495,6 +501,46 @@
 		return next;     // "-G0" or (bogus) terminal "-G"
 }
 
+static char **handle_switch_a(char *arg, char **next)
+{
+	if (!strcmp (arg, "ansi"))
+		standard = STANDARD_C89;
+
+	return next;
+}
+
+static char **handle_switch_s(char *arg, char **next)
+{
+	if (!strncmp (arg, "std=", 4))
+	{
+		arg += 4;
+
+		if (!strcmp (arg, "c89") ||
+		    !strcmp (arg, "iso9899:1990"))
+			standard = STANDARD_C89;
+
+		else if (!strcmp (arg, "iso9899:199409"))
+			standard = STANDARD_C94;
+
+		else if (!strcmp (arg, "c99") ||
+			 !strcmp (arg, "c9x") ||
+			 !strcmp (arg, "iso9899:1999") ||
+			 !strcmp (arg, "iso9899:199x"))
+			standard = STANDARD_C99;
+
+		else if (!strcmp (arg, "gnu89"))
+			standard = STANDARD_GNU89;
+
+		else if (!strcmp (arg, "gnu99") || !strcmp (arg, "gnu9x"))
+			standard = STANDARD_GNU99;
+
+		else
+			die ("Unsupported C dialect");
+	}
+
+	return next;
+}
+
 static char **handle_nostdinc(char *arg, char **next)
 {
 	add_pre_buffer("#nostdinc\n");
@@ -538,6 +584,8 @@
 	case 'O': return handle_switch_O(arg, next);
 	case 'f': return handle_switch_f(arg, next);
 	case 'G': return handle_switch_G(arg, next);
+	case 'a': return handle_switch_a(arg, next);
+	case 's': return handle_switch_s(arg, next);
 	default:
 		break;
 	}
@@ -613,6 +661,33 @@
 		add_pre_buffer("#weak_define __SIZE_TYPE__ unsigned int\n");
 	add_pre_buffer("#weak_define __STDC__ 1\n");
 
+	switch (standard)
+	{
+		case STANDARD_C89:
+			add_pre_buffer("#weak_define __STRICT_ANSI__\n");
+			break;
+
+		case STANDARD_C94:
+			add_pre_buffer("#weak_define __STDC_VERSION__ 199409L\n");
+			add_pre_buffer("#weak_define __STRICT_ANSI__\n");
+			break;
+
+		case STANDARD_C99:
+			add_pre_buffer("#weak_define __STDC_VERSION__ 199901L\n");
+			add_pre_buffer("#weak_define __STRICT_ANSI__\n");
+			break;
+
+		case STANDARD_GNU89:
+			break;
+
+		case STANDARD_GNU99:
+			add_pre_buffer("#weak_define __STDC_VERSION__ 199901L\n");
+			break;
+
+		default:
+			assert (0);
+	}
+
 	add_pre_buffer("#define __builtin_stdarg_start(a,b) ((a) = (__builtin_va_list)(&(b)))\n");
 	add_pre_buffer("#define __builtin_va_start(a,b) ((a) = (__builtin_va_list)(&(b)))\n");
 	add_pre_buffer("#define __builtin_va_arg(arg,type)  ({ type __va_arg_ret = *(type *)(arg); arg += sizeof(type); __va_arg_ret; })\n");

[-- Attachment #5: fortify-builtins.diff --]
[-- Type: application/octet-stream, Size: 2242 bytes --]

__FORTIFY_SOURCE=2 converts memcpy() into __builtin___memcpy_chk() etc.
The patch adds some/all? of the relevant builtin prototypes.

Signed-off-by: Geoff Johnstone <geoffSHEEP.johnstoneFROG@googlemail.com>


diff -ur sparse-0.4.1-vanilla/lib.c sparse-0.4.1/lib.c
--- sparse-0.4.1-vanilla/lib.c	2007-11-13 12:17:49.000000000 +0000
+++ sparse-0.4.1/lib.c	2008-03-30 15:11:15.722458714 +0100
@@ -592,6 +592,22 @@
 	add_pre_buffer("extern long __builtin_alpha_inslh(long, long);\n");
 	add_pre_buffer("extern long __builtin_alpha_cmpbge(long, long);\n");
 	add_pre_buffer("extern long __builtin_labs(long);\n");
+
+	/* And some __FORTIFY_SOURCE ones.. */
+	add_pre_buffer ("extern __SIZE_TYPE__ __builtin_object_size(void *, int);\n");
+	add_pre_buffer ("extern void * __builtin___memcpy_chk(void *, const void *, __SIZE_TYPE__, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern void * __builtin___memmove_chk(void *, const void *, __SIZE_TYPE__, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern void * __builtin___mempcpy_chk(void *, const void *, __SIZE_TYPE__, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern void * __builtin___memset_chk(void *, int, __SIZE_TYPE__, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern int __builtin___sprintf_chk(char *, int, __SIZE_TYPE__, const char *, ...);\n");
+	add_pre_buffer ("extern int __builtin___snprintf_chk(char *, __SIZE_TYPE__, int , __SIZE_TYPE__, const char *, ...);\n");
+	add_pre_buffer ("extern char * __builtin___stpcpy_chk(char *, const char *, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern char * __builtin___strcat_chk(char *, const char *, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern char * __builtin___strcpy_chk(char *, const char *, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern char * __builtin___strncat_chk(char *, const char *, __SIZE_TYPE__, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern char * __builtin___strncpy_chk(char *, const char *, __SIZE_TYPE__, __SIZE_TYPE__);\n");
+	add_pre_buffer ("extern int __builtin___vsprintf_chk(char *, int, __SIZE_TYPE__, const char *, __builtin_va_list);\n");
+	add_pre_buffer ("extern int __builtin___vsnprintf_chk(char *, __SIZE_TYPE__, int, __SIZE_TYPE__, const char *, __builtin_va_list ap);\n");
 }
 
 void create_builtin_stream(void)

Re: four sparse patches

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

Geoff Johnstone wrote:
>>> I've attached four patches that I've written for sparse to
>>> use it for a userland project.

Regarding Wmix-decl-code.diff, I agree that that warning definitely
needs an option controlling it. but GCC already has that option and
calls it "-Wdeclaration-after-statement", so matching GCC's name seems
potentially useful.  (However, I can imagine corner cases where it
might prove problematic, such as wanting to pass that option to GCC
and not Sparse or vice versa.)  Also, I agree that the default should
depend on the C standard in use, and I see no compatibility reason why
the warning should remain for code that explicitly asks for C99.
Thus, I haven't applied this version of the patch.  I'd love to apply
an updated version with those two changes.

Regarding incomplete structs, your patch seems reasonable as far as I
know, and it doesn't break the test suite, so I've applied and pushed
it.  Per your concerns, if this patch doesn't represent the correct
fix, the code can change later when we have a test case that breaks
with this patch.  Please do consider writing a patch for a new test
case based on your example.

Your argument parsing for -ansi and -std= looks great to me.  Applied
and pushed.

The new builtins for fortify handling seem fine.  Applied and pushed.

Thanks for your patches.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: four sparse patches

[Geoff Johnstone]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

>  Regarding Wmix-decl-code.diff, I agree that that warning definitely
>  needs an option controlling it. but GCC already has that option and
>  calls it "-Wdeclaration-after-statement", so matching GCC's name
>  seems potentially useful. (However, I can imagine corner cases
>  where it might prove problematic, such as wanting to pass that
>  option to GCC and not Sparse or vice versa.) Also, I agree that the
>  default should depend on the C standard in use, and I see no
>  compatibility reason why the warning should remain for code that
>  explicitly asks for C99.  Thus, I haven't applied this version of
>  the patch.

I've attached a revised version of the patch that:
 - Renames the option to -Wdeclaration-after-statement, as per GCC
   (wasn't hitherto aware of that gcc option).
 - Defaults based on chosen C dialect.
 - Adds a few tests. (I'll do a separate patch for tests for the
   incomplete struct patch.)
 - Was made wrt the git trunk at about 11:30 UTC on 12th April.

Geoff.

[-- Attachment #2: Wdeclaration-after-statement.diff --]
[-- Type: application/octet-stream, Size: 4944 bytes --]

This adds -W[no-]declaration-after-statement, which makes warnings about
declarations after statements a command-line option. (The code to implement
the warning was already in there via a #define; the patch just exposes it
at runtime.) Rationale: C99 allows them, C89 doesn't.

Signed-off-by: Geoff Johnstone <geoffSHEEP.johnstoneFROG@googlemail.com>

diff --git a/lib.c b/lib.c
index 75afdb7..0abcc9a 100644
--- a/lib.c
+++ b/lib.c
@@ -208,6 +208,7 @@ int Wtransparent_union = 1;
 int Wtypesign = 0;
 int Wundef = 0;
 int Wuninitialized = 1;
+int Wdeclarationafterstatement = -1;
 
 int dbg_entry = 0;
 int dbg_dead = 0;
@@ -372,6 +373,7 @@ static const struct warning {
 	{ "typesign", &Wtypesign },
 	{ "undef", &Wundef },
 	{ "uninitialized", &Wuninitialized },
+	{ "declaration-after-statement", &Wdeclarationafterstatement },
 };
 
 enum {
@@ -456,6 +458,28 @@ static void handle_onoff_switch_finalize(const struct warning warnings[], int n)
 static void handle_switch_W_finalize(void)
 {
 	handle_onoff_switch_finalize(warnings, sizeof(warnings) / sizeof(warnings[0]));
+
+	/* default Wdeclarationafterstatement based on the C dialect */
+	if (-1 == Wdeclarationafterstatement)
+	{
+		switch (standard)
+		{
+			case STANDARD_C89:
+			case STANDARD_C94:
+				Wdeclarationafterstatement = 1;
+				break;
+
+			case STANDARD_C99:
+			case STANDARD_GNU89:
+			case STANDARD_GNU99:
+				Wdeclarationafterstatement = 0;
+				break;
+
+			default:
+				assert (0);
+		}
+
+	}
 }
 
 static void handle_switch_v_finalize(void)
diff --git a/lib.h b/lib.h
index cef4ab8..42a3396 100644
--- a/lib.h
+++ b/lib.h
@@ -108,6 +108,7 @@ extern int Wtransparent_union;
 extern int Wtypesign;
 extern int Wundef;
 extern int Wuninitialized;
+extern int Wdeclarationafterstatement;
 
 extern int dbg_entry;
 extern int dbg_dead;
diff --git a/parse.c b/parse.c
index 6255737..8346fe2 100644
--- a/parse.c
+++ b/parse.c
@@ -28,8 +28,6 @@
 #include "expression.h"
 #include "target.h"
 
-#define warn_on_mixed (1)
-
 static struct symbol_list **function_symbol_list;
 struct symbol_list *function_computed_target_list;
 struct statement_list *function_computed_goto_list;
@@ -1810,7 +1808,7 @@ static struct token * statement_list(struct token *token, struct statement_list
 			stmt = alloc_statement(token->pos, STMT_DECLARATION);
 			token = external_declaration(token, &stmt->declaration);
 		} else {
-			seen_statement = warn_on_mixed;
+			seen_statement = Wdeclarationafterstatement;
 			token = statement(token, &stmt);
 		}
 		add_statement(list, stmt);
diff --git a/sparse.1 b/sparse.1
index 7a39d85..0f9131d 100644
--- a/sparse.1
+++ b/sparse.1
@@ -112,6 +112,16 @@ Sparse issues these warnings by default.  To turn them off, use
 \fB\-Wno\-decl\fR.
 .
 .TP
+.B \-Wdeclaration-after-statement
+Warn about declarations that are not at the start of a block.
+
+These declarations are permitted in C99 but not in C89.
+
+Sparse issues these warnings by default only when the C dialect is
+C89 (i.e. -ansi or -std=c89).  To turn them off, use
+\fB\-Wno\-declaration\-after\-statement\fR.
+.
+.TP
 .B \-Wdefault\-bitfield\-sign
 Warn about any bitfield with no explicit signedness.
 
diff --git a/validation/declaration-after-statement-ansi.c b/validation/declaration-after-statement-ansi.c
new file mode 100644
index 0000000..333410b
--- /dev/null
+++ b/validation/declaration-after-statement-ansi.c
@@ -0,0 +1,12 @@
+static void func (int i)
+{
+	i;
+	int j = i;
+}
+/*
+ * check-name: declaration after statement (ANSI)
+ * check-command: sparse -ansi $file
+ * check-error-start
+declaration-after-statement-ansi.c:4:2: warning: mixing declarations and code
+ * check-error-end
+ */
diff --git a/validation/declaration-after-statement-c89.c b/validation/declaration-after-statement-c89.c
new file mode 100644
index 0000000..78632cd
--- /dev/null
+++ b/validation/declaration-after-statement-c89.c
@@ -0,0 +1,12 @@
+static void func (int i)
+{
+	i;
+	int j = i;
+}
+/*
+ * check-name: declaration after statement (C89)
+ * check-command: sparse -std=c89 $file
+ * check-error-start
+declaration-after-statement-c89.c:4:2: warning: mixing declarations and code
+ * check-error-end
+ */
diff --git a/validation/declaration-after-statement-c99.c b/validation/declaration-after-statement-c99.c
new file mode 100644
index 0000000..dd36e6e
--- /dev/null
+++ b/validation/declaration-after-statement-c99.c
@@ -0,0 +1,9 @@
+static void func (int i)
+{
+	i;
+	int j = i;
+}
+/*
+ * check-name: declaration after statement (C99)
+ * check-command: sparse -std=c99 $file
+ */
diff --git a/validation/declaration-after-statement-default.c b/validation/declaration-after-statement-default.c
new file mode 100644
index 0000000..c3fe2cd
--- /dev/null
+++ b/validation/declaration-after-statement-default.c
@@ -0,0 +1,9 @@
+static void func (int i)
+{
+	i;
+	int j = i;
+}
+/*
+ * check-name: declaration after statement (default)
+ * check-command: sparse $file
+ */

Re: four sparse patches

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

Geoff Johnstone wrote:
>>  Regarding Wmix-decl-code.diff, I agree that that warning definitely
>>  needs an option controlling it. but GCC already has that option and
>>  calls it "-Wdeclaration-after-statement", so matching GCC's name
>>  seems potentially useful. (However, I can imagine corner cases
>>  where it might prove problematic, such as wanting to pass that
>>  option to GCC and not Sparse or vice versa.) Also, I agree that the
>>  default should depend on the C standard in use, and I see no
>>  compatibility reason why the warning should remain for code that
>>  explicitly asks for C99.  Thus, I haven't applied this version of
>>  the patch.
> 
> I've attached a revised version of the patch that:
>  - Renames the option to -Wdeclaration-after-statement, as per GCC
>    (wasn't hitherto aware of that gcc option).
>  - Defaults based on chosen C dialect.
>  - Adds a few tests. (I'll do a separate patch for tests for the
>    incomplete struct patch.)
>  - Was made wrt the git trunk at about 11:30 UTC on 12th April.

Looks good; applied and pushed.  Thanks!

- Josh Triplett



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]