introduce Wdirective_within_macro

Commit Message

Oleg Nesterov March 12, 2020, 3:09 p.m. UTC

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 <[e-mail]>
---
 cgcc                                     | 2 +-
 lib.c                                    | 2 ++
 lib.h                                    | 1 +
 pre-process.c                            | 5 +++--
 validation/preprocessor/preprocessor22.c | 8 ++++----
 5 files changed, 11 insertions(+), 7 deletions(-)

Comments

Linus Torvalds March 12, 2020, 6:24 p.m. UTC | #1

On Thu, Mar 12, 2020 at 8:09 AM Oleg Nesterov <[e-mail]> 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

Luc Van Oostenryck March 12, 2020, 7:12 p.m. UTC | #2

On Thu, Mar 12, 2020 at 11:24:06AM -0700, Linus Torvalds wrote:
> On Thu, Mar 12, 2020 at 8:09 AM Oleg Nesterov <[e-mail]> 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

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