[PATCH] jffs2: fix sparse errors: directive in argument list

[PATCH] jffs2: fix sparse errors: directive in argument list

[Erico Nunes]

This patch fixes the following errors reported when running sparse:
fs/jffs2/super.c:378:1: error: directive in argument list
fs/jffs2/super.c:380:1: error: directive in argument list
fs/jffs2/super.c:381:1: error: directive in argument list
fs/jffs2/super.c:383:1: error: directive in argument list
---
 fs/jffs2/super.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 0defb1c..ec9e16f 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -361,6 +361,14 @@ MODULE_ALIAS_FS("jffs2");
 static int __init init_jffs2_fs(void)
 {
 	int ret;
+	const char desc[] = "version 2.2."
+#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
+		" (NAND)"
+#endif
+#ifdef CONFIG_JFFS2_SUMMARY
+		" (SUMMARY) "
+#endif
+		" © 2001-2006 Red Hat, Inc.";
 
 	/* Paranoia checks for on-medium structures. If we ask GCC
 	   to pack them with __attribute__((packed)) then it _also_
@@ -374,14 +382,7 @@ static int __init init_jffs2_fs(void)
 	BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
 	BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
 
-	pr_info("version 2.2."
-#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
-	       " (NAND)"
-#endif
-#ifdef CONFIG_JFFS2_SUMMARY
-	       " (SUMMARY) "
-#endif
-	       " © 2001-2006 Red Hat, Inc.\n");
+	pr_info("%s\n", desc);
 
 	jffs2_inode_cachep = kmem_cache_create("jffs2_i",
 					     sizeof(struct jffs2_inode_info),
-- 
1.8.4.2

Re: [PATCH] jffs2: fix sparse errors: directive in argument list

[Joe Perches]

On Sun, 2013-11-17 at 18:18 -0200, Erico Nunes wrote:
> This patch fixes the following errors reported when running sparse:
> fs/jffs2/super.c:378:1: error: directive in argument list
> fs/jffs2/super.c:380:1: error: directive in argument list
> fs/jffs2/super.c:381:1: error: directive in argument list
> fs/jffs2/super.c:383:1: error: directive in argument list

Seems like a sparse error more than anything else.
(adding sparse mailing list)

Also you need to add a "Signed-off-by:" line
with your email address.

see: Documentation/SubmittingPatches

> ---
>  fs/jffs2/super.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
> index 0defb1c..ec9e16f 100644
> --- a/fs/jffs2/super.c
> +++ b/fs/jffs2/super.c
> @@ -361,6 +361,14 @@ MODULE_ALIAS_FS("jffs2");
>  static int __init init_jffs2_fs(void)
>  {
>  	int ret;
> +	const char desc[] = "version 2.2."
> +#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
> +		" (NAND)"
> +#endif
> +#ifdef CONFIG_JFFS2_SUMMARY
> +		" (SUMMARY) "
> +#endif
> +		" © 2001-2006 Red Hat, Inc.";
>  
>  	/* Paranoia checks for on-medium structures. If we ask GCC
>  	   to pack them with __attribute__((packed)) then it _also_
> @@ -374,14 +382,7 @@ static int __init init_jffs2_fs(void)
>  	BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
>  	BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
>  
> -	pr_info("version 2.2."
> -#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
> -	       " (NAND)"
> -#endif
> -#ifdef CONFIG_JFFS2_SUMMARY
> -	       " (SUMMARY) "
> -#endif
> -	       " © 2001-2006 Red Hat, Inc.\n");
> +	pr_info("%s\n", desc);
>  
>  	jffs2_inode_cachep = kmem_cache_create("jffs2_i",
>  					     sizeof(struct jffs2_inode_info),

Re: [PATCH] jffs2: fix sparse errors: directive in argument list

[Erico Nunes]

Thanks for your reply.

I'll resubmit shortly.

Do you mean it as an error in the sparse tool?
I don't think so, I took a look and it seems to be designed to output
an error for that case.


On Sun, Nov 17, 2013 at 7:40 PM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2013-11-17 at 18:18 -0200, Erico Nunes wrote:
>> This patch fixes the following errors reported when running sparse:
>> fs/jffs2/super.c:378:1: error: directive in argument list
>> fs/jffs2/super.c:380:1: error: directive in argument list
>> fs/jffs2/super.c:381:1: error: directive in argument list
>> fs/jffs2/super.c:383:1: error: directive in argument list
>
> Seems like a sparse error more than anything else.
> (adding sparse mailing list)
>
> Also you need to add a "Signed-off-by:" line
> with your email address.
>
> see: Documentation/SubmittingPatches
>
>> ---
>>  fs/jffs2/super.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
>> index 0defb1c..ec9e16f 100644
>> --- a/fs/jffs2/super.c
>> +++ b/fs/jffs2/super.c
>> @@ -361,6 +361,14 @@ MODULE_ALIAS_FS("jffs2");
>>  static int __init init_jffs2_fs(void)
>>  {
>>       int ret;
>> +     const char desc[] = "version 2.2."
>> +#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>> +             " (NAND)"
>> +#endif
>> +#ifdef CONFIG_JFFS2_SUMMARY
>> +             " (SUMMARY) "
>> +#endif
>> +             " © 2001-2006 Red Hat, Inc.";
>>
>>       /* Paranoia checks for on-medium structures. If we ask GCC
>>          to pack them with __attribute__((packed)) then it _also_
>> @@ -374,14 +382,7 @@ static int __init init_jffs2_fs(void)
>>       BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
>>       BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
>>
>> -     pr_info("version 2.2."
>> -#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>> -            " (NAND)"
>> -#endif
>> -#ifdef CONFIG_JFFS2_SUMMARY
>> -            " (SUMMARY) "
>> -#endif
>> -            " © 2001-2006 Red Hat, Inc.\n");
>> +     pr_info("%s\n", desc);
>>
>>       jffs2_inode_cachep = kmem_cache_create("jffs2_i",
>>                                            sizeof(struct jffs2_inode_info),
>
>
>

[PATCH] jffs2: fix sparse errors: directive in argument list

[Erico Nunes]

This patch fixes the following errors reported when running sparse:
fs/jffs2/super.c:378:1: error: directive in argument list
fs/jffs2/super.c:380:1: error: directive in argument list
fs/jffs2/super.c:381:1: error: directive in argument list
fs/jffs2/super.c:383:1: error: directive in argument list

Signed-off-by: Erico Nunes <nunes.erico@gmail.com>
---
 fs/jffs2/super.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 0defb1c..ec9e16f 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -361,6 +361,14 @@ MODULE_ALIAS_FS("jffs2");
 static int __init init_jffs2_fs(void)
 {
 	int ret;
+	const char desc[] = "version 2.2."
+#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
+		" (NAND)"
+#endif
+#ifdef CONFIG_JFFS2_SUMMARY
+		" (SUMMARY) "
+#endif
+		" © 2001-2006 Red Hat, Inc.";
 
 	/* Paranoia checks for on-medium structures. If we ask GCC
 	   to pack them with __attribute__((packed)) then it _also_
@@ -374,14 +382,7 @@ static int __init init_jffs2_fs(void)
 	BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
 	BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
 
-	pr_info("version 2.2."
-#ifdef CONFIG_JFFS2_FS_WRITEBUFFER
-	       " (NAND)"
-#endif
-#ifdef CONFIG_JFFS2_SUMMARY
-	       " (SUMMARY) "
-#endif
-	       " © 2001-2006 Red Hat, Inc.\n");
+	pr_info("%s\n", desc);
 
 	jffs2_inode_cachep = kmem_cache_create("jffs2_i",
 					     sizeof(struct jffs2_inode_info),
-- 
1.8.4.2

Re: [PATCH] jffs2: fix sparse errors: directive in argument list

[Joe Perches]

On Sun, 2013-11-17 at 20:34 -0200, Erico Nunes wrote:
> Do you mean it as an error in the sparse tool?

Yes.  I think it's a defect in how sparse
treats string concatenation.

That style:

	printk("%s\n",
#ifdef FOO
	"foo"
#endif
#ifdef BAR
	"bar"
#endif
	"string");

is pretty common in the kernel sources.

The patch itself is otherwise fine, but
perhaps unnecessary.

Re: [PATCH] jffs2: fix sparse errors: directive in argument list

[Richard Weinberger]

On Sun, Nov 17, 2013 at 11:45 PM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2013-11-17 at 20:34 -0200, Erico Nunes wrote:
>> Do you mean it as an error in the sparse tool?
>
> Yes.  I think it's a defect in how sparse
> treats string concatenation.
>
> That style:
>
>         printk("%s\n",
> #ifdef FOO
>         "foo"
> #endif
> #ifdef BAR
>         "bar"
> #endif
>         "string");
>
> is pretty common in the kernel sources.
>
> The patch itself is otherwise fine, but
> perhaps unnecessary.

I agree with Joe, the patch fixes a non-issue.

-- 
Thanks,
//richard

Re: [PATCH] jffs2: fix sparse errors: directive in argument list

[Al Viro]

On Sun, Nov 17, 2013 at 02:45:05PM -0800, Joe Perches wrote:
> On Sun, 2013-11-17 at 20:34 -0200, Erico Nunes wrote:
> > Do you mean it as an error in the sparse tool?
> 
> Yes.  I think it's a defect in how sparse
> treats string concatenation.
> 
> That style:
> 
> 	printk("%s\n",
> #ifdef FOO
> 	"foo"
> #endif
> #ifdef BAR
> 	"bar"
> #endif
> 	"string");
> 
> is pretty common in the kernel sources.

... and it's perfectly fine, until somebody starts playing in nasal
daemon country and do that in *macro* arguments.  And a nasal daemon
country it is - it's an undefined behaviour.  See 6.10.3p11 in C99.
And trying to define a semantics for that gets real ugly real fast.
sparse matches gcc behaviour (I hope), but it warns about such abuses.
It's a defect, all right - one being reported by sparse.

Folks, please, RTFStandard if you decide to play clever games with
preprocessing.  Chapter 6.10 is not particulary long or complicated.
C99 has improved the preprocessor semantics a whole lot compared to
the earlier horrible mess (mostly by defining it in terms of token
stream transformations rather then text ones), but it's still very
easy to abuse...

[PATCH] Clarify error on directive in macro arguments (Re: [PATCH] jffs2: fix sparse errors: directive in argument list)

[Greg Price]

[+linux-sparse and Chris]

On Mon, Nov 18, 2013 at 01:33:49AM +0000, Al Viro wrote:
> On Sun, Nov 17, 2013 at 02:45:05PM -0800, Joe Perches wrote:
> > Yes.  I think it's a defect in how sparse
> > treats string concatenation.
> > 
> > That style [... with printk ...] is pretty common in the kernel sources.
> 
> ... and it's perfectly fine, until somebody starts playing in nasal
> daemon country and do that in *macro* arguments.  And a nasal daemon
> country it is - it's an undefined behaviour.  See 6.10.3p11 in C99.
> And trying to define a semantics for that gets real ugly real fast.
> sparse matches gcc behaviour (I hope), but it warns about such abuses.
> It's a defect, all right - one being reported by sparse.

Perhaps the following tweak to the error message would make this
subtlety clearer?

Cheers,
Greg


From: Greg Price <price@mit.edu>
Date: Sun, 17 Nov 2013 17:57:41 -0800
Subject: [PATCH] Clarify error on directive in macro arguments

Preprocessor directives in the arguments of a real function
are innocuous and in some contexts common.  If a developer
doesn't realize that a "function" is implemented as a macro,
they may mistake this error for a false alarm.

See http://www.spinics.net/lists/kernel/msg1636974.html
and http://www.spinics.net/lists/kernel/msg1636976.html
for an example.

Easy enough to clarify that this is a macro, so do it.

Signed-off-by: Greg Price <price@mit.edu>
---
 pre-process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pre-process.c b/pre-process.c
index d521318..db58a97 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -204,7 +204,7 @@ static struct token *collect_arg(struct token *prev, int vararg, struct position
 		if (next->pos.newline && match_op(next, '#')) {
 			if (!next->pos.noexpand) {
 				sparse_error(next->pos,
-					     "directive in argument list");
+					     "directive in macro argument list");
 				preprocessor_line(stream, p);
 				__free_token(next);	/* Free the '#' token */
 				continue;
-- 
1.8.3.2

Re: [PATCH] Clarify error on directive in macro arguments (Re: [PATCH] jffs2: fix sparse errors: directive in argument list)

[Joe Perches]

On Sun, 2013-11-17 at 21:07 -0500, Greg Price wrote:
> [+linux-sparse and Chris]
> 
> On Mon, Nov 18, 2013 at 01:33:49AM +0000, Al Viro wrote:
> > On Sun, Nov 17, 2013 at 02:45:05PM -0800, Joe Perches wrote:
> > > Yes.  I think it's a defect in how sparse
> > > treats string concatenation.
> > > 
> > > That style [... with printk ...] is pretty common in the kernel sources.
> > 
> > ... and it's perfectly fine, until somebody starts playing in nasal
> > daemon country and do that in *macro* arguments.  And a nasal daemon
> > country it is - it's an undefined behaviour.  See 6.10.3p11 in C99.
> > And trying to define a semantics for that gets real ugly real fast.
> > sparse matches gcc behaviour (I hope), but it warns about such abuses.
> > It's a defect, all right - one being reported by sparse.
> 
> Perhaps the following tweak to the error message would make this
> subtlety clearer?

Maybe, but this case isn't a macro.  It's a function.
Dunno if differentiating when it's a macro or a
function is difficult though.

Re: [PATCH] Clarify error on directive in macro arguments (Re: [PATCH] jffs2: fix sparse errors: directive in argument list)

[Erico Nunes]

On Mon, Nov 18, 2013 at 12:15 AM, Joe Perches <joe@perches.com> wrote:
> On Sun, 2013-11-17 at 21:07 -0500, Greg Price wrote:
>
> Maybe, but this case isn't a macro.  It's a function.
> Dunno if differentiating when it's a macro or a
> function is difficult though.
>
>

The case which was initially reported by sparse at
http://www.spinics.net/lists/kernel/msg1636974.html was using pr_info
(which is a macro) and not printk.

Re: [PATCH] Clarify error on directive in macro arguments (Re: [PATCH] jffs2: fix sparse errors: directive in argument list)

[Al Viro]

On Sun, Nov 17, 2013 at 06:15:32PM -0800, Joe Perches wrote:
> > > sparse matches gcc behaviour (I hope), but it warns about such abuses.
> > > It's a defect, all right - one being reported by sparse.
> > 
> > Perhaps the following tweak to the error message would make this
> > subtlety clearer?
> 
> Maybe, but this case isn't a macro.  It's a function.
> Dunno if differentiating when it's a macro or a
> function is difficult though.

In which case?  With printk() it's perfectly fine and sparse
will not complain at all.  With pr_info(), OTOH, we have
#define pr_info(fmt, ...) \
        printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
and when you start playing that kind of games with it, you get warnings.

Re: [PATCH] Clarify error on directive in macro arguments (Re: [PATCH] jffs2: fix sparse errors: directive in argument list)

[Greg Price]

On Sun, Nov 17, 2013 at 06:15:32PM -0800, Joe Perches wrote:
> On Sun, 2013-11-17 at 21:07 -0500, Greg Price wrote:
> > Perhaps the following tweak to the error message would make this
> > subtlety clearer?
> 
> Maybe, but this case isn't a macro.  It's a function.
> Dunno if differentiating when it's a macro or a
> function is difficult though.

Yeah, this error message is already only emitted for directives in
macro arguments -- in this case, pr_info.  It's in sparse's
preprocessor code; the error arises when a directive is spotted while
parsing a macro's arguments.  By the time sparse (or an idealized C
compiler) parses the arguments of a real function, the token stream is
already the output of the preprocessor and any directives are gone.

