[PATCH 0/5] more validation of C99 for-loop initializers

[PATCH 0/5] more validation of C99 for-loop initializers

[Luc Van Oostenryck]

This serie adds some validations of C99-style for-loop
initializers: scope and storage.

Patch 1 replaces the current indirect test by a direct one
Patch 2 adds a test case for the scope
Patch 3 adds some test cases for storage.
Patch 4 is a preparatory patch for 5).
Patch 5 adds missing storage validation

Luc Van Oostenryck (5):
  replace test for c99 for-loop initializers
  add test case for scope of C99 for-loop declarations
  add test cases for storage of c99 for-loop declarations
  add a method to external_declaration()
  check the storage of C99 for-loop initializers

 lib.c                          |  2 +-
 parse.c                        | 37 ++++++++++++++++++++++++++++++-------
 parse.h                        |  3 ++-
 validation/c99-for-loop-decl.c | 40 ++++++++++++++++++++++++++++++++++++++++
 validation/c99-for-loop.c      | 36 ++++++++++++------------------------
 5 files changed, 85 insertions(+), 33 deletions(-)
 create mode 100644 validation/c99-for-loop-decl.c

-- 
2.11.0

[PATCH 1/5] replace test for c99 for-loop initializers

[Luc Van Oostenryck]

The existing test is an indirect test, using a warning
about context imbalance to show that some part of code
was discarded.

Now that we have the minimal tools to test the output of
test-linearize, use them to replace the test by a direct one.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
index 42246c513..427fde268 100644
--- a/validation/c99-for-loop.c
+++ b/validation/c99-for-loop.c
@@ -1,33 +1,21 @@
-int op(int);
-
-static int good(void)
+int c99(void);
+int c99(void)
 {
-	__context__(1);
-	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
-	}
-	__context__(-1);
-	return 1;
-}
+	int r = -1;
 
-static int bad(void)
-{
-	__context__(1);
 	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
+		r = i;
 	}
-	return 1;
+
+	return r;
 }
+
 /*
  * check-name: C99 for loop variable declaration
+ * check-command: test-linearize $file
  *
- * check-error-start
-c99-for-loop.c:16:12: warning: context imbalance in 'bad' - different lock contexts for basic block
- * check-error-end
+ * check-output-ignore
+ * check-output-contains: phisrc\\.
+ * check-output-contains: phi\\.
+ * check-output-contains: add\\.
  */
-- 
2.11.0

[PATCH 2/5] add test case for scope of C99 for-loop declarations

[Luc Van Oostenryck]

Insure that variable declared inside a C99 for-loop
have their scope restricted to this loop.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 validation/c99-for-loop-decl.c

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
new file mode 100644
index 000000000..c2ceaab99
--- /dev/null
+++ b/validation/c99-for-loop-decl.c
@@ -0,0 +1,18 @@
+static int bad_scope(void)
+{
+	int r = 0;
+
+	for (int i = 0; i < 10; i++) {
+		r = i;
+	}
+
+	return i;			/* check-should-fail */
+}
+
+/*
+ * check-name: C99 for-loop declarations
+ *
+ * check-error-start
+c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
+ * check-error-end
+ */
-- 
2.11.0

[PATCH 3/5] add test cases for storage of c99 for-loop declarations

[Luc Van Oostenryck]

Variable declared inside a C99 for-loop must have register
or automatic storage; static & extern storage are invalid.
These test cases verify that we warns if it is not the case.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index c2ceaab99..b9db8c9c6 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -9,10 +9,33 @@ static int bad_scope(void)
 	return i;			/* check-should-fail */
 }
 
+static int c99(void)
+{
+	int r = 0;
+
+	for (         int i = 0; i < 10; i++)	/* check-should-pass */
+		r = i;
+	for (    auto int j = 0; j < 10; j++)	/* check-should-pass */
+		r = j;
+	for (register int k = 0; k < 10; k++)	/* check-should-pass */
+		r = k;
+	for (  extern int l = 0; l < 10; l++)	/* check-should-fail */
+		r = l;
+	for (  extern int m;     m < 10; m++)	/* check-should-fail */
+		r = m;
+	for (  static int n = 0; n < 10; n++)	/* check-should-fail */
+		r = n;
+	return r;
+}
+
 /*
  * check-name: C99 for-loop declarations
+ * check-known-to-fail
  *
  * check-error-start
+c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
+c99-for-loop-decl.c:24:27: error: non-local var 'm' in for-loop initializer
+c99-for-loop-decl.c:26:27: error: non-local var 'n' in for-loop initializer
 c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
  * check-error-end
  */
-- 
2.11.0

[PATCH 4/5] add a method to external_declaration()

[Luc Van Oostenryck]

After parsing and validation, the symbols in the declaration
are added to the list given in argument, *if* they are not extern
symbols. The symbols that are extern are them not added to the list.

This is what is needed for usual declarations but ignoring extern
symbols make it impossible to emit a diagnostic in less usual
situation.

This is motivated by the validation of variable declaration inside
a for-loop initializer, which valid in C99 but only for variable
with local storage.

The changes consist in moving the part 'add the symbol to the list if
the symbol is not extern' to a separate function which will be called
by default.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c   |  2 +-
 parse.c | 22 +++++++++++++++-------
 parse.h |  3 ++-
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/lib.c b/lib.c
index d47325243..9badc09b3 100644
--- a/lib.c
+++ b/lib.c
@@ -1080,7 +1080,7 @@ static struct symbol_list *sparse_tokenstream(struct token *token)
 
 	// Parse the resulting C code
 	while (!eof_token(token))
-		token = external_declaration(token, &translation_unit_used_list);
+		token = external_declaration(token, NULL, &translation_unit_used_list);
 	return translation_unit_used_list;
 }
 
diff --git a/parse.c b/parse.c
index a4a126720..866186fd2 100644
--- a/parse.c
+++ b/parse.c
@@ -2230,7 +2230,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
 	e1 = NULL;
 	/* C99 variable declaration? */
 	if (lookup_type(token)) {
-		token = external_declaration(token, &syms);
+		token = external_declaration(token, NULL, &syms);
 	} else {
 		token = parse_expression(token, &e1);
 		token = expect(token, ';', "in 'for'");
@@ -2457,7 +2457,7 @@ static struct token * statement_list(struct token *token, struct statement_list
 				seen_statement = 0;
 			}
 			stmt = alloc_statement(token->pos, STMT_DECLARATION);
-			token = external_declaration(token, &stmt->declaration);
+			token = external_declaration(token, NULL, &stmt->declaration);
 		} else {
 			seen_statement = Wdeclarationafterstatement;
 			token = statement(token, &stmt);
@@ -2785,7 +2785,15 @@ static struct token *toplevel_asm_declaration(struct token *token, struct symbol
 	return token;
 }
 
-struct token *external_declaration(struct token *token, struct symbol_list **list)
+static void add_stmt_decl(struct symbol_list **list, struct symbol *decl)
+{
+	if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
+		add_symbol(list, decl);
+		fn_local_symbol(decl);
+	}
+}
+
+struct token *external_declaration(struct token *token, add_decl_t add_decl, struct symbol_list **list)
 {
 	struct ident *ident = NULL;
 	struct symbol *decl;
@@ -2864,6 +2872,9 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 		sparse_error(token->pos, "void declaration");
 	}
 
