Sparse preprocessing bug with zero-arg variadic macros

Sparse preprocessing bug with zero-arg variadic macros

[Josh Poimboeuf]

Hi,

I think I'm seeing a bug in the sparse preprocessor.  I've reduced it to
the following test case.

----------------------
/* If the macro has arguments, prepend them with a comma. */
#define ARGS_APPEND(...) , ## __VA_ARGS__

#define __ARG17(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12,	\
		_13, _14, _15, _16, _17, ...) _17

/* Return 1 if the macro has arguments, 0 otherwise. */
#define HAS_ARGS(...) __ARG17(0, ## __VA_ARGS__, 1, 1, 1, 1, 1, 1, 1,	\
			      1, 1, 1, 1, 1, 1, 1, 1, 1, 0)

/* Macros for passing inline asm constraints as macro arguments */
#define ARGS(args...) args
#define OUTPUTS ARGS
#define INPUTS ARGS
#define CLOBBERS ARGS

#define __CLOBBERS_APPEND_0(...)
#define __CLOBBERS_APPEND_1(...)  , __VA_ARGS__

#define __CLOBBERS_APPEND(has_args, ...) \
	__CLOBBERS_APPEND_ ## has_args (__VA_ARGS__)

#define _CLOBBERS_APPEND(has_args, ...) \
	__CLOBBERS_APPEND(has_args, __VA_ARGS__)

/* If the macro has arguments, prepend them with a colon. */
#define CLOBBERS_APPEND(...) \
	_CLOBBERS_APPEND(HAS_ARGS(__VA_ARGS__), __VA_ARGS__)


void foo()
{
	ARGS_APPEND()
	CLOBBERS_APPEND()
}
----------------------

  $ gcc -c -o test.o test.c -E; cat test.o
  # 1 "test.c"
  # 1 "<built-in>"
  # 1 "<command-line>"
  # 31 "<command-line>"
  # 1 "/usr/include/stdc-predef.h" 1 3 4
  # 32 "<command-line>" 2
  # 1 "test.c"
  # 31 "test.c"
  void foo()
  {
  
  
  }

  $ sparse -c -o test.o test.c -E
  
  void foo()
  {
  			,
  			,
  }


Notice that sparse incorrectly inserts a comma for both macros when they
have zero arguments.

-- 
Josh

Re: Sparse preprocessing bug with zero-arg variadic macros

[Linus Torvalds]

On Thu, Aug 31, 2017 at 6:34 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> I think I'm seeing a bug in the sparse preprocessor.  I've reduced it to
> the following test case.

I think the real reduced test-case is just this:

    #define ARGS_APPEND(...) ,## __VA_ARGS__
    ARGS_APPEND()

and you can run if through "sparse -E" to see the comma (while gcc -E
does not have it).

I'm adding Al to the cc list because he's the pre-processor person.
Hopefully he has gotten out from under most of his emails from his
move.

sparse gets it right if there is any non-VA_ARGS argument to the
symbol, but not if __VA_ARGS__ is all of the argument to the macro.

Al?

                 Linus

Re: Sparse preprocessing bug with zero-arg variadic macros

[Al Viro]

On Thu, Aug 31, 2017 at 10:19:32AM -0700, Linus Torvalds wrote:
> On Thu, Aug 31, 2017 at 6:34 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > I think I'm seeing a bug in the sparse preprocessor.  I've reduced it to
> > the following test case.
> 
> I think the real reduced test-case is just this:
> 
>     #define ARGS_APPEND(...) ,## __VA_ARGS__
>     ARGS_APPEND()
> 
> and you can run if through "sparse -E" to see the comma (while gcc -E
> does not have it).
> 
> I'm adding Al to the cc list because he's the pre-processor person.
> Hopefully he has gotten out from under most of his emails from his
> move.
> 
> sparse gets it right if there is any non-VA_ARGS argument to the
> symbol, but not if __VA_ARGS__ is all of the argument to the macro.

Umm...  The problem is in collect_arguments() - it treats that as "argument
present, expands to empty" rather than "argument absent" in case when the
argument list consists of ... and no arguments are given.

