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

login
register
mail settings
Submitter Greg Price
Date Nov. 18, 2013, 2:07 a.m.
Message ID <20131118020746.GX16018@ringworld.MIT.EDU>
Download mbox | patch
Permalink /patch/291925/
State New
Headers show

Comments

Greg Price - Nov. 18, 2013, 2:07 a.m.
[+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(-)
Joe Perches - Nov. 18, 2013, 2:15 a.m.
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.
Erico Nunes - Nov. 18, 2013, 2:23 a.m.
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.
Al Viro - Nov. 18, 2013, 2:34 a.m.
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.
Greg Price - Nov. 18, 2013, 4:01 a.m.
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

Patch

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;