+	if (!add_decl)
+		add_decl = add_stmt_decl;
+
 	for (;;) {
 		if (!is_typedef && match_op(token, '=')) {
 			if (decl->ctype.modifiers & MOD_EXTERN) {
@@ -2873,10 +2884,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 			token = initializer(&decl->initializer, token->next);
 		}
 		if (!is_typedef) {
-			if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
-				add_symbol(list, decl);
-				fn_local_symbol(decl);
-			}
+			add_decl(list, decl);
 		}
 		check_declaration(decl);
 		if (decl->same_symbol) {
diff --git a/parse.h b/parse.h
index a2b3e3889..d3199727e 100644
--- a/parse.h
+++ b/parse.h
@@ -129,7 +129,8 @@ extern int show_statement(struct statement *);
 extern void show_statement_list(struct statement_list *, const char *);
 extern int show_expression(struct expression *);
 
-extern struct token *external_declaration(struct token *token, struct symbol_list **list);
+typedef void (*add_decl_t)(struct symbol_list **list, struct symbol *decl);
+extern struct token *external_declaration(struct token *token, add_decl_t add_decl, struct symbol_list **list);
 
 extern struct symbol *ctype_integer(int size, int want_unsigned);
 
-- 
2.11.0

[PATCH 5/5] check the storage of C99 for-loop initializers

[Luc Van Oostenryck]

In C99, it is valid to declarare a variable inside
a for-loop initializer but only when the storage is local
(automatic or register).

Until now this was not enforced.

Fix that by adding the appropriate checks called by
external_declaration() via the new function pointer when
parsing this declaration in a for-loop context.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                        | 17 ++++++++++++++++-
 validation/c99-for-loop-decl.c |  1 -
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/parse.c b/parse.c
index 866186fd2..5b029ccdc 100644
--- a/parse.c
+++ b/parse.c
@@ -2217,6 +2217,21 @@ static struct token *parse_return_statement(struct token *token, struct statemen
 	return expression_statement(token->next, &stmt->ret_value);
 }
 
+static void add_for_loop_decl(struct symbol_list **list, struct symbol *sym)
+{
+	unsigned long storage;
+
+	storage = sym->ctype.modifiers & MOD_STORAGE;
+	if (storage & ~(MOD_AUTO | MOD_REGISTER)) {
+		const char *name = show_ident(sym->ident);
+		sparse_error(sym->pos, "non-local var '%s' in for-loop initializer", name);
+		sym->ctype.modifiers &= ~MOD_STORAGE;
+	}
+
+	add_symbol(list, sym);
+	fn_local_symbol(sym);
+}
+
 static struct token *parse_for_statement(struct token *token, struct statement *stmt)
 {
 	struct symbol_list *syms;
@@ -2230,7 +2245,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
 	e1 = NULL;
 	/* C99 variable declaration? */
 	if (lookup_type(token)) {
-		token = external_declaration(token, NULL, &syms);
+		token = external_declaration(token, add_for_loop_decl, &syms);
 	} else {
 		token = parse_expression(token, &e1);
 		token = expect(token, ';', "in 'for'");
diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index b9db8c9c6..e813b0ae3 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -30,7 +30,6 @@ static int c99(void)
 
 /*
  * check-name: C99 for-loop declarations
- * check-known-to-fail
  *
  * check-error-start
 c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
-- 
2.11.0

Re: [PATCH 1/5] replace test for c99 for-loop initializers

[Ramsay Jones]

On 18/02/17 20:30, Luc Van Oostenryck wrote:
> The existing test is an indirect test, using a warning
> about context imbalance to show that some part of code
> was discarded.
> 
> Now that we have the minimal tools to test the output of
> test-linearize, use them to replace the test by a direct one.

Hmm, it may be a more direct test, but it is not clear
just what is being tested (or indeed how it is being tested).

> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  validation/c99-for-loop.c | 36 ++++++++++++------------------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
> index 42246c513..427fde268 100644
> --- a/validation/c99-for-loop.c
> +++ b/validation/c99-for-loop.c
> @@ -1,33 +1,21 @@
> -int op(int);
> -
> -static int good(void)
> +int c99(void);
> +int c99(void)
>  {
> -	__context__(1);
> -	for (int i = 0; i < 10; i++) {
> -		if (!op(i)) {
> -			__context__(-1);
> -			return 0;
> -		}
> -	}
> -	__context__(-1);
> -	return 1;
> -}
> +	int r = -1;
>  
> -static int bad(void)
> -{
> -	__context__(1);
>  	for (int i = 0; i < 10; i++) {
> -		if (!op(i)) {
> -			__context__(-1);
> -			return 0;
> -		}
> +		r = i;
>  	}
> -	return 1;
> +
> +	return r;
>  }
> +
>  /*
>   * check-name: C99 for loop variable declaration
> + * check-command: test-linearize $file
>   *
> - * check-error-start
> -c99-for-loop.c:16:12: warning: context imbalance in 'bad' - different lock contexts for basic block
> - * check-error-end
> + * check-output-ignore
> + * check-output-contains: phisrc\\.
> + * check-output-contains: phi\\.
> + * check-output-contains: add\\.
>   */
> 

After applying this patch, I edited validation/c99-for-loop.c
like so:

    $ git diff
    diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
    index 427fde2..6b24fa8 100644
    --- a/validation/c99-for-loop.c
    +++ b/validation/c99-for-loop.c
    @@ -2,8 +2,9 @@ int c99(void);
     int c99(void)
     {
            int r = -1;
    +       int i;
 
    -       for (int i = 0; i < 10; i++) {
    +       for (i = 0; i < 10; i++) {
                    r = i;
            }
 
    $ 

This modified test still passes (indeed the output is identical).
:-P

ATB,
Ramsay Jones

Re: [PATCH 1/5] replace test for c99 for-loop initializers

[Luc Van Oostenryck]

On Sat, Feb 18, 2017 at 10:37:36PM +0000, Ramsay Jones wrote:
> 
> 
> On 18/02/17 20:30, Luc Van Oostenryck wrote:
> > The existing test is an indirect test, using a warning
> > about context imbalance to show that some part of code
> > was discarded.
> > 
> > Now that we have the minimal tools to test the output of
> > test-linearize, use them to replace the test by a direct one.
> 
> Hmm, it may be a more direct test, but it is not clear
> just what is being tested (or indeed how it is being tested).
> 
> 
> After applying this patch, I edited validation/c99-for-loop.c
> like so:
> 
>     $ git diff
>     diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
>     index 427fde2..6b24fa8 100644
>     --- a/validation/c99-for-loop.c
>     +++ b/validation/c99-for-loop.c
>     @@ -2,8 +2,9 @@ int c99(void);
>      int c99(void)
>      {
>             int r = -1;
>     +       int i;
>  
>     -       for (int i = 0; i < 10; i++) {
>     +       for (i = 0; i < 10; i++) {
>                     r = i;
>             }
>  
>     $ 
> 
> This modified test still passes (indeed the output is identical).
> :-P
> 
> ATB,
> Ramsay Jones

Which is wonderful because it's exactly what it should be.

In fact I would love to be able to do this: show that two versions
of a program/function give exactly the same output, but the testsuite
can't do that (yet).

Now, you're right that It's may be not very clear what is being tested.
I may make more sense once you replace it in the context of the patch
it replace and the associated fix:
- 0e91f878 ("validation: Check C99 for loop variables")
- ed73fd32 ("linearize: Emit C99 declarations correctly")
If you try the test case on the tree with the patch ed73fd32 reverted,
you will see that the two versions doesn't give anymore the same result:
The C89 version (your) version will still give the right output
but the C99 version (the one of the test case) will essentialy gives
an empty output (the code used to ignore the C99-style declaration
and patch ed73fd32 fixed that).
Be careful also to the fact that this test case depends on testsuite
features only present on sparse-next, not on the master tree.


Luc Van Oostenryck

Re: [PATCH 1/5] replace test for c99 for-loop initializers

[Ramsay Jones]

On 19/02/17 01:10, Luc Van Oostenryck wrote:
> On Sat, Feb 18, 2017 at 10:37:36PM +0000, Ramsay Jones wrote:
>>
>>
>> On 18/02/17 20:30, Luc Van Oostenryck wrote:
>>> The existing test is an indirect test, using a warning
>>> about context imbalance to show that some part of code
>>> was discarded.
>>>
>>> Now that we have the minimal tools to test the output of
>>> test-linearize, use them to replace the test by a direct one.
>>
>> Hmm, it may be a more direct test, but it is not clear
>> just what is being tested (or indeed how it is being tested).
>>
>>
>> After applying this patch, I edited validation/c99-for-loop.c
>> like so:
>>
>>     $ git diff
>>     diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
>>     index 427fde2..6b24fa8 100644
>>     --- a/validation/c99-for-loop.c
>>     +++ b/validation/c99-for-loop.c
>>     @@ -2,8 +2,9 @@ int c99(void);
>>      int c99(void)
>>      {
>>             int r = -1;
>>     +       int i;
>>  
>>     -       for (int i = 0; i < 10; i++) {
>>     +       for (i = 0; i < 10; i++) {
>>                     r = i;
>>             }
>>  
>>     $ 
>>
>> This modified test still passes (indeed the output is identical).
>> :-P
>>
>> ATB,
>> Ramsay Jones
> 
> Which is wonderful because it's exactly what it should be.

So why is it called c99-for-loop.c? ;-) [rhetorical question]

> In fact I would love to be able to do this: show that two versions
> of a program/function give exactly the same output, but the testsuite
> can't do that (yet).

OK

> Now, you're right that It's may be not very clear what is being tested.

Indeed! (So, the commit message could use some improvement, right?)

> I may make more sense once you replace it in the context of the patch
> it replace and the associated fix:
> - 0e91f878 ("validation: Check C99 for loop variables")
> - ed73fd32 ("linearize: Emit C99 declarations correctly")

OK, so that sheds some light on the issue ...

> If you try the test case on the tree with the patch ed73fd32 reverted,
> you will see that the two versions doesn't give anymore the same result:
> The C89 version (your) version will still give the right output
> but the C99 version (the one of the test case) will essentialy gives
> an empty output (the code used to ignore the C99-style declaration
> and patch ed73fd32 fixed that).

Yes, I can confirm this behaviour. If I 'git revert ed73fd32' then
rebuild, sure enough the c99 version is 'essentially empty', but the
c89 version is as before.

So, how could I tell from the commit message alone, that this is what
the test is doing?

ATB,
Ramsay Jones

[PATCH v2 0/5] more validation of C99 for-loop initializers

[Luc Van Oostenryck]

This serie adds some validations of C99-style for-loop
initializers: scope and storage.

Patch 1 replaces the current indirect test by a direct one
Patch 2 adds a test case for the scope
Patch 3 adds some test cases for storage.
Patch 4 is a preparatory patch for 5).
Patch 5 adds missing storage validation


Changes since v1:
- better log message for patch 1, thanks to Ramsay Jones

Luc Van Oostenryck (5):
  replace test for c99 for-loop initializers
  add test case for scope of C99 for-loop declarations
  add test cases for storage of c99 for-loop declarations
  add a method to external_declaration()
  check the storage of C99 for-loop initializers

 lib.c                          |  2 +-
 parse.c                        | 37 ++++++++++++++++++++++++++++++-------
 parse.h                        |  3 ++-
 validation/c99-for-loop-decl.c | 40 ++++++++++++++++++++++++++++++++++++++++
 validation/c99-for-loop.c      | 36 ++++++++++++------------------------
 5 files changed, 85 insertions(+), 33 deletions(-)
 create mode 100644 validation/c99-for-loop-decl.c

-- 
2.11.0

[PATCH v2 1/5] replace test for c99 for-loop initializers

[Luc Van Oostenryck]

This test is to insure that a for-loop with C99-style initializer
linearize correctly: the same as a C89-style one (modulo any effect
on the scope of the variables). For example that code like:
	for (int = 0; i < 10; i++)
		do_stuff(i);
is linearized the same as  code like:
	int i;
	for (i = 0; i < 10; i++)
		do_stuff(i);

A test for this already exist in the testsuite:
	0e91f878 ("validation: Check C99 for loop variables")
which show the correctness of the fix::
	ed73fd32 ("linearize: Emit C99 declarations correctly")
But this test is an indirect one, using the presence or absence of
warning about context imbalance to show that some part of code is
present or not.

Now that we have the minimal tools to test the output of
test-linearize, use them to replace the test by a direct one.

Note: ideally we would like to show that the C89 & the C99 version
generate the same code but the testsuie deosn't allow this (yet).

CC: Ramsay Jones <ramsay@ramsayjones.plus.com>
Test-case-for: ed73fd32 ("linearize: Emit C99 declarations correctly")
Replaces:      0e91f878 ("validation: Check C99 for loop variables")
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
index 42246c513..427fde268 100644
--- a/validation/c99-for-loop.c
+++ b/validation/c99-for-loop.c
@@ -1,33 +1,21 @@
-int op(int);
-
-static int good(void)
+int c99(void);
+int c99(void)
 {
-	__context__(1);
-	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
-	}
-	__context__(-1);
-	return 1;
-}
+	int r = -1;
 
-static int bad(void)
-{
-	__context__(1);
 	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
+		r = i;
 	}
-	return 1;
+
+	return r;
 }
+
 /*
  * check-name: C99 for loop variable declaration
+ * check-command: test-linearize $file
  *
- * check-error-start
-c99-for-loop.c:16:12: warning: context imbalance in 'bad' - different lock contexts for basic block
- * check-error-end
+ * check-output-ignore
+ * check-output-contains: phisrc\\.
+ * check-output-contains: phi\\.
+ * check-output-contains: add\\.
  */
-- 
2.11.0

[PATCH v2 2/5] add test case for scope of C99 for-loop declarations

[Luc Van Oostenryck]

Insure that variable declared inside a C99 for-loop
have their scope restricted to this loop.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 validation/c99-for-loop-decl.c

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
new file mode 100644
index 000000000..c2ceaab99
--- /dev/null
+++ b/validation/c99-for-loop-decl.c
@@ -0,0 +1,18 @@
+static int bad_scope(void)
+{
+	int r = 0;
+
+	for (int i = 0; i < 10; i++) {
+		r = i;
+	}
+
+	return i;			/* check-should-fail */
+}
+
+/*
+ * check-name: C99 for-loop declarations
+ *
+ * check-error-start
+c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
+ * check-error-end
+ */
-- 
2.11.0

[PATCH v2 3/5] add test cases for storage of c99 for-loop declarations

[Luc Van Oostenryck]

Variable declared inside a C99 for-loop must have register
or automatic storage; static & extern storage are invalid.
These test cases verify that we warns if it is not the case.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index c2ceaab99..b9db8c9c6 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -9,10 +9,33 @@ static int bad_scope(void)
 	return i;			/* check-should-fail */
 }
 
+static int c99(void)
+{
+	int r = 0;
+
+	for (         int i = 0; i < 10; i++)	/* check-should-pass */
+		r = i;
+	for (    auto int j = 0; j < 10; j++)	/* check-should-pass */
+		r = j;
+	for (register int k = 0; k < 10; k++)	/* check-should-pass */
+		r = k;
+	for (  extern int l = 0; l < 10; l++)	/* check-should-fail */
+		r = l;
+	for (  extern int m;     m < 10; m++)	/* check-should-fail */
+		r = m;
+	for (  static int n = 0; n < 10; n++)	/* check-should-fail */
+		r = n;
+	return r;
+}
+
 /*
  * check-name: C99 for-loop declarations
+ * check-known-to-fail
  *
  * check-error-start
+c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
+c99-for-loop-decl.c:24:27: error: non-local var 'm' in for-loop initializer
+c99-for-loop-decl.c:26:27: error: non-local var 'n' in for-loop initializer
 c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
  * check-error-end
  */
-- 
2.11.0

[PATCH v2 4/5] add a method to external_declaration()

[Luc Van Oostenryck]

After parsing and validation, the symbols in the declaration
are added to the list given in argument, *if* they are not extern
symbols. The symbols that are extern are them not added to the list.

This is what is needed for usual declarations but ignoring extern
symbols make it impossible to emit a diagnostic in less usual
situation.

This is motivated by the validation of variable declaration inside
a for-loop initializer, which valid in C99 but only for variable
with local storage.

The changes consist in moving the part 'add the symbol to the list if
the symbol is not extern' to a separate function which will be called
by default.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c   |  2 +-
 parse.c | 22 +++++++++++++++-------
 parse.h |  3 ++-
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/lib.c b/lib.c
index d47325243..9badc09b3 100644
--- a/lib.c
+++ b/lib.c
@@ -1080,7 +1080,7 @@ static struct symbol_list *sparse_tokenstream(struct token *token)
 
 	// Parse the resulting C code
 	while (!eof_token(token))
-		token = external_declaration(token, &translation_unit_used_list);
+		token = external_declaration(token, NULL, &translation_unit_used_list);
 	return translation_unit_used_list;
 }
 
diff --git a/parse.c b/parse.c
index a4a126720..866186fd2 100644
--- a/parse.c
+++ b/parse.c
@@ -2230,7 +2230,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
 	e1 = NULL;
 	/* C99 variable declaration? */
 	if (lookup_type(token)) {
-		token = external_declaration(token, &syms);
+		token = external_declaration(token, NULL, &syms);
 	} else {
 		token = parse_expression(token, &e1);
 		token = expect(token, ';', "in 'for'");
@@ -2457,7 +2457,7 @@ static struct token * statement_list(struct token *token, struct statement_list
 				seen_statement = 0;
 			}
 			stmt = alloc_statement(token->pos, STMT_DECLARATION);
-			token = external_declaration(token, &stmt->declaration);
+			token = external_declaration(token, NULL, &stmt->declaration);
 		} else {
 			seen_statement = Wdeclarationafterstatement;
 			token = statement(token, &stmt);
@@ -2785,7 +2785,15 @@ static struct token *toplevel_asm_declaration(struct token *token, struct symbol
 	return token;
 }
 
-struct token *external_declaration(struct token *token, struct symbol_list **list)
+static void add_stmt_decl(struct symbol_list **list, struct symbol *decl)
+{
+	if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
+		add_symbol(list, decl);
+		fn_local_symbol(decl);
+	}
+}
+
+struct token *external_declaration(struct token *token, add_decl_t add_decl, struct symbol_list **list)
 {
 	struct ident *ident = NULL;
 	struct symbol *decl;
@@ -2864,6 +2872,9 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 		sparse_error(token->pos, "void declaration");
 	}
 
+	if (!add_decl)
+		add_decl = add_stmt_decl;
+
 	for (;;) {
 		if (!is_typedef && match_op(token, '=')) {
 			if (decl->ctype.modifiers & MOD_EXTERN) {
@@ -2873,10 +2884,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 			token = initializer(&decl->initializer, token->next);
 		}
 		if (!is_typedef) {
-			if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
-				add_symbol(list, decl);
-				fn_local_symbol(decl);
-			}
+			add_decl(list, decl);
 		}
 		check_declaration(decl);
 		if (decl->same_symbol) {
diff --git a/parse.h b/parse.h
index a2b3e3889..d3199727e 100644
--- a/parse.h
+++ b/parse.h
@@ -129,7 +129,8 @@ extern int show_statement(struct statement *);
 extern void show_statement_list(struct statement_list *, const char *);
 extern int show_expression(struct expression *);
 
-extern struct token *external_declaration(struct token *token, struct symbol_list **list);
+typedef void (*add_decl_t)(struct symbol_list **list, struct symbol *decl);
+extern struct token *external_declaration(struct token *token, add_decl_t add_decl, struct symbol_list **list);
 
 extern struct symbol *ctype_integer(int size, int want_unsigned);
 
-- 
2.11.0

[PATCH v2 5/5] check the storage of C99 for-loop initializers

[Luc Van Oostenryck]

In C99, it is valid to declarare a variable inside
a for-loop initializer but only when the storage is local
(automatic or register).

Until now this was not enforced.

Fix that by adding the appropriate checks called by
external_declaration() via the new function pointer when
parsing this declaration in a for-loop context.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                        | 17 ++++++++++++++++-
 validation/c99-for-loop-decl.c |  1 -
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/parse.c b/parse.c
index 866186fd2..5b029ccdc 100644
--- a/parse.c
+++ b/parse.c
@@ -2217,6 +2217,21 @@ static struct token *parse_return_statement(struct token *token, struct statemen
 	return expression_statement(token->next, &stmt->ret_value);
 }
 
+static void add_for_loop_decl(struct symbol_list **list, struct symbol *sym)
+{
+	unsigned long storage;
+
+	storage = sym->ctype.modifiers & MOD_STORAGE;
+	if (storage & ~(MOD_AUTO | MOD_REGISTER)) {
+		const char *name = show_ident(sym->ident);
+		sparse_error(sym->pos, "non-local var '%s' in for-loop initializer", name);
+		sym->ctype.modifiers &= ~MOD_STORAGE;
+	}
+
+	add_symbol(list, sym);
+	fn_local_symbol(sym);
+}
+
 static struct token *parse_for_statement(struct token *token, struct statement *stmt)
 {
 	struct symbol_list *syms;
@@ -2230,7 +2245,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
 	e1 = NULL;
 	/* C99 variable declaration? */
 	if (lookup_type(token)) {
-		token = external_declaration(token, NULL, &syms);
+		token = external_declaration(token, add_for_loop_decl, &syms);
 	} else {
 		token = parse_expression(token, &e1);
 		token = expect(token, ';', "in 'for'");
diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index b9db8c9c6..e813b0ae3 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -30,7 +30,6 @@ static int c99(void)
 
 /*
  * check-name: C99 for-loop declarations
- * check-known-to-fail
  *
  * check-error-start
 c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
-- 
2.11.0

Re: [PATCH v2 1/5] replace test for c99 for-loop initializers

[Ramsay Jones]

On 20/02/17 07:20, Luc Van Oostenryck wrote:
> This test is to insure that a for-loop with C99-style initializer
> linearize correctly: the same as a C89-style one (modulo any effect
> on the scope of the variables). For example that code like:
> 	for (int = 0; i < 10; i++)
> 		do_stuff(i);
> is linearized the same as  code like:
> 	int i;
> 	for (i = 0; i < 10; i++)
> 		do_stuff(i);
> 
> A test for this already exist in the testsuite:
> 	0e91f878 ("validation: Check C99 for loop variables")
> which show the correctness of the fix::
> 	ed73fd32 ("linearize: Emit C99 declarations correctly")
> But this test is an indirect one, using the presence or absence of
> warning about context imbalance to show that some part of code is
> present or not.
> 
> Now that we have the minimal tools to test the output of
> test-linearize, use them to replace the test by a direct one.
> 
> Note: ideally we would like to show that the C89 & the C99 version
> generate the same code but the testsuie deosn't allow this (yet).
> 
> CC: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Test-case-for: ed73fd32 ("linearize: Emit C99 declarations correctly")
> Replaces:      0e91f878 ("validation: Check C99 for loop variables")
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---

Yep, much better. Thanks!

ATB,
Ramsay Jones

[resent to the mailinglist]

Re: [PATCH 4/5] add a method to external_declaration()

[Christopher Li]

On Sun, Feb 19, 2017 at 4:30 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> After parsing and validation, the symbols in the declaration
> are added to the list given in argument, *if* they are not extern
> symbols. The symbols that are extern are them not added to the list.
>
> This is what is needed for usual declarations but ignoring extern
> symbols make it impossible to emit a diagnostic in less usual
> situation.

I have other situation want to use this as well.

> --- a/parse.h
> +++ b/parse.h
> @@ -129,7 +129,8 @@ extern int show_statement(struct statement *);
>  extern void show_statement_list(struct statement_list *, const char *);
>  extern int show_expression(struct expression *);
>
> -extern struct token *external_declaration(struct token *token, struct symbol_list **list);
> +typedef void (*add_decl_t)(struct symbol_list **list, struct symbol *decl);
> +extern struct token *external_declaration(struct token *token, add_decl_t add_decl, struct symbol_list **list);
>

I think the logic should be, "external_declaration" accept token as input.
For each newly declared symbol, it call to the callbacks function
to receive the symbol. The receive behavior is depend on the callback
function. The default function can be adding the symbol to a list.

So the struct symbol_list **list should turn into transparent argument as
context for the call back.

> --- a/lib.c
> +++ b/lib.c
> @@ -1080,7 +1080,7 @@ static struct symbol_list *sparse_tokenstream(struct token *token)
>
>         // Parse the resulting C code
>         while (!eof_token(token))
> -               token = external_declaration(token, &translation_unit_used_list);
> +               token = external_declaration(token, NULL, &translation_unit_used_list);

I prefer here just provide the default call back which is
add_stmt_decl in your case.

>         return translation_unit_used_list;
>  }
>
> diff --git a/parse.c b/parse.c
> index a4a126720..866186fd2 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -2230,7 +2230,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
>         e1 = NULL;
>         /* C99 variable declaration? */
>         if (lookup_type(token)) {
> -               token = external_declaration(token, &syms);
> +               token = external_declaration(token, NULL, &syms);

Same here.


>         } else {
>                 token = parse_expression(token, &e1);
>                 token = expect(token, ';', "in 'for'");
> @@ -2457,7 +2457,7 @@ static struct token * statement_list(struct token *token, struct statement_list
>                                 seen_statement = 0;
>                         }
>                         stmt = alloc_statement(token->pos, STMT_DECLARATION);
> -                       token = external_declaration(token, &stmt->declaration);
> +                       token = external_declaration(token, NULL, &stmt->declaration);

And here.

> +       if (!add_decl)
> +               add_decl = add_stmt_decl;
> +

We don't need this if we ask the caller to provide the call back.


>         for (;;) {
>                 if (!is_typedef && match_op(token, '=')) {
>                         if (decl->ctype.modifiers & MOD_EXTERN) {
> @@ -2873,10 +2884,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
>                         token = initializer(&decl->initializer, token->next);
>                 }
>                 if (!is_typedef) {
> -                       if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
> -                               add_symbol(list, decl);
> -                               fn_local_symbol(decl);
> -                       }
> +                       add_decl(list, decl);
change this to some thing like:

if (add_decl)
                   add_decl(decl, decl_args);


>                 }
>                 check_declaration(decl);
>                 if (decl->same_symbol) {


Chris

Re: [PATCH 4/5] add a method to external_declaration()

[Luc Van Oostenryck]

On Mon, Feb 27, 2017 at 4:37 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Sun, Feb 19, 2017 at 4:30 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> After parsing and validation, the symbols in the declaration
>> are added to the list given in argument, *if* they are not extern
>> symbols. The symbols that are extern are them not added to the list.
>>
>> This is what is needed for usual declarations but ignoring extern
>> symbols make it impossible to emit a diagnostic in less usual
>> situation.
>
> I have other situation want to use this as well.
>
>> --- a/parse.h
>> +++ b/parse.h
>> @@ -129,7 +129,8 @@ extern int show_statement(struct statement *);
>>  extern void show_statement_list(struct statement_list *, const char *);
>>  extern int show_expression(struct expression *);
>>
>> -extern struct token *external_declaration(struct token *token, struct symbol_list **list);
>> +typedef void (*add_decl_t)(struct symbol_list **list, struct symbol *decl);
>> +extern struct token *external_declaration(struct token *token, add_decl_t add_decl, struct symbol_list **list);
>>
>
> I think the logic should be, "external_declaration" accept token as input.
> For each newly declared symbol, it call to the callbacks function
> to receive the symbol. The receive behavior is depend on the callback
> function. The default function can be adding the symbol to a list.
>
> So the struct symbol_list **list should turn into transparent argument as
> context for the call back.

Yes, it can certainly be more general.

By 'transparent' you mean a void pointer?

For the two case currently concerned the callback argument can be a
statement pointer
because the two symbol list belong to a statement and I fact I
hesitated to use it or the list.
I choose the smallest change.

If for the other situation you talked here above, we can also use a
statement pointer,
I would prefrer to do so instead than using a void pointer but well it
doesn't matter much
anyway.


>> --- a/lib.c
>> +++ b/lib.c
>> @@ -1080,7 +1080,7 @@ static struct symbol_list *sparse_tokenstream(struct token *token)
>>
>>         // Parse the resulting C code
>>         while (!eof_token(token))
>> -               token = external_declaration(token, &translation_unit_used_list);
>> +               token = external_declaration(token, NULL, &translation_unit_used_list);
>
> I prefer here just provide the default call back which is
> add_stmt_decl in your case.

Sure, I can do this.


Luc

Re: [PATCH 4/5] add a method to external_declaration()

[Luc Van Oostenryck]

On Mon, Feb 27, 2017 at 11:37:09PM +0800, Christopher Li wrote:
> On Sun, Feb 19, 2017 at 4:30 AM, Luc Van Oostenryck wrote:
> >
> > This is what is needed for usual declarations but ignoring extern
> > symbols make it impossible to emit a diagnostic in less usual
> > situation.
> 
> I have other situation want to use this as well.

...

> So the struct symbol_list **list should turn into transparent argument as
> context for the call back.
> 

...

> I prefer here just provide the default call back which is
> add_stmt_decl in your case.

OK. I've made the changes related to this and also prepared
for something more generic (like the name of the method).

I've left the arg type as 'symbol_list **' for the moment though,
it will be easy enough to change when/if needed.

Luc 

[PATCH v3 0/7] more validation of C99 for-loop initializers

[Luc Van Oostenryck]

This serie adds scope & storage validations of C99-style for-loop
initializers.

Patch 1 replaces the current indirect test by a direct one
Patch 2-3 add test cases for the scope & storage
Patch 4-5 add missing storage validation
Patch 6-7 move some checks into default_process_decl().

Changes since v1:
- better log message for patch 1, thanks to Ramsay Jones

Changes since v2:
- patches 1-3 are unchanged.
- do not use 'NULL' for the default method
- limit changes to parse.c, leaving external_declaration() untouched
- use a more generic and exact name for the method
- move a check to default_process_decl()


Luc Van Oostenryck (7):
  replace test for c99 for-loop initializers
  add test case for scope of C99 for-loop declarations
  add test cases for storage of c99 for-loop declarations
  add a method to external_declaration()
  check the storage of C99 for-loop initializers
  make process_decl() aware of the presence of an initializer
  move check extern with initializer to default_process_decl()

 parse.c                        | 54 ++++++++++++++++++++++++++++++++----------
 validation/c99-for-loop-decl.c | 40 +++++++++++++++++++++++++++++++
 validation/c99-for-loop.c      | 36 ++++++++++------------------
 3 files changed, 94 insertions(+), 36 deletions(-)
 create mode 100644 validation/c99-for-loop-decl.c

-- 
2.11.1

[PATCH v3 1/7] replace test for c99 for-loop initializers

[Luc Van Oostenryck]

This test is to insure that a for-loop with C99-style initializer
linearize correctly: the same as a C89-style one (modulo any effect
on the scope of the variables). For example that code like:
	for (int = 0; i < 10; i++)
		do_stuff(i);
is linearized the same as  code like:
	int i;
	for (i = 0; i < 10; i++)
		do_stuff(i);

A test for this already exist in the testsuite:
	0e91f878 ("validation: Check C99 for loop variables")
which show the correctness of the fix::
	ed73fd32 ("linearize: Emit C99 declarations correctly")
But this test is an indirect one, using the presence or absence of
warning about context imbalance to show that some part of code is
present or not.

Now that we have the minimal tools to test the output of
test-linearize, use them to replace the test by a direct one.

Note: ideally we would like to show that the C89 & the C99 version
generate the same code but the testsuie deosn't allow this (yet).

Test-case-for: ed73fd32 ("linearize: Emit C99 declarations correctly")
Replaces:      0e91f878 ("validation: Check C99 for loop variables")
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
index 42246c513..427fde268 100644
--- a/validation/c99-for-loop.c
+++ b/validation/c99-for-loop.c
@@ -1,33 +1,21 @@
-int op(int);
-
-static int good(void)
+int c99(void);
+int c99(void)
 {
-	__context__(1);
-	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
-	}
-	__context__(-1);
-	return 1;
-}
+	int r = -1;
 
-static int bad(void)
-{
-	__context__(1);
 	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
+		r = i;
 	}
-	return 1;
+
+	return r;
 }
+
 /*
  * check-name: C99 for loop variable declaration
+ * check-command: test-linearize $file
  *
- * check-error-start
-c99-for-loop.c:16:12: warning: context imbalance in 'bad' - different lock contexts for basic block
- * check-error-end
+ * check-output-ignore
+ * check-output-contains: phisrc\\.
+ * check-output-contains: phi\\.
+ * check-output-contains: add\\.
  */
-- 
2.11.1

[PATCH v3 2/7] add test case for scope of C99 for-loop declarations

[Luc Van Oostenryck]

Insure that variable declared inside a C99 for-loop
have their scope restricted to this loop.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 validation/c99-for-loop-decl.c

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
new file mode 100644
index 000000000..c2ceaab99
--- /dev/null
+++ b/validation/c99-for-loop-decl.c
@@ -0,0 +1,18 @@
+static int bad_scope(void)
+{
+	int r = 0;
+
+	for (int i = 0; i < 10; i++) {
+		r = i;
+	}
+
+	return i;			/* check-should-fail */
+}
+
+/*
+ * check-name: C99 for-loop declarations
+ *
+ * check-error-start
+c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
+ * check-error-end
+ */
-- 
2.11.1

[PATCH v3 3/7] add test cases for storage of c99 for-loop declarations

[Luc Van Oostenryck]

Variable declared inside a C99 for-loop must have register
or automatic storage; static & extern storage are invalid.
These test cases verify that we warns if it is not the case.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index c2ceaab99..b9db8c9c6 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -9,10 +9,33 @@ static int bad_scope(void)
 	return i;			/* check-should-fail */
 }
 
+static int c99(void)
+{
+	int r = 0;
+
+	for (         int i = 0; i < 10; i++)	/* check-should-pass */
+		r = i;
+	for (    auto int j = 0; j < 10; j++)	/* check-should-pass */
+		r = j;
+	for (register int k = 0; k < 10; k++)	/* check-should-pass */
+		r = k;
+	for (  extern int l = 0; l < 10; l++)	/* check-should-fail */
+		r = l;
+	for (  extern int m;     m < 10; m++)	/* check-should-fail */
+		r = m;
+	for (  static int n = 0; n < 10; n++)	/* check-should-fail */
+		r = n;
+	return r;
+}
+
 /*
  * check-name: C99 for-loop declarations
+ * check-known-to-fail
  *
  * check-error-start
+c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
+c99-for-loop-decl.c:24:27: error: non-local var 'm' in for-loop initializer
+c99-for-loop-decl.c:26:27: error: non-local var 'n' in for-loop initializer
 c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
  * check-error-end
  */
-- 
2.11.1

[PATCH v3 4/7] add a method to external_declaration()

[Luc Van Oostenryck]

After parsing and validation, the symbols in the declaration
are added to the list given in argument, *if* they are not extern
symbols. The symbols that are extern are them not added to the list.

This is what is needed for usual declarations but ignoring extern
symbols make it impossible to emit a diagnostic in less usual
situation.

This is motivated by the validation of variable declaration inside
a for-loop initializer, which is valid in C99 but only for variable
with local storage.

The changes are made up of:
- extract the part 'add local symbols to the list' to a separate
  function: default_process_decl() as preparatory step to make
- replace the part 'add local symbols to the list' by a call
  to a new function pointer given in argument,

Also, to make the change non-invasive for others files:
- rename 'external_declaration()' into 'external_decl()'
- make 'external_declaration()' a small helper calling
  'external_decl()' with 'default_process_decl()' as the method.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/parse.c b/parse.c
index d07b27a21..b9b8d1ae0 100644
--- a/parse.c
+++ b/parse.c
@@ -48,6 +48,9 @@ static struct symbol_list **function_symbol_list;
 struct symbol_list *function_computed_target_list;
 struct statement_list *function_computed_goto_list;
 
+typedef void (*process_decl_t)(struct symbol_list **list, struct symbol *decl);
+static struct token *external_decl(struct token *, process_decl_t, struct symbol_list **);
+
 static struct token *statement(struct token *token, struct statement **tree);
 static struct token *handle_attributes(struct token *token, struct decl_state *ctx, unsigned int keywords);
 
@@ -2797,7 +2800,8 @@ static struct token *toplevel_asm_declaration(struct token *token, struct symbol
 	return token;
 }
 
-struct token *external_declaration(struct token *token, struct symbol_list **list)
+static struct token *external_decl(struct token *token, process_decl_t process_decl,
+		struct symbol_list **list)
 {
 	struct ident *ident = NULL;
 	struct symbol *decl;
@@ -2884,12 +2888,8 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 			}
 			token = initializer(&decl->initializer, token->next);
 		}
-		if (!is_typedef) {
-			if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
-				add_symbol(list, decl);
-				fn_local_symbol(decl);
-			}
-		}
+		if (!is_typedef)
+			process_decl(list, decl);
 		check_declaration(decl);
 		if (decl->same_symbol) {
 			decl->definition = decl->same_symbol->definition;
@@ -2929,3 +2929,16 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 	}
 	return expect(token, ';', "at end of declaration");
 }
+
+static void default_process_decl(struct symbol_list **list, struct symbol *decl)
+{
+	if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
+		add_symbol(list, decl);
+		fn_local_symbol(decl);
+	}
+}
+
+struct token *external_declaration(struct token *token, struct symbol_list **list)
+{
+	return external_decl(token, default_process_decl, list);
+}
-- 
2.11.1

[PATCH v3 5/7] check the storage of C99 for-loop initializers

[Luc Van Oostenryck]

In C99, it is valid to declare a variable inside a
for-loop initializer but only when the storage is local
(automatic or register).
Until now this was not enforced.

Fix this by replacing, when parsing declarations in a for-loop
context, the method external_decl::default_process_decl()
by a new one containing the appropriate checks.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                        | 17 ++++++++++++++++-
 validation/c99-for-loop-decl.c |  1 -
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/parse.c b/parse.c
index b9b8d1ae0..693bb497f 100644
--- a/parse.c
+++ b/parse.c
@@ -2232,6 +2232,21 @@ static struct token *parse_return_statement(struct token *token, struct statemen
 	return expression_statement(token->next, &stmt->ret_value);
 }
 
+static void process_for_loop_decl(struct symbol_list **list, struct symbol *sym)
+{
+	unsigned long storage;
+
+	storage = sym->ctype.modifiers & MOD_STORAGE;
+	if (storage & ~(MOD_AUTO | MOD_REGISTER)) {
+		const char *name = show_ident(sym->ident);
+		sparse_error(sym->pos, "non-local var '%s' in for-loop initializer", name);
+		sym->ctype.modifiers &= ~MOD_STORAGE;
+	}
+
+	add_symbol(list, sym);
+	fn_local_symbol(sym);
+}
+
 static struct token *parse_for_statement(struct token *token, struct statement *stmt)
 {
 	struct symbol_list *syms;
@@ -2245,7 +2260,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
 	e1 = NULL;
 	/* C99 variable declaration? */
 	if (lookup_type(token)) {
-		token = external_declaration(token, &syms);
+		token = external_decl(token, process_for_loop_decl, &syms);
 	} else {
 		token = parse_expression(token, &e1);
 		token = expect(token, ';', "in 'for'");
diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index b9db8c9c6..e813b0ae3 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -30,7 +30,6 @@ static int c99(void)
 
 /*
  * check-name: C99 for-loop declarations
- * check-known-to-fail
  *
  * check-error-start
 c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
-- 
2.11.1

[PATCH v3 6/7] make process_decl() aware of the presence of an initializer

[Luc Van Oostenryck]

The call to the method external_decl::process_decl() is
preceded by some processing and validation if there is also
an initializer.

Make the process_decl() method aware of the presence of such
an initializer in order to be able to make appropriate
validations.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/parse.c b/parse.c
index 693bb497f..c3c877b10 100644
--- a/parse.c
+++ b/parse.c
@@ -48,7 +48,7 @@ static struct symbol_list **function_symbol_list;
 struct symbol_list *function_computed_target_list;
 struct statement_list *function_computed_goto_list;
 
-typedef void (*process_decl_t)(struct symbol_list **list, struct symbol *decl);
+typedef void (*process_decl_t)(struct symbol_list **list, struct symbol *decl, int has_init);
 static struct token *external_decl(struct token *, process_decl_t, struct symbol_list **);
 
 static struct token *statement(struct token *token, struct statement **tree);
@@ -2232,7 +2232,7 @@ static struct token *parse_return_statement(struct token *token, struct statemen
 	return expression_statement(token->next, &stmt->ret_value);
 }
 
-static void process_for_loop_decl(struct symbol_list **list, struct symbol *sym)
+static void process_for_loop_decl(struct symbol_list **list, struct symbol *sym, int has_init)
 {
 	unsigned long storage;
 
@@ -2896,7 +2896,9 @@ static struct token *external_decl(struct token *token, process_decl_t process_d
 	}
 
 	for (;;) {
+		int has_init = 0;
 		if (!is_typedef && match_op(token, '=')) {
+			has_init = 1;
 			if (decl->ctype.modifiers & MOD_EXTERN) {
 				warning(decl->pos, "symbol with external linkage has initializer");
 				decl->ctype.modifiers &= ~MOD_EXTERN;
@@ -2904,7 +2906,7 @@ static struct token *external_decl(struct token *token, process_decl_t process_d
 			token = initializer(&decl->initializer, token->next);
 		}
 		if (!is_typedef)
-			process_decl(list, decl);
+			process_decl(list, decl, has_init);
 		check_declaration(decl);
 		if (decl->same_symbol) {
 			decl->definition = decl->same_symbol->definition;
@@ -2945,7 +2947,7 @@ static struct token *external_decl(struct token *token, process_decl_t process_d
 	return expect(token, ';', "at end of declaration");
 }
 
-static void default_process_decl(struct symbol_list **list, struct symbol *decl)
+static void default_process_decl(struct symbol_list **list, struct symbol *decl, int has_init)
 {
 	if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
 		add_symbol(list, decl);
-- 
2.11.1

[PATCH v3 7/7] move check extern with initializer to default_process_decl()

[Luc Van Oostenryck]

The call to the method external_decl::process_decl() is
preceded by some processing if there is also an initializer.
Part of this processing is a check if the declaration has not
external linkage, otherwise an error is issued and the
'extern' is removed from the declaration.

While also valid for C99 for-loop initializer, this is less
desirable because in this context, 'extern' is invalid anyway
and removing it from the declaration make it imposible to issue
a diagnostic about it.

Fix this by moving the 'extern with initializer' check into the
default_process_decl() method, where it is always pertinent and
so allowing process_for_loop_decl() to make its own diagnostic.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                        | 8 ++++----
 validation/c99-for-loop-decl.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/parse.c b/parse.c
index c3c877b10..e33e8b32f 100644
--- a/parse.c
+++ b/parse.c
@@ -2899,10 +2899,6 @@ static struct token *external_decl(struct token *token, process_decl_t process_d
 		int has_init = 0;
 		if (!is_typedef && match_op(token, '=')) {
 			has_init = 1;
-			if (decl->ctype.modifiers & MOD_EXTERN) {
-				warning(decl->pos, "symbol with external linkage has initializer");
-				decl->ctype.modifiers &= ~MOD_EXTERN;
-			}
 			token = initializer(&decl->initializer, token->next);
 		}
 		if (!is_typedef)
@@ -2949,6 +2945,10 @@ static struct token *external_decl(struct token *token, process_decl_t process_d
 
 static void default_process_decl(struct symbol_list **list, struct symbol *decl, int has_init)
 {
+	if (has_init && decl->ctype.modifiers & MOD_EXTERN) {
+		warning(decl->pos, "symbol with external linkage has initializer");
+		decl->ctype.modifiers &= ~MOD_EXTERN;
+	}
 	if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
 		add_symbol(list, decl);
 		fn_local_symbol(decl);
diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index e813b0ae3..d382d3c9b 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -32,7 +32,7 @@ static int c99(void)
  * check-name: C99 for-loop declarations
  *
  * check-error-start
-c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
+c99-for-loop-decl.c:22:27: error: non-local var 'l' in for-loop initializer
 c99-for-loop-decl.c:24:27: error: non-local var 'm' in for-loop initializer
 c99-for-loop-decl.c:26:27: error: non-local var 'n' in for-loop initializer
 c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
-- 
2.11.1

Re: [PATCH v3 0/7] more validation of C99 for-loop initializers

[Christopher Li]

On Tue, Feb 28, 2017 at 6:03 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> This serie adds scope & storage validations of C99-style for-loop
> initializers.

Just a heads up I am traveling in the next few days. My time and internet access
is unpredictable. I will try to apply them if I got a chance.  In the
worse case, I will
work on it on Sunday.

Chris

Re: [PATCH v3 0/7] more validation of C99 for-loop initializers

[Luc Van Oostenryck]

On Tue, Feb 28, 2017 at 5:34 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Tue, Feb 28, 2017 at 6:03 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> This serie adds scope & storage validations of C99-style for-loop
>> initializers.
>
> Just a heads up I am traveling in the next few days. My time and internet access
> is unpredictable. I will try to apply them if I got a chance.  In the
> worse case, I will
> work on it on Sunday.
>
> Chris

OK. There is no urgency anyway but thanks for letting known.
Have a nice travel.

Re: [PATCH v3 4/7] add a method to external_declaration()

[Christopher Li]

On Tue, Feb 28, 2017 at 6:04 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> The changes are made up of:
> - extract the part 'add local symbols to the list' to a separate
>   function: default_process_decl() as preparatory step to make
> - replace the part 'add local symbols to the list' by a call
>   to a new function pointer given in argument,
>
> Also, to make the change non-invasive for others files:
> - rename 'external_declaration()' into 'external_decl()'
> - make 'external_declaration()' a small helper calling
>   'external_decl()' with 'default_process_decl()' as the method.

I will apply this patch in the sparse-next with some minor comment.
Mostly naming the function.

> ---
>  parse.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/parse.c b/parse.c
> index d07b27a21..b9b8d1ae0 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -48,6 +48,9 @@ static struct symbol_list **function_symbol_list;
>  struct symbol_list *function_computed_target_list;
>  struct statement_list *function_computed_goto_list;
>
> +typedef void (*process_decl_t)(struct symbol_list **list, struct symbol *decl);
> +static struct token *external_decl(struct token *, process_decl_t, struct symbol_list **);
> +
>  static struct token *statement(struct token *token, struct statement **tree);
>  static struct token *handle_attributes(struct token *token, struct decl_state *ctx, unsigned int keywords);
>
> @@ -2797,7 +2800,8 @@ static struct token *toplevel_asm_declaration(struct token *token, struct symbol
>         return token;
>  }
>
> -struct token *external_declaration(struct token *token, struct symbol_list **list)
> +static struct token *external_decl(struct token *token, process_decl_t process_decl,
> +               struct symbol_list **list)

I am fine with the code logic change. However, I wish function has better names.
It just looking at the function name, "external_decl" vs "external_declaration",
it is hard to guess which function is lower level than the other.

> -               }
> +               if (!is_typedef)
> +                       process_decl(list, decl);

I think here better check the process_decl is not NULL.
The caller might issue NULL call back which means it is not
interested in adding the symbol to a list.
> +
> +static void default_process_decl(struct symbol_list **list, struct symbol *decl)
> +{
> +       if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
> +               add_symbol(list, decl);
> +               fn_local_symbol(decl);
> +       }

Again, if I look at just pure function name "default_process_decl", it is
very hard to me to guess what should happen in this function.
After all, "process a declare" is very generic, it can mean any thing.

I suggest a more meaningful name related what the code does.
e.g. "track_local_declaration". I don't know, I want to find a better name.
You are welcome to suggest a better one.

> +}
> +
> +struct token *external_declaration(struct token *token, struct symbol_list **list)
> +{
> +       return external_decl(token, default_process_decl, list);
> +}

If I don't have a better function name for external_decl, I might just not use
the wrapper function at all.

Chris

Re: [PATCH v3 5/7] check the storage of C99 for-loop initializers

[Christopher Li]

On Tue, Feb 28, 2017 at 6:04 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> In C99, it is valid to declare a variable inside a
> for-loop initializer but only when the storage is local
> (automatic or register).
> Until now this was not enforced.
>

Will apply in sparse-next with some comment.

>
> +static void process_for_loop_decl(struct symbol_list **list, struct symbol *sym)
> +{
> +       unsigned long storage;
> +
> +       storage = sym->ctype.modifiers & MOD_STORAGE;
> +       if (storage & ~(MOD_AUTO | MOD_REGISTER)) {
> +               const char *name = show_ident(sym->ident);
> +               sparse_error(sym->pos, "non-local var '%s' in for-loop initializer", name);
> +               sym->ctype.modifiers &= ~MOD_STORAGE;
> +       }
> +
> +       add_symbol(list, sym);
> +       fn_local_symbol(sym);

The more I look at it, the more I feel that the above two lines
should kind of appear in every process_*_declar function.

So may be we should just change your patch 4/7 into:

               if (!is_typedef) {

+                     if (process_decl)
+                                process_decl(sym);

                       if (!(decl->ctype.modifiers & (MOD_EXTERN |
MOD_INLINE))) {
                               add_symbol(list, decl);
                               fn_local_symbol(decl);
                       }
               }

Then for the default case, just pass NULL as process_decl. Might want
to find a better name for this function callback arguments as well.

Because you are actually not interested in change the behavor of adding
to the list at all. You just want to get an notification when the symbol does
get added into the list. Now thing of it, I can't come up with one example
process_xxx_decl not add the symbol nor doing the fn_local_symbol.

Chris

Re: [PATCH v3 6/7] make process_decl() aware of the presence of an initializer

[Christopher Li]

On Tue, Feb 28, 2017 at 6:04 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:


> @@ -2896,7 +2896,9 @@ static struct token *external_decl(struct token *token, process_decl_t process_d
>         }
>
>         for (;;) {
> +               int has_init = 0;
>                 if (!is_typedef && match_op(token, '=')) {
> +                       has_init = 1;

This can be written as:

int has_init = !is_typedef && match_op(token, '=');
if (has_init) {
     ....

>                         if (decl->ctype.modifiers & MOD_EXTERN) {
>                                 warning(decl->pos, "symbol with external linkage has initializer");
>                                 decl->ctype.modifiers &= ~MOD_EXTERN;
> @@ -2904,7 +2906,7 @@ static struct token *external_decl(struct token *token, process_decl_t process_d
>                         token = initializer(&decl->initializer, token->next);
>                 }
>                 if (!is_typedef)
> -                       process_decl(list, decl);
> +                       process_decl(list, decl, has_init);

My comment regarding patch 5/7 still stands valid here. If you keep the common
logic here, move the code from "default_process_decl" body back to here.
You can avoid passing has_init into process_decl at all.

Passing the "has_init" into a call back function make the code hard to
read because the logic has separated into two function. At the same time
process_for_loop_decl does not issue this warning at all, I think it should.

I will apply this patch for sparse-next, I agree the warning is useful
behavior. I am also expecting a follow up patch.

Chris

Re: [PATCH v3 4/7] add a method to external_declaration()

[Luc Van Oostenryck]

On Sun, Mar 05, 2017 at 10:04:01PM +0800, Christopher Li wrote:
> I am fine with the code logic change. However, I wish function has better names.
> It just looking at the function name, "external_decl" vs "external_declaration",
> it is hard to guess which function is lower level than the other.

Sure. I didn't liked it myself but at the moment I didn't got something
better. I'll try harder.
 
> > -               }
> > +               if (!is_typedef)
> > +                       process_decl(list, decl);
> 
> I think here better check the process_decl is not NULL.
> The caller might issue NULL call back which means it is not
> interested in adding the symbol to a list.

Yes, we can and it's cheap but I think we'll always need a
callback here. And even if we don't, it's easy enough to provide
a callback that do nothing which avoid to always have to test the
pointer here to handle an hypothetical corner case.

> > +static void default_process_decl(struct symbol_list **list, struct symbol *decl)
> > +{
> > +       if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
> > +               add_symbol(list, decl);
> > +               fn_local_symbol(decl);
> > +       }
> 
> Again, if I look at just pure function name "default_process_decl", it is
> very hard to me to guess what should happen in this function.
> After all, "process a declare" is very generic, it can mean any thing.

Yes, sure, but the keyword here is 'default' like in "don't care what this
is doing, it's the that's correct for most/normal situations".

> I suggest a more meaningful name related what the code does.
> e.g. "track_local_declaration". I don't know, I want to find a better name.
> You are welcome to suggest a better one.

I'll look for one.

> > +}
> > +
> > +struct token *external_declaration(struct token *token, struct symbol_list **list)
> > +{
> > +       return external_decl(token, default_process_decl, list);
> > +}
> 
> If I don't have a better function name for external_decl, I might just not use
> the wrapper function at all.

Yes, OK.
And to be honest, the current name 'external_declaration()' is far
from being a good name eithers: it doesn't give a hint on *what*
the function is doing and it's not only for 'extern' declaration.

Luc

Re: [PATCH v3 5/7] check the storage of C99 for-loop initializers

[Luc Van Oostenryck]

On Sun, Mar 05, 2017 at 10:26:53PM +0800, Christopher Li wrote:
> >
> > +static void process_for_loop_decl(struct symbol_list **list, struct symbol *sym)
> > +{
> > +       ...
> > +       add_symbol(list, sym);
> > +       fn_local_symbol(sym);
> 
> The more I look at it, the more I feel that the above two lines
> should kind of appear in every process_*_declar function.

I agree and it certainly does for the current two cases.
And it's why I've kept the symbol_list as type.
But I understood you thought differently in your previous
comment:
	"The default function can be adding the symbol to a list.
	So the struct symbol_list **list should turn into transparent
	argument as context for the call back."

> So may be we should just change your patch 4/7 into:
> 
>                if (!is_typedef) {
> 
> +                     if (process_decl)
> +                                process_decl(sym);
> 
>                        if (!(decl->ctype.modifiers & (MOD_EXTERN |
> MOD_INLINE))) {
>                                add_symbol(list, decl);
>                                fn_local_symbol(decl);
>                        }
>                }
> 
> Then for the default case, just pass NULL as process_decl. Might want
> to find a better name for this function callback arguments as well.
> 
> Because you are actually not interested in change the behavor of adding
> to the list at all. You just want to get an notification when the symbol does
> get added into the list. Now thing of it, I can't come up with one example
> process_xxx_decl not add the symbol nor doing the fn_local_symbol.

Yes.
I almost came with a solution with an ops struct with two operations:
- one that do some checking
- another that do the 'action' like adding to the list
But then it wasn't needed for the default & c99-for-loop cases here.

I'll see what can be done.

Luc

Re: [PATCH v3 6/7] make process_decl() aware of the presence of an initializer

[Luc Van Oostenryck]

On Sun, Mar 05, 2017 at 10:49:37PM +0800, Christopher Li wrote:
> This can be written as:
> 
> int has_init = !is_typedef && match_op(token, '=');
> if (has_init) {

Yes, it can.
But honestly I absolutely detest this 'has_init'.

>      ....
> 
> Passing the "has_init" into a call back function make the code hard to
> read because the logic has separated into two function. At the same time
> process_for_loop_decl does not issue this warning at all, I think it should.

Since I just realize that this 'has_init' is not needed as we can
simply test the presence of decl->initializer, I'll remove it.
 
> I will apply this patch for sparse-next, I agree the warning is useful
> behavior. I am also expecting a follow up patch.

Yes.
Don't bother to add it to sparse-next, I'll send another version later.


Luc

[PATCH v4 0/6] more validation of C99 for-loop initializers

[Luc Van Oostenryck]

This serie adds scope & storage validations of C99-style for-loop
initializers.

Patch 1 replaces the current indirect test by a direct one
Patch 2-3 add test cases for the scope & storage
Patch 4-5 add missing storage validation
Patch 6 reorder default validation with the c99-for-loop one

Changes since v1:
- better log message for patch 1, thanks to Ramsay Jones

Changes since v2:
- patches 1-3 are unchanged.
- do not use 'NULL' for the default method
- limit changes to parse.c, leaving external_declaration() untouched
- use a more generic and exact name for the method
- move a check to default_process_decl()

Changes since v3:
- patches 1-3 are unchanged.
- focus the role of the method on validation
- use again 'NULL', meaning that no additional validation is needed
- remove the wrapper as it's not not needed anymore
- get around with the poor naming


Luc Van Oostenryck (6):
  replace test for c99 for-loop initializers
  add test case for scope of C99 for-loop declarations
  add test cases for storage of c99 for-loop declarations
  add an optional validation method to external_declaration()
  check the storage of C99 for-loop initializers
  move 'extern with initializer' validation after the validate method

 lib.c                          |  2 +-
 parse.c                        | 30 +++++++++++++++++++++++-------
 parse.h                        |  3 ++-
 validation/c99-for-loop-decl.c | 40 ++++++++++++++++++++++++++++++++++++++++
 validation/c99-for-loop.c      | 36 ++++++++++++------------------------
 5 files changed, 78 insertions(+), 33 deletions(-)
 create mode 100644 validation/c99-for-loop-decl.c

-- 
2.11.1

[PATCH v4 1/6] replace test for c99 for-loop initializers

[Luc Van Oostenryck]

This test is to insure that a for-loop with C99-style initializer
linearize correctly: the same as a C89-style one (modulo any effect
on the scope of the variables). For example that code like:
	for (int = 0; i < 10; i++)
		do_stuff(i);
is linearized the same as  code like:
	int i;
	for (i = 0; i < 10; i++)
		do_stuff(i);

A test for this already exist in the testsuite:
	0e91f878 ("validation: Check C99 for loop variables")
which show the correctness of the fix::
	ed73fd32 ("linearize: Emit C99 declarations correctly")
But this test is an indirect one, using the presence or absence of
warning about context imbalance to show that some part of code is
present or not.

Now that we have the minimal tools to test the output of
test-linearize, use them to replace the test by a direct one.

Note: ideally we would like to show that the C89 & the C99 version
generate the same code but the testsuie deosn't allow this (yet).

Test-case-for: ed73fd32 ("linearize: Emit C99 declarations correctly")
Replaces:      0e91f878 ("validation: Check C99 for loop variables")
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/validation/c99-for-loop.c b/validation/c99-for-loop.c
index 42246c513..427fde268 100644
--- a/validation/c99-for-loop.c
+++ b/validation/c99-for-loop.c
@@ -1,33 +1,21 @@
-int op(int);
-
-static int good(void)
+int c99(void);
+int c99(void)
 {
-	__context__(1);
-	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
-	}
-	__context__(-1);
-	return 1;
-}
+	int r = -1;
 
-static int bad(void)
-{
-	__context__(1);
 	for (int i = 0; i < 10; i++) {
-		if (!op(i)) {
-			__context__(-1);
-			return 0;
-		}
+		r = i;
 	}
-	return 1;
+
+	return r;
 }
+
 /*
  * check-name: C99 for loop variable declaration
+ * check-command: test-linearize $file
  *
- * check-error-start
-c99-for-loop.c:16:12: warning: context imbalance in 'bad' - different lock contexts for basic block
- * check-error-end
+ * check-output-ignore
+ * check-output-contains: phisrc\\.
+ * check-output-contains: phi\\.
+ * check-output-contains: add\\.
  */
-- 
2.11.1

[PATCH v4 2/6] add test case for scope of C99 for-loop declarations

[Luc Van Oostenryck]

Insure that variable declared inside a C99 for-loop
have their scope restricted to this loop.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 validation/c99-for-loop-decl.c

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
new file mode 100644
index 000000000..c2ceaab99
--- /dev/null
+++ b/validation/c99-for-loop-decl.c
@@ -0,0 +1,18 @@
+static int bad_scope(void)
+{
+	int r = 0;
+
+	for (int i = 0; i < 10; i++) {
+		r = i;
+	}
+
+	return i;			/* check-should-fail */
+}
+
+/*
+ * check-name: C99 for-loop declarations
+ *
+ * check-error-start
+c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
+ * check-error-end
+ */
-- 
2.11.1

[PATCH v4 3/6] add test cases for storage of c99 for-loop declarations

[Luc Van Oostenryck]

Variable declared inside a C99 for-loop must have register
or automatic storage; static & extern storage are invalid.
These test cases verify that we warns if it is not the case.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/c99-for-loop-decl.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index c2ceaab99..b9db8c9c6 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -9,10 +9,33 @@ static int bad_scope(void)
 	return i;			/* check-should-fail */
 }
 
+static int c99(void)
+{
+	int r = 0;
+
+	for (         int i = 0; i < 10; i++)	/* check-should-pass */
+		r = i;
+	for (    auto int j = 0; j < 10; j++)	/* check-should-pass */
+		r = j;
+	for (register int k = 0; k < 10; k++)	/* check-should-pass */
+		r = k;
+	for (  extern int l = 0; l < 10; l++)	/* check-should-fail */
+		r = l;
+	for (  extern int m;     m < 10; m++)	/* check-should-fail */
+		r = m;
+	for (  static int n = 0; n < 10; n++)	/* check-should-fail */
+		r = n;
+	return r;
+}
+
 /*
  * check-name: C99 for-loop declarations
+ * check-known-to-fail
  *
  * check-error-start
+c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
+c99-for-loop-decl.c:24:27: error: non-local var 'm' in for-loop initializer
+c99-for-loop-decl.c:26:27: error: non-local var 'n' in for-loop initializer
 c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
  * check-error-end
  */
-- 
2.11.1

[PATCH v4 4/6] add an optional validation method to external_declaration()

[Luc Van Oostenryck]

After parsing and validation, the symbols in the declaration
are added to the list given in argument, *if* they are not extern
symbols. The symbols that are extern are them not added to the list.

This is what is needed for usual declarations but ignoring extern
symbols make it impossible to emit a diagnostic in less usual
situation.

This is motivated by the validation of variable declaration inside
a for-loop initializer, which is valid in C99 but only for variable
with local storage.

The change consists in adding to external_declaration() an optional
callback 'validate_decl()' which, if present (non-null), is called
just before adding the declaration to the list.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c   |  2 +-
 parse.c | 10 +++++++---
 parse.h |  3 ++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/lib.c b/lib.c
index d76b8368e..272d2c88a 100644
--- a/lib.c
+++ b/lib.c
@@ -1085,7 +1085,7 @@ static struct symbol_list *sparse_tokenstream(struct token *token)
 
 	// Parse the resulting C code
 	while (!eof_token(token))
-		token = external_declaration(token, &translation_unit_used_list);
+		token = external_declaration(token, &translation_unit_used_list, NULL);
 	return translation_unit_used_list;
 }
 
diff --git a/parse.c b/parse.c
index d07b27a21..d91a4bced 100644
--- a/parse.c
+++ b/parse.c
@@ -2242,7 +2242,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
 	e1 = NULL;
 	/* C99 variable declaration? */
 	if (lookup_type(token)) {
-		token = external_declaration(token, &syms);
+		token = external_declaration(token, &syms, NULL);
 	} else {
 		token = parse_expression(token, &e1);
 		token = expect(token, ';', "in 'for'");
@@ -2469,7 +2469,7 @@ static struct token * statement_list(struct token *token, struct statement_list
 				seen_statement = 0;
 			}
 			stmt = alloc_statement(token->pos, STMT_DECLARATION);
-			token = external_declaration(token, &stmt->declaration);
+			token = external_declaration(token, &stmt->declaration, NULL);
 		} else {
 			seen_statement = Wdeclarationafterstatement;
 			token = statement(token, &stmt);
@@ -2797,7 +2797,8 @@ static struct token *toplevel_asm_declaration(struct token *token, struct symbol
 	return token;
 }
 
-struct token *external_declaration(struct token *token, struct symbol_list **list)
+struct token *external_declaration(struct token *token, struct symbol_list **list,
+		validate_decl_t validate_decl)
 {
 	struct ident *ident = NULL;
 	struct symbol *decl;
@@ -2885,6 +2886,9 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 			token = initializer(&decl->initializer, token->next);
 		}
 		if (!is_typedef) {
+			if (validate_decl)
+				validate_decl(decl);
+
 			if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
 				add_symbol(list, decl);
 				fn_local_symbol(decl);
diff --git a/parse.h b/parse.h
index a2b3e3889..26227a387 100644
--- a/parse.h
+++ b/parse.h
@@ -129,7 +129,8 @@ extern int show_statement(struct statement *);
 extern void show_statement_list(struct statement_list *, const char *);
 extern int show_expression(struct expression *);
 
-extern struct token *external_declaration(struct token *token, struct symbol_list **list);
+typedef void (*validate_decl_t)(struct symbol *decl);
+extern struct token *external_declaration(struct token *, struct symbol_list **, validate_decl_t);
 
 extern struct symbol *ctype_integer(int size, int want_unsigned);
 
-- 
2.11.1

[PATCH v4 5/6] check the storage of C99 for-loop initializers

[Luc Van Oostenryck]

In C99, it is valid to declare a variable inside a
for-loop initializer but only when the storage is local
(automatic or register). Until now this was not enforced.

Fix this, when parsing declarations in a for-loop context,
by calling external_decl() with a validate method doing the
appropriate check of the storage.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                        | 13 ++++++++++++-
 validation/c99-for-loop-decl.c |  1 -
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/parse.c b/parse.c
index d91a4bced..6ec1bab97 100644
--- a/parse.c
+++ b/parse.c
@@ -2229,6 +2229,17 @@ static struct token *parse_return_statement(struct token *token, struct statemen
 	return expression_statement(token->next, &stmt->ret_value);
 }
 
+static void validate_for_loop_decl(struct symbol *sym)
+{
+	unsigned long storage = sym->ctype.modifiers & MOD_STORAGE;
+
+	if (storage & ~(MOD_AUTO | MOD_REGISTER)) {
+		const char *name = show_ident(sym->ident);
+		sparse_error(sym->pos, "non-local var '%s' in for-loop initializer", name);
+		sym->ctype.modifiers &= ~MOD_STORAGE;
+	}
+}
+
 static struct token *parse_for_statement(struct token *token, struct statement *stmt)
 {
 	struct symbol_list *syms;
@@ -2242,7 +2253,7 @@ static struct token *parse_for_statement(struct token *token, struct statement *
 	e1 = NULL;
 	/* C99 variable declaration? */
 	if (lookup_type(token)) {
-		token = external_declaration(token, &syms, NULL);
+		token = external_declaration(token, &syms, validate_for_loop_decl);
 	} else {
 		token = parse_expression(token, &e1);
 		token = expect(token, ';', "in 'for'");
diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index b9db8c9c6..e813b0ae3 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -30,7 +30,6 @@ static int c99(void)
 
 /*
  * check-name: C99 for-loop declarations
- * check-known-to-fail
  *
  * check-error-start
 c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
-- 
2.11.1

[PATCH v4 6/6] move 'extern with initializer' validation after the validate method

[Luc Van Oostenryck]

Before the call to the method external_decl::validate_decl()
there is another validation done which check if the declaration
linkage is not external, otherwise an error is issued and the
'extern' is removed from the declaration.

While also valid for C99 for-loop initializer, this is less
desirable because in this context, 'extern' is invalid anyway
and removing it from the declaration make it imposible to issue
a diagnostic about it.

Fix this by moving the 'extern with initializer' check after the
call to validate_decl() method, where it is always pertinent and
so allowing process_for_loop_decl() to make its own diagnostic.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                        | 9 +++++----
 validation/c99-for-loop-decl.c | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/parse.c b/parse.c
index 6ec1bab97..80f0337cc 100644
--- a/parse.c
+++ b/parse.c
@@ -2890,16 +2890,17 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
 
 	for (;;) {
 		if (!is_typedef && match_op(token, '=')) {
-			if (decl->ctype.modifiers & MOD_EXTERN) {
-				warning(decl->pos, "symbol with external linkage has initializer");
-				decl->ctype.modifiers &= ~MOD_EXTERN;
-			}
 			token = initializer(&decl->initializer, token->next);
 		}
 		if (!is_typedef) {
 			if (validate_decl)
 				validate_decl(decl);
 
+			if (decl->initializer && decl->ctype.modifiers & MOD_EXTERN) {
+				warning(decl->pos, "symbol with external linkage has initializer");
+				decl->ctype.modifiers &= ~MOD_EXTERN;
+			}
+
 			if (!(decl->ctype.modifiers & (MOD_EXTERN | MOD_INLINE))) {
 				add_symbol(list, decl);
 				fn_local_symbol(decl);
diff --git a/validation/c99-for-loop-decl.c b/validation/c99-for-loop-decl.c
index e813b0ae3..d382d3c9b 100644
--- a/validation/c99-for-loop-decl.c
+++ b/validation/c99-for-loop-decl.c
@@ -32,7 +32,7 @@ static int c99(void)
  * check-name: C99 for-loop declarations
  *
  * check-error-start
-c99-for-loop-decl.c:22:27: warning: symbol with external linkage has initializer
+c99-for-loop-decl.c:22:27: error: non-local var 'l' in for-loop initializer
 c99-for-loop-decl.c:24:27: error: non-local var 'm' in for-loop initializer
 c99-for-loop-decl.c:26:27: error: non-local var 'n' in for-loop initializer
 c99-for-loop-decl.c:9:16: error: undefined identifier 'i'
-- 
2.11.1

Re: [PATCH v4 0/6] more validation of C99 for-loop initializers

[Christopher Li]

On Mon, Mar 6, 2017 at 3:21 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> This serie adds scope & storage validations of C99-style for-loop
> initializers.

This series looks great now. Thank you.

All applied to sparse-next.

Chris

Re: [PATCH v4 0/6] more validation of C99 for-loop initializers

[Luc Van Oostenryck]

On Mon, Mar 06, 2017 at 08:59:35AM +0800, Christopher Li wrote:
> On Mon, Mar 6, 2017 at 3:21 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > This serie adds scope & storage validations of C99-style for-loop
> > initializers.
> 
> This series looks great now. Thank you.
> 
> All applied to sparse-next.

Yes, I like how it turned out.
Thanks to you too.

Luc

Re: [PATCH v3 4/7] add a method to external_declaration()

[Christopher Li]

On Sun, Mar 5, 2017 at 11:12 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> And to be honest, the current name 'external_declaration()' is far
> from being a good name eithers: it doesn't give a hint on *what*
> the function is doing and it's not only for 'extern' declaration.

Actually, external_declaration is a GREAT name. The reason is that,
it is base on the C grammar  BNF form.

<external-declaration> ::= <function-definition>
                         | <declaration>

It consume just enough token to complete a "<external-declaration>".

Same as a lot of other sparse parser function name.
"struct-or-union", "pointer" etc. In that regard, it is following a consistent
naming scheme. It is easier to match code to the BNF form.
Conceptional clear about what the code supposed to do.

I did not come up with that, Linus did.

Chris