Cheers,
Greg

Re: [PATCH] Clarify error on directive in macro arguments (Re: [PATCH] jffs2: fix sparse errors: directive in argument list)

[Josh Triplett]

On Sun, Nov 17, 2013 at 09:07:46PM -0500, Greg Price wrote:
> From: Greg Price <price@mit.edu>
> Date: Sun, 17 Nov 2013 17:57:41 -0800
> Subject: [PATCH] Clarify error on directive in macro arguments
> 
> Preprocessor directives in the arguments of a real function
> are innocuous and in some contexts common.  If a developer
> doesn't realize that a "function" is implemented as a macro,
> they may mistake this error for a false alarm.
> 
> See http://www.spinics.net/lists/kernel/msg1636974.html
> and http://www.spinics.net/lists/kernel/msg1636976.html
> for an example.
> 
> Easy enough to clarify that this is a macro, so do it.
> 
> Signed-off-by: Greg Price <price@mit.edu>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Good call.

>  pre-process.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pre-process.c b/pre-process.c
> index d521318..db58a97 100644
> --- a/pre-process.c
> +++ b/pre-process.c
> @@ -204,7 +204,7 @@ static struct token *collect_arg(struct token *prev, int vararg, struct position
>  		if (next->pos.newline && match_op(next, '#')) {
>  			if (!next->pos.noexpand) {
>  				sparse_error(next->pos,
> -					     "directive in argument list");
> +					     "directive in macro argument list");
>  				preprocessor_line(stream, p);
>  				__free_token(next);	/* Free the '#' token */
>  				continue;
> -- 
> 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] jffs2: fix sparse errors: directive in argument list

[Josh Triplett]

On Mon, Nov 18, 2013 at 01:33:49AM +0000, Al Viro wrote:
> On Sun, Nov 17, 2013 at 02:45:05PM -0800, Joe Perches wrote:
> > On Sun, 2013-11-17 at 20:34 -0200, Erico Nunes wrote:
> > > Do you mean it as an error in the sparse tool?
> > 
> > Yes.  I think it's a defect in how sparse
> > treats string concatenation.
> > 
> > That style:
> > 
> > 	printk("%s\n",
> > #ifdef FOO
> > 	"foo"
> > #endif
> > #ifdef BAR
> > 	"bar"
> > #endif
> > 	"string");
> > 
> > is pretty common in the kernel sources.
> 
> ... and it's perfectly fine, until somebody starts playing in nasal
> daemon country and do that in *macro* arguments.  And a nasal daemon
> country it is - it's an undefined behaviour.  See 6.10.3p11 in C99.
> And trying to define a semantics for that gets real ugly real fast.
> sparse matches gcc behaviour (I hope), but it warns about such abuses.
> It's a defect, all right - one being reported by sparse.
> 
> Folks, please, RTFStandard if you decide to play clever games with
> preprocessing.  Chapter 6.10 is not particulary long or complicated.
> C99 has improved the preprocessor semantics a whole lot compared to
> the earlier horrible mess (mostly by defining it in terms of token
> stream transformations rather then text ones), but it's still very
> easy to abuse...

And since pr_foo (and the underlying pr_fmt) are already macros, that's
exactly the problem.

- Josh Triplett