* [PATCH v3 1/7] replace test for c99 for-loop initializers
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
@ 2017-02-28 10:03 ` Luc Van Oostenryck
2017-02-28 10:03 ` [PATCH v3 2/7] add test case for scope of C99 for-loop declarations Luc Van Oostenryck
` (6 subsequent siblings)
7 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 10:03 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v3 2/7] add test case for scope of C99 for-loop declarations
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
2017-02-28 10:03 ` [PATCH v3 1/7] replace test for c99 " Luc Van Oostenryck
@ 2017-02-28 10:03 ` Luc Van Oostenryck
2017-02-28 10:03 ` [PATCH v3 3/7] add test cases for storage of c99 " Luc Van Oostenryck
` (5 subsequent siblings)
7 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 10:03 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v3 3/7] add test cases for storage of c99 for-loop declarations
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
2017-02-28 10:03 ` [PATCH v3 1/7] replace test for c99 " Luc Van Oostenryck
2017-02-28 10:03 ` [PATCH v3 2/7] add test case for scope of C99 for-loop declarations Luc Van Oostenryck
@ 2017-02-28 10:03 ` Luc Van Oostenryck
2017-02-28 10:04 ` [PATCH v3 4/7] add a method to external_declaration() Luc Van Oostenryck
` (4 subsequent siblings)
7 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 10:03 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v3 4/7] add a method to external_declaration()
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
` (2 preceding siblings ...)
2017-02-28 10:03 ` [PATCH v3 3/7] add test cases for storage of c99 " Luc Van Oostenryck
@ 2017-02-28 10:04 ` Luc Van Oostenryck
2017-03-05 14:04 ` Christopher Li
2017-02-28 10:04 ` [PATCH v3 5/7] check the storage " Luc Van Oostenryck
` (3 subsequent siblings)
7 siblings, 1 reply; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 10:04 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v3 4/7] add a method to external_declaration()
2017-02-28 10:04 ` [PATCH v3 4/7] add a method to external_declaration() Luc Van Oostenryck
@ 2017-03-05 14:04 ` Christopher Li
2017-03-05 15:12 ` Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
0 siblings, 2 replies; 45+ messages in thread
From: Christopher Li @ 2017-03-05 14:04 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v3 4/7] add a method to external_declaration()
2017-03-05 14:04 ` Christopher Li
@ 2017-03-05 15:12 ` Luc Van Oostenryck
2017-03-06 1:13 ` Christopher Li
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
1 sibling, 1 reply; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 15:12 UTC (permalink / raw)
To: Christopher Li; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v3 4/7] add a method to external_declaration()
2017-03-05 15:12 ` Luc Van Oostenryck
@ 2017-03-06 1:13 ` Christopher Li
0 siblings, 0 replies; 45+ messages in thread
From: Christopher Li @ 2017-03-06 1:13 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 0/6] more validation of C99 for-loop initializers
2017-03-05 14:04 ` Christopher Li
2017-03-05 15:12 ` Luc Van Oostenryck
@ 2017-03-05 19:21 ` Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 1/6] replace test for c99 " Luc Van Oostenryck
` (6 more replies)
1 sibling, 7 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 19:21 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH v4 1/6] replace test for c99 for-loop initializers
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
@ 2017-03-05 19:21 ` Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 2/6] add test case for scope of C99 for-loop declarations Luc Van Oostenryck
` (5 subsequent siblings)
6 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 19:21 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 2/6] add test case for scope of C99 for-loop declarations
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 1/6] replace test for c99 " Luc Van Oostenryck
@ 2017-03-05 19:21 ` Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 3/6] add test cases for storage of c99 " Luc Van Oostenryck
` (4 subsequent siblings)
6 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 19:21 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 3/6] add test cases for storage of c99 for-loop declarations
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 1/6] replace test for c99 " Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 2/6] add test case for scope of C99 for-loop declarations Luc Van Oostenryck
@ 2017-03-05 19:21 ` Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 4/6] add an optional validation method to external_declaration() Luc Van Oostenryck
` (3 subsequent siblings)
6 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 19:21 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 4/6] add an optional validation method to external_declaration()
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
` (2 preceding siblings ...)
2017-03-05 19:21 ` [PATCH v4 3/6] add test cases for storage of c99 " Luc Van Oostenryck
@ 2017-03-05 19:21 ` Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 5/6] check the storage of C99 for-loop initializers Luc Van Oostenryck
` (2 subsequent siblings)
6 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 19:21 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 5/6] check the storage of C99 for-loop initializers
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
` (3 preceding siblings ...)
2017-03-05 19:21 ` [PATCH v4 4/6] add an optional validation method to external_declaration() Luc Van Oostenryck
@ 2017-03-05 19:21 ` Luc Van Oostenryck
2017-03-05 19:21 ` [PATCH v4 6/6] move 'extern with initializer' validation after the validate method Luc Van Oostenryck
2017-03-06 0:59 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Christopher Li
6 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 19:21 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v4 6/6] move 'extern with initializer' validation after the validate method
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
` (4 preceding siblings ...)
2017-03-05 19:21 ` [PATCH v4 5/6] check the storage of C99 for-loop initializers Luc Van Oostenryck
@ 2017-03-05 19:21 ` Luc Van Oostenryck
2017-03-06 0:59 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Christopher Li
6 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 19:21 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v4 0/6] more validation of C99 for-loop initializers
2017-03-05 19:21 ` [PATCH v4 0/6] more validation of C99 for-loop initializers Luc Van Oostenryck
` (5 preceding siblings ...)
2017-03-05 19:21 ` [PATCH v4 6/6] move 'extern with initializer' validation after the validate method Luc Van Oostenryck
@ 2017-03-06 0:59 ` Christopher Li
2017-03-06 1:08 ` Luc Van Oostenryck
6 siblings, 1 reply; 45+ messages in thread
From: Christopher Li @ 2017-03-06 0:59 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 5/7] check the storage of C99 for-loop initializers
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
` (3 preceding siblings ...)
2017-02-28 10:04 ` [PATCH v3 4/7] add a method to external_declaration() Luc Van Oostenryck
@ 2017-02-28 10:04 ` Luc Van Oostenryck
2017-03-05 14:26 ` Christopher Li
2017-02-28 10:04 ` [PATCH v3 6/7] make process_decl() aware of the presence of an initializer Luc Van Oostenryck
` (2 subsequent siblings)
7 siblings, 1 reply; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 10:04 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v3 5/7] check the storage of C99 for-loop initializers
2017-02-28 10:04 ` [PATCH v3 5/7] check the storage " Luc Van Oostenryck
@ 2017-03-05 14:26 ` Christopher Li
2017-03-05 15:24 ` Luc Van Oostenryck
0 siblings, 1 reply; 45+ messages in thread
From: Christopher Li @ 2017-03-05 14:26 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v3 5/7] check the storage of C99 for-loop initializers
2017-03-05 14:26 ` Christopher Li
@ 2017-03-05 15:24 ` Luc Van Oostenryck
0 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 15:24 UTC (permalink / raw)
To: Christopher Li; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 6/7] make process_decl() aware of the presence of an initializer
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
` (4 preceding siblings ...)
2017-02-28 10:04 ` [PATCH v3 5/7] check the storage " Luc Van Oostenryck
@ 2017-02-28 10:04 ` Luc Van Oostenryck
2017-03-05 14:49 ` Christopher Li
2017-02-28 10:04 ` [PATCH v3 7/7] move check extern with initializer to default_process_decl() Luc Van Oostenryck
2017-02-28 16:34 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Christopher Li
7 siblings, 1 reply; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 10:04 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v3 6/7] make process_decl() aware of the presence of an initializer
2017-02-28 10:04 ` [PATCH v3 6/7] make process_decl() aware of the presence of an initializer Luc Van Oostenryck
@ 2017-03-05 14:49 ` Christopher Li
2017-03-05 15:29 ` Luc Van Oostenryck
0 siblings, 1 reply; 45+ messages in thread
From: Christopher Li @ 2017-03-05 14:49 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v3 6/7] make process_decl() aware of the presence of an initializer
2017-03-05 14:49 ` Christopher Li
@ 2017-03-05 15:29 ` Luc Van Oostenryck
0 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-03-05 15:29 UTC (permalink / raw)
To: Christopher Li; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 7/7] move check extern with initializer to default_process_decl()
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
` (5 preceding siblings ...)
2017-02-28 10:04 ` [PATCH v3 6/7] make process_decl() aware of the presence of an initializer Luc Van Oostenryck
@ 2017-02-28 10:04 ` Luc Van Oostenryck
2017-02-28 16:34 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Christopher Li
7 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 10:04 UTC (permalink / raw)
To: linux-sparse; +Cc: Christopher Li, 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v3 0/7] more validation of C99 for-loop initializers
2017-02-28 10:03 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Luc Van Oostenryck
` (6 preceding siblings ...)
2017-02-28 10:04 ` [PATCH v3 7/7] move check extern with initializer to default_process_decl() Luc Van Oostenryck
@ 2017-02-28 16:34 ` Christopher Li
2017-02-28 16:40 ` Luc Van Oostenryck
7 siblings, 1 reply; 45+ messages in thread
From: Christopher Li @ 2017-02-28 16:34 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v3 0/7] more validation of C99 for-loop initializers
2017-02-28 16:34 ` [PATCH v3 0/7] more validation of C99 for-loop initializers Christopher Li
@ 2017-02-28 16:40 ` Luc Van Oostenryck
0 siblings, 0 replies; 45+ messages in thread
From: Luc Van Oostenryck @ 2017-02-28 16:40 UTC (permalink / raw)
To: Christopher Li; +Cc: Linux-Sparse
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.
^ permalink raw reply [flat|nested] 45+ messages in thread