[PATCH] introduce Wdirective_within_macro

[PATCH] introduce Wdirective_within_macro

[Oleg Nesterov]

When used on linux kernel, sparse issues a lot of "directive in macro's
argument list" errors, "#if" within a macro invocation is widely used in
the kernel code.

Downgrade this sparse_error() to warning() and add the new
-Wdirective-within-macro knob.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 cgcc                                     | 2 +-
 lib.c                                    | 2 ++
 lib.h                                    | 1 +
 pre-process.c                            | 5 +++--
 validation/preprocessor/preprocessor22.c | 8 ++++----
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/cgcc b/cgcc
index 9c6ad883..9f5897e9 100755
--- a/cgcc
+++ b/cgcc
@@ -127,7 +127,7 @@ exit 0;
 
 sub check_only_option {
     my ($arg) = @_;
-    return 1 if $arg =~ /^-W(no-?)?(address-space|bitwise|cast-to-as|cast-truncate|constant-suffix|context|decl|default-bitfield-sign|designated-init|do-while|enum-mismatch|external-function-has-definition|init-cstring|memcpy-max-count|non-pointer-null|old-initializer|one-bit-signed-bitfield|override-init-all|paren-string|ptr-subtraction-blows|return-void|sizeof-bool|sparse-all|sparse-error|transparent-union|typesign|undef|unknown-attribute)$/;
+    return 1 if $arg =~ /^-W(no-?)?(address-space|bitwise|cast-to-as|cast-truncate|constant-suffix|context|decl|default-bitfield-sign|designated-init|directive-within-macro|do-while|enum-mismatch|external-function-has-definition|init-cstring|memcpy-max-count|non-pointer-null|old-initializer|one-bit-signed-bitfield|override-init-all|paren-string|ptr-subtraction-blows|return-void|sizeof-bool|sparse-all|sparse-error|transparent-union|typesign|undef|unknown-attribute)$/;
     return 1 if $arg =~ /^-v(no-?)?(entry|dead)$/;
     return 1 if $arg =~ /^-f(dump-ir|memcpy-max-count|diagnostic-prefix)(=\S*)?$/;
     return 1 if $arg =~ /^-f(mem2reg|optim)(-enable|-disable|=last)?$/;
diff --git a/lib.c b/lib.c
index f15e4d99..264a890e 100644
--- a/lib.c
+++ b/lib.c
@@ -264,6 +264,7 @@ int Wdecl = 1;
 int Wdeclarationafterstatement = -1;
 int Wdefault_bitfield_sign = 0;
 int Wdesignated_init = 1;
+int Wdirective_within_macro = 1;
 int Wdo_while = 0;
 int Wimplicit_int = 1;
 int Winit_cstring = 0;
@@ -740,6 +741,7 @@ static const struct flag warnings[] = {
 	{ "declaration-after-statement", &Wdeclarationafterstatement },
 	{ "default-bitfield-sign", &Wdefault_bitfield_sign },
 	{ "designated-init", &Wdesignated_init },
+	{ "directive-within-macro", &Wdirective_within_macro },
 	{ "do-while", &Wdo_while },
 	{ "enum-mismatch", &Wenum_mismatch },
 	{ "external-function-has-definition", &Wexternal_function_has_definition },
diff --git a/lib.h b/lib.h
index 72651cef..49db0117 100644
--- a/lib.h
+++ b/lib.h
@@ -153,6 +153,7 @@ extern int Wdecl;
 extern int Wdeclarationafterstatement;
 extern int Wdefault_bitfield_sign;
 extern int Wdesignated_init;
+extern int Wdirective_within_macro;
 extern int Wdo_while;
 extern int Wenum_mismatch;
 extern int Wexternal_function_has_definition;
diff --git a/pre-process.c b/pre-process.c
index 433d1bf8..e79a447a 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -271,8 +271,9 @@ static struct token *collect_arg(struct token *prev, int vararg, struct position
 	while (!eof_token(next = scan_next(p))) {
 		if (next->pos.newline && match_op(next, '#')) {
 			if (!next->pos.noexpand) {
-				sparse_error(next->pos,
-					     "directive in macro's argument list");
+				if (Wdirective_within_macro)
+					warning(next->pos,
+						"directive in macro's argument list");
 				preprocessor_line(stream, p);
 				__free_token(next);	/* Free the '#' token */
 				continue;
diff --git a/validation/preprocessor/preprocessor22.c b/validation/preprocessor/preprocessor22.c
index fb28daaa..277334c6 100644
--- a/validation/preprocessor/preprocessor22.c
+++ b/validation/preprocessor/preprocessor22.c
@@ -20,10 +20,10 @@ define_struct(a, {
  * check-command: sparse -E $file
  *
  * check-error-start
-preprocessor/preprocessor22.c:6:1: error: directive in macro's argument list
-preprocessor/preprocessor22.c:8:1: error: directive in macro's argument list
-preprocessor/preprocessor22.c:10:1: error: directive in macro's argument list
-preprocessor/preprocessor22.c:12:1: error: directive in macro's argument list
+preprocessor/preprocessor22.c:6:1: warning: directive in macro's argument list
+preprocessor/preprocessor22.c:8:1: warning: directive in macro's argument list
+preprocessor/preprocessor22.c:10:1: warning: directive in macro's argument list
+preprocessor/preprocessor22.c:12:1: warning: directive in macro's argument list
  * check-error-end
  *
  * check-output-start
-- 
2.25.1.362.g51ebf55

Re: [PATCH] introduce Wdirective_within_macro

[Linus Torvalds]

On Thu, Mar 12, 2020 at 8:09 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> When used on linux kernel, sparse issues a lot of "directive in macro's
> argument list" errors, "#if" within a macro invocation is widely used in
> the kernel code.

Ack. Downgrading to a warning is a good thing anyway.

I'd even be ok with making the default be "don't warn", and enable
warnings only if explicitly asked for, or perhaps with "pedantic" (not
that I think sparse cares about pedantic right now).

Yes, it's undefined behavior. But sparse does the right thing, and
it's the better thing to do. And it's not like we're necessarily
always particularly pedantic about some other cases.

Now, the example where somebody _redefined_ a macro inside the macro
expansion, that's a different thing. That's just crazy. Maybe we could
make that "directive in macro argument list" thing be a more nuanced
flag?

             Linus

Re: [PATCH] introduce Wdirective_within_macro

[Luc Van Oostenryck]

On Thu, Mar 12, 2020 at 11:24:06AM -0700, Linus Torvalds wrote:
> On Thu, Mar 12, 2020 at 8:09 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > When used on linux kernel, sparse issues a lot of "directive in macro's
> > argument list" errors, "#if" within a macro invocation is widely used in
> > the kernel code.
> 
> Ack. Downgrading to a warning is a good thing anyway.
> 
> I'd even be ok with making the default be "don't warn", and enable
> warnings only if explicitly asked for, or perhaps with "pedantic" (not
> that I think sparse cares about pedantic right now).

*nod*
 
> Yes, it's undefined behavior. But sparse does the right thing, and
> it's the better thing to do. And it's not like we're necessarily
> always particularly pedantic about some other cases.
> 
> Now, the example where somebody _redefined_ a macro inside the macro
> expansion, that's a different thing. That's just crazy. Maybe we could
> make that "directive in macro argument list" thing be a more nuanced
> flag?

Yes, it's what I was thinking too. The #if*/#elif/#else/#endif should be
perfectly safe here. A redefine is indeed crazy, same for a self-#undef
IMO (even if its meaning is better defined and I think it would need a
small change with sym->expansion), #include could make some sense
but probably should be avoided too, like the other directives.

-- Luc

[PATCH] cpp: silently allow conditional directives within macro

[Luc Van Oostenryck]

The presence of preprocessor directives within the arguments
of a macro invocation is Undefined Behaviour [6.10.3p11].
However, conditional directives are harmless here and are
useful (and commonly used in the kernel).

So, relax the warning by restricting it to non-conditional
directives.

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

diff --git a/pre-process.c b/pre-process.c
index 6670fb0cd557..4b2cd054f897 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -48,6 +48,7 @@ static struct ident_list *macros;	// only needed for -dD
 static int false_nesting = 0;
 static int counter_macro = 0;		// __COUNTER__ expansion
 static int include_level = 0;
+static int expanding = 0;
 
 #define INCLUDEPATHS 300
 const char *includepath[INCLUDEPATHS+1] = {
@@ -232,8 +233,13 @@ static int expand_one_symbol(struct token **list)
 		sym->expander(token);
 		return 1;
 	} else {
+		int rc;
+
 		sym->used_in = file_scope;
-		return expand(list, sym);
+		expanding = 1;
+		rc = expand(list, sym);
+		expanding = 0;
+		return rc;
 	}
 }
 
@@ -271,9 +277,6 @@ static struct token *collect_arg(struct token *prev, int vararg, struct position
 	while (!eof_token(next = scan_next(p))) {
 		if (next->pos.newline && match_op(next, '#')) {
 			if (!next->pos.noexpand) {
-				if (Wdirective_within_macro)
-					warning(next->pos,
-						"directive in macro's argument list");
 				preprocessor_line(stream, p);
 				__free_token(next);	/* Free the '#' token */
 				continue;
@@ -2075,6 +2078,7 @@ static void handle_preprocessor_line(struct stream *stream, struct token **line,
 	int (*handler)(struct stream *, struct token **, struct token *);
 	struct token *token = start->next;
 	int is_normal = 1;
+	int is_cond = 0;	// is one of {is,ifdef,ifndef,elif,else,endif}
 
 	if (eof_token(token))
 		return;
@@ -2084,6 +2088,7 @@ static void handle_preprocessor_line(struct stream *stream, struct token **line,
 		if (sym) {
 			handler = sym->handler;
 			is_normal = sym->normal;
+			is_cond = !sym->normal;
 		} else {
 			handler = handle_nondirective;
 		}
@@ -2098,6 +2103,12 @@ static void handle_preprocessor_line(struct stream *stream, struct token **line,
 		if (false_nesting)
 			goto out;
 	}
+
+	if (expanding) {
+		if (Wdirective_within_macro && !is_cond)
+			warning(start->pos, "directive in macro's argument list");
+		expanding = 0;		// warn only once
+	}
 	if (!handler(stream, line, token))	/* all set */
 		return;
 
-- 
2.25.1

Re: [PATCH] cpp: silently allow conditional directives within macro

[Oleg Nesterov]

On 03/12, Luc Van Oostenryck wrote:
>
> @@ -2098,6 +2103,12 @@ static void handle_preprocessor_line(struct stream *stream, struct token **line,
>  		if (false_nesting)
>  			goto out;
>  	}
> +
> +	if (expanding) {
> +		if (Wdirective_within_macro && !is_cond)
                    ^^^^^^^^^^^^^^^^^^^^^^^
I am not sure Wdirective_within_macro is really useful after this change,
I'd suggest to drop my patch but this is up to you.

Anyway,

> +			warning(start->pos, "directive in macro's argument list");
> +		expanding = 0;		// warn only once
> +	}

then you should probably update preprocessor22.c ? See below.

I am not sure about validation/preprocessor/expand-redef.c added by the
previous patch,

	 * check-output-start

	 1 2 1 2
	 * check-output-end

shouldn't you add check-known-to-fail into this file or change the output

	1
	2 1
	2

?

Oleg.


--- a/validation/preprocessor/preprocessor22.c
+++ b/validation/preprocessor/preprocessor22.c
@@ -19,13 +19,6 @@ define_struct(a, {
  *
  * check-command: sparse -E $file
  *
- * check-error-start
-preprocessor/preprocessor22.c:6:1: warning: directive in macro's argument list
-preprocessor/preprocessor22.c:8:1: warning: directive in macro's argument list
-preprocessor/preprocessor22.c:10:1: warning: directive in macro's argument list
-preprocessor/preprocessor22.c:12:1: warning: directive in macro's argument list
- * check-error-end
- *
  * check-output-start
 
 struct {

Re: [PATCH] cpp: silently allow conditional directives within macro

[Luc Van Oostenryck]

On Fri, Mar 13, 2020 at 05:17:48PM +0100, Oleg Nesterov wrote:
> On 03/12, Luc Van Oostenryck wrote:
> >
> > @@ -2098,6 +2103,12 @@ static void handle_preprocessor_line(struct stream *stream, struct token **line,
> >  		if (false_nesting)
> >  			goto out;
> >  	}
> > +
> > +	if (expanding) {
> > +		if (Wdirective_within_macro && !is_cond)
>                     ^^^^^^^^^^^^^^^^^^^^^^^
> I am not sure Wdirective_within_macro is really useful after this change,
> I'd suggest to drop my patch but this is up to you.

Yes, it makes much less sense now. I've replaced that by supporting
-pedantic.
 
> > +			warning(start->pos, "directive in macro's argument list");
> > +		expanding = 0;		// warn only once
> > +	}
> 
> then you should probably update preprocessor22.c ? See below.
> 
> I am not sure about validation/preprocessor/expand-redef.c added by the
> previous patch,

Yes, I hadn't updated the testcases because the patch just a RFC
to see if just allowing the #if/... was OK (but I forgot to mark
is as such) and they also depended on the previous RFC patch about
the expansion of newline.

Series updated and pushed now.
Thanks for the review.

-- Luc