* [PATCH] introduce Wdirective_within_macro
@ 2020-03-12 15:09 Oleg Nesterov
2020-03-12 18:24 ` Linus Torvalds
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2020-03-12 15:09 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Alexey Gladkov, Dan Carpenter, linux-sparse
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] introduce Wdirective_within_macro
2020-03-12 15:09 [PATCH] introduce Wdirective_within_macro Oleg Nesterov
@ 2020-03-12 18:24 ` Linus Torvalds
2020-03-12 19:12 ` Luc Van Oostenryck
2020-03-12 21:16 ` [PATCH] cpp: silently allow conditional directives within macro Luc Van Oostenryck
0 siblings, 2 replies; 6+ messages in thread
From: Linus Torvalds @ 2020-03-12 18:24 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Luc Van Oostenryck, Alexey Gladkov, Dan Carpenter,
Sparse Mailing-list
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] introduce Wdirective_within_macro
2020-03-12 18:24 ` Linus Torvalds
@ 2020-03-12 19:12 ` Luc Van Oostenryck
2020-03-12 21:16 ` [PATCH] cpp: silently allow conditional directives within macro Luc Van Oostenryck
1 sibling, 0 replies; 6+ messages in thread
From: Luc Van Oostenryck @ 2020-03-12 19:12 UTC (permalink / raw)
To: Linus Torvalds
Cc: Oleg Nesterov, Alexey Gladkov, Dan Carpenter, Sparse Mailing-list
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cpp: silently allow conditional directives within macro
2020-03-12 18:24 ` Linus Torvalds
2020-03-12 19:12 ` Luc Van Oostenryck
@ 2020-03-12 21:16 ` Luc Van Oostenryck
2020-03-13 16:17 ` Oleg Nesterov
1 sibling, 1 reply; 6+ messages in thread
From: Luc Van Oostenryck @ 2020-03-12 21:16 UTC (permalink / raw)
To: linux-sparse; +Cc: Linus Torvalds, Oleg Nesterov, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] cpp: silently allow conditional directives within macro
2020-03-12 21:16 ` [PATCH] cpp: silently allow conditional directives within macro Luc Van Oostenryck
@ 2020-03-13 16:17 ` Oleg Nesterov
2020-03-16 0:46 ` Luc Van Oostenryck
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2020-03-13 16:17 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: linux-sparse, Linus Torvalds
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 {
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cpp: silently allow conditional directives within macro
2020-03-13 16:17 ` Oleg Nesterov
@ 2020-03-16 0:46 ` Luc Van Oostenryck
0 siblings, 0 replies; 6+ messages in thread
From: Luc Van Oostenryck @ 2020-03-16 0:46 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: linux-sparse, Linus Torvalds
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-16 0:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-12 15:09 [PATCH] introduce Wdirective_within_macro Oleg Nesterov
2020-03-12 18:24 ` Linus Torvalds
2020-03-12 19:12 ` Luc Van Oostenryck
2020-03-12 21:16 ` [PATCH] cpp: silently allow conditional directives within macro Luc Van Oostenryck
2020-03-13 16:17 ` Oleg Nesterov
2020-03-16 0:46 ` Luc Van Oostenryck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).