What a mess...  Note that for non-vararg it *is* the right interpretation
(with #define A(x) [x] we will have A() interpreted as "empty token sequence
as the only argument", not "no arguments given").  For vararg case we
normally do not need to distinguish "not given" and "empty" - the only
thing that cares is exactly the ,## kludge.  There with
#define B(x,...) [x,##__VA_ARGS__]
B(1) and B(1,) yield [1] and [1,] resp.  And for everything other than
"just ..." we even get it right...

I see what's going on there; will post a fix in a few.

Re: Sparse preprocessing bug with zero-arg variadic macros

[Al Viro]

On Thu, Aug 31, 2017 at 09:54:33PM +0100, Al Viro wrote:
> What a mess...  Note that for non-vararg it *is* the right interpretation
> (with #define A(x) [x] we will have A() interpreted as "empty token sequence
> as the only argument", not "no arguments given").  For vararg case we
> normally do not need to distinguish "not given" and "empty" - the only
> thing that cares is exactly the ,## kludge.  There with
> #define B(x,...) [x,##__VA_ARGS__]
> B(1) and B(1,) yield [1] and [1,] resp.  And for everything other than
> "just ..." we even get it right...
> 
> I see what's going on there; will post a fix in a few.


Fix macro argument parsing for (...) case

Nasty corner case for the sake of ,##__VA_ARGS__ perversion - for something
like #define A(x,...) [x,##__VA_ARGS] we want A(1) to expand to [1] and
A(1,) - to [1,].  In other words, "no vararg given" and "vararg empty" are
different and need to be distinguished.  Unfortunately, in case when there
was nothing but vararg we got it wrong - #define A(...) ,##__VA_ARGS ended
up with A() interpreted as "one empty argument" (as it would in non-vararg
case) rather than "zero arguments".

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/pre-process.c b/pre-process.c
index 74414df..8800dce 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -296,9 +296,11 @@ static int collect_arguments(struct token *start, struct token *arglist, struct
 		for (count = 0; count < wanted; count++) {
 			struct argcount *p = &arglist->next->count;
 			next = collect_arg(start, p->vararg, &what->pos, p->normal);
-			arglist = arglist->next->next;
 			if (eof_token(next))
 				goto Eclosing;
+			if (p->vararg && wanted == 1 && eof_token(start->next))
+				break;
+			arglist = arglist->next->next;
 			args[count].arg = start->next;
 			args[count].n_normal = p->normal;
 			args[count].n_quoted = p->quoted;

Re: Sparse preprocessing bug with zero-arg variadic macros

[Josh Triplett]

On Thu, Aug 31, 2017 at 10:09:22PM +0100, Al Viro wrote:
> On Thu, Aug 31, 2017 at 09:54:33PM +0100, Al Viro wrote:
> > What a mess...  Note that for non-vararg it *is* the right interpretation
> > (with #define A(x) [x] we will have A() interpreted as "empty token sequence
> > as the only argument", not "no arguments given").  For vararg case we
> > normally do not need to distinguish "not given" and "empty" - the only
> > thing that cares is exactly the ,## kludge.  There with
> > #define B(x,...) [x,##__VA_ARGS__]
> > B(1) and B(1,) yield [1] and [1,] resp.  And for everything other than
> > "just ..." we even get it right...
> > 
> > I see what's going on there; will post a fix in a few.
> 
> 
> Fix macro argument parsing for (...) case
> 
> Nasty corner case for the sake of ,##__VA_ARGS__ perversion - for something
> like #define A(x,...) [x,##__VA_ARGS] we want A(1) to expand to [1] and
> A(1,) - to [1,].  In other words, "no vararg given" and "vararg empty" are
> different and need to be distinguished.  Unfortunately, in case when there
> was nothing but vararg we got it wrong - #define A(...) ,##__VA_ARGS ended
> up with A() interpreted as "one empty argument" (as it would in non-vararg
> case) rather than "zero arguments".
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/pre-process.c b/pre-process.c
> index 74414df..8800dce 100644
> --- a/pre-process.c
> +++ b/pre-process.c
> @@ -296,9 +296,11 @@ static int collect_arguments(struct token *start, struct token *arglist, struct
>  		for (count = 0; count < wanted; count++) {
>  			struct argcount *p = &arglist->next->count;
>  			next = collect_arg(start, p->vararg, &what->pos, p->normal);
> -			arglist = arglist->next->next;
>  			if (eof_token(next))
>  				goto Eclosing;
> +			if (p->vararg && wanted == 1 && eof_token(start->next))
> +				break;
> +			arglist = arglist->next->next;
>  			args[count].arg = start->next;
>  			args[count].n_normal = p->normal;
>  			args[count].n_quoted = p->quoted;

This looks plausible; we should also add a test for it, though.

Re: Sparse preprocessing bug with zero-arg variadic macros

[Al Viro]

On Thu, Aug 31, 2017 at 02:34:44PM -0700, Josh Triplett wrote:
> On Thu, Aug 31, 2017 at 10:09:22PM +0100, Al Viro wrote:
> > On Thu, Aug 31, 2017 at 09:54:33PM +0100, Al Viro wrote:
> > > What a mess...  Note that for non-vararg it *is* the right interpretation
> > > (with #define A(x) [x] we will have A() interpreted as "empty token sequence
> > > as the only argument", not "no arguments given").  For vararg case we
> > > normally do not need to distinguish "not given" and "empty" - the only
> > > thing that cares is exactly the ,## kludge.  There with
> > > #define B(x,...) [x,##__VA_ARGS__]
> > > B(1) and B(1,) yield [1] and [1,] resp.  And for everything other than
> > > "just ..." we even get it right...
> > > 
> > > I see what's going on there; will post a fix in a few.
> > 
> > 
> > Fix macro argument parsing for (...) case
> > 
> > Nasty corner case for the sake of ,##__VA_ARGS__ perversion - for something
> > like #define A(x,...) [x,##__VA_ARGS] we want A(1) to expand to [1] and
> > A(1,) - to [1,].  In other words, "no vararg given" and "vararg empty" are
> > different and need to be distinguished.  Unfortunately, in case when there
> > was nothing but vararg we got it wrong - #define A(...) ,##__VA_ARGS ended
> > up with A() interpreted as "one empty argument" (as it would in non-vararg
> > case) rather than "zero arguments".
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > diff --git a/pre-process.c b/pre-process.c
> > index 74414df..8800dce 100644
> > --- a/pre-process.c
> > +++ b/pre-process.c
> > @@ -296,9 +296,11 @@ static int collect_arguments(struct token *start, struct token *arglist, struct
> >  		for (count = 0; count < wanted; count++) {
> >  			struct argcount *p = &arglist->next->count;
> >  			next = collect_arg(start, p->vararg, &what->pos, p->normal);
> > -			arglist = arglist->next->next;
> >  			if (eof_token(next))
> >  				goto Eclosing;
> > +			if (p->vararg && wanted == 1 && eof_token(start->next))
> > +				break;
> > +			arglist = arglist->next->next;
> >  			args[count].arg = start->next;
> >  			args[count].n_normal = p->normal;
> >  			args[count].n_quoted = p->quoted;
> 
> This looks plausible; we should also add a test for it, though.

throw this in, perhaps?

diff --git a/validation/preprocessor/preprocessor23.c b/validation/preprocessor/preprocessor23.c
index 25be508..a778483 100644
--- a/validation/preprocessor/preprocessor23.c
+++ b/validation/preprocessor/preprocessor23.c
@@ -12,6 +12,9 @@ I(,)
 I(x,)
 I(,x)
 I(x,x)
+#define J(...) ,##__VA_ARGS__
+J()
+J(x)
 /*
  * check-name: Preprocessor #23
  * check-command: sparse -E $file
@@ -29,6 +32,7 @@ I(x,x)
 ,x
 ,x
 ,xx
+,x
  * check-output-end
  *
  * check-error-start

Re: Sparse preprocessing bug with zero-arg variadic macros

[Christopher Li]

On Thu, Aug 31, 2017 at 5:48 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> throw this in, perhaps?
>
> diff --git a/validation/preprocessor/preprocessor23.c b/validation/preprocessor/preprocessor23.c
> index 25be508..a778483 100644

This one does not have a signed off.
I will combine it with your previous patch.

I have test with your patch.
It fixes the simplified test example Linus give out.
I also do the stress test on allmodconfig kernel
source. There is no impact on sparse warning
given on kernel source. The stress test timing
is close enough.

I already apply the patch. I will push to master soon.

Thanks for the quick reply to fix this bug.

Chris