introduce Wdirective_within_macro

PATCH In review Oleg Nesterov 3 participants 6 messages Mar 12, 2020 – Mar 16, 2020 Sparse checker

Share & tools

Oleg Nesterov

author

2020-03-12 15:09 UTC #0 lore raw

PATCH 1/1 introduce Wdirective_within_macro

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 <[email protected]>

---
 cgcc                                     | 2 +-
 lib.c                                    | 2 ++
 lib.h                                    | 1 +
 pre-process.c                            | 5 +++--
 validation/preprocessor/preprocessor22.c | 8 ++++----
 5 files changed, 11 insertions(+), 7 deletions(-)

cgcc +1 -1

index 9c6ad883..9f5897e9 100755
@@ -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)?$/;

lib.c +2 -0

index f15e4d99..264a890e 100644
@@ -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 },

lib.h +1 -0

index 72651cef..49db0117 100644
@@ -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;

pre-process.c +3 -2

index 433d1bf8..e79a447a 100644
@@ -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;

validation/preprocessor/preprocessor22.c +4 -4

index fb28daaa..277334c6 100644
@@ -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

Signature · 1 line

2.25.1.362.g51ebf55

Linus Torvalds

2020-03-12 18:24 UTC #1 lore raw

Quoted 4 lines · 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.

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

2020-03-12 19:13 UTC #2 lore raw

Quoted 11 lines · Linus Torvalds

Quoted 4 lines · 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.
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*

Quoted 8 lines

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

Luc Van Oostenryck

2020-03-12 21:16 UTC #3 lore raw

PATCH 1/1 cpp: silently allow conditional directives within macro

In reply to Linus Torvalds

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 <[email protected]>

---
 pre-process.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

pre-process.c +15 -4

index 6670fb0cd557..4b2cd054f897 100644
@@ -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;

Signature · 1 line

2.25.1

Oleg Nesterov

author

2020-03-13 16:17 UTC #4 lore raw

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

Review static void handle_preprocessor_line(struct stream *stream, struct token **line, · line 2108

Quoted 8 lines · Luc Van Oostenryck

(quoted diff) +3 -0

@@ -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,

Review static void handle_preprocessor_line(struct stream *stream, struct token **line, · line 2111

Quoted 3 lines

(quoted diff) +3 -0

+			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.

validation/preprocessor/preprocessor22.c +0 -7

@@ -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 {

Luc Van Oostenryck

2020-03-16 00:46 UTC #5 lore raw

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

Quoted 10 lines · Oleg Nesterov

Review static void handle_preprocessor_line(struct stream *stream, struct token **line, · line 2108
Quoted 8 lines · Luc Van Oostenryck
(quoted diff) +3 -0
@@ -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)
                    ^^^^^^^^^^^^^^^^^^^^^^^

Quoted 2 lines

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.

Quoted 8 lines

Review static void handle_preprocessor_line(struct stream *stream, struct token **line, · line 2111
Quoted 3 lines
(quoted diff) +3 -0
+			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