* C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) @ 2017-06-08 16:49 Marek Polacek 2017-06-08 17:24 ` David Malcolm 2017-06-10 0:47 ` Jason Merrill 0 siblings, 2 replies; 13+ messages in thread From: Marek Polacek @ 2017-06-08 16:49 UTC (permalink / raw) To: GCC Patches, Joseph Myers, David Malcolm, Martin Sebor, Jason Merrill This is the hopefully last incarnation of the patch. The change from the last time[0] is simpy that I've added a new test and the warning has been renamed to -Wmultistatement-macros. David - any another comments? Joseph - how about the C parts? Jason - how about the C++ parts? Thanks, [0] https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00458.html Bootstrapped/regtested on x86_64-linux, ok for trunk? 2017-06-08 Marek Polacek <polacek@redhat.com> PR c/80116 * c-common.h (warn_for_multistatement_macros): Declare. * c-warn.c (warn_for_multistatement_macros): New function. * c.opt (Wmultistatement-macros): New option. * c-parser.c (c_parser_if_body): Set the location of the body of the conditional after parsing all the labels. Call warn_for_multistatement_macros. (c_parser_else_body): Likewise. (c_parser_switch_statement): Likewise. (c_parser_while_statement): Likewise. (c_parser_for_statement): Likewise. (c_parser_statement): Add a default argument. Save the location after labels have been parsed. (c_parser_c99_block_statement): Likewise. * parser.c (cp_parser_statement): Add a default argument. Save the location of the expression-statement after labels have been parsed. (cp_parser_implicitly_scoped_statement): Set the location of the body of the conditional after parsing all the labels. Call warn_for_multistatement_macros. (cp_parser_already_scoped_statement): Likewise. * doc/invoke.texi: Document -Wmultistatement-macros. * c-c++-common/Wmultistatement-macros-1.c: New test. * c-c++-common/Wmultistatement-macros-2.c: New test. * c-c++-common/Wmultistatement-macros-3.c: New test. * c-c++-common/Wmultistatement-macros-4.c: New test. * c-c++-common/Wmultistatement-macros-5.c: New test. * c-c++-common/Wmultistatement-macros-6.c: New test. * c-c++-common/Wmultistatement-macros-7.c: New test. * c-c++-common/Wmultistatement-macros-8.c: New test. * c-c++-common/Wmultistatement-macros-9.c: New test. * c-c++-common/Wmultistatement-macros-10.c: New test. * c-c++-common/Wmultistatement-macros-11.c: New test. diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h index 79072e6..686932c 100644 --- gcc/c-family/c-common.h +++ gcc/c-family/c-common.h @@ -1539,6 +1539,8 @@ extern bool maybe_warn_shift_overflow (location_t, tree, tree); extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **); extern bool diagnose_mismatched_attributes (tree, tree); extern tree do_warn_duplicated_branches_r (tree *, int *, void *); +extern void warn_for_multistatement_macros (location_t, location_t, + location_t); /* In c-attribs.c. */ extern bool attribute_takes_identifier_p (const_tree); diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c index 35321a6..d883330 100644 --- gcc/c-family/c-warn.c +++ gcc/c-family/c-warn.c @@ -2401,3 +2401,91 @@ do_warn_duplicated_branches_r (tree *tp, int *, void *) do_warn_duplicated_branches (*tp); return NULL_TREE; } + +/* Implementation of -Wmultistatement-macros. This warning warns about + cases when a macro expands to multiple statements not wrapped in + do {} while (0) or ({ }) and is used as a body of if/else/for/while + conditionals. For example, + + #define DOIT x++; y++ + + if (c) + DOIT; + + will increment y unconditionally. + + BODY_LOC is the location of the first token in the body after labels + have been parsed, NEXT_LOC is the location of the next token after the + body of the conditional has been parsed, and GUARD_LOC is the location + of the conditional. */ + +void +warn_for_multistatement_macros (location_t body_loc, location_t next_loc, + location_t guard_loc) +{ + if (!warn_multistatement_macros) + return; + + /* Ain't got time to waste. We only care about macros here. */ + if (!from_macro_expansion_at (body_loc) + || !from_macro_expansion_at (next_loc)) + return; + + /* Let's skip macros defined in system headers. */ + if (in_system_header_at (body_loc) + || in_system_header_at (next_loc)) + return; + + /* Find the actual tokens in the macro definition. BODY_LOC and + NEXT_LOC have to come from the same spelling location, but they + will resolve to different locations in the context of the macro + definition. */ + location_t body_loc_exp + = linemap_resolve_location (line_table, body_loc, + LRK_MACRO_DEFINITION_LOCATION, NULL); + location_t next_loc_exp + = linemap_resolve_location (line_table, next_loc, + LRK_MACRO_DEFINITION_LOCATION, NULL); + location_t guard_loc_exp + = linemap_resolve_location (line_table, guard_loc, + LRK_MACRO_DEFINITION_LOCATION, NULL); + + /* These are some funky cases we don't want to warn about. */ + if (body_loc_exp == guard_loc_exp + || next_loc_exp == guard_loc_exp + || body_loc_exp == next_loc_exp) + return; + + /* Find the macro map for the macro expansion BODY_LOC. */ + const line_map *map = linemap_lookup (line_table, body_loc); + const line_map_macro *macro_map = linemap_check_macro (map); + + /* Now see if the following token is coming from the same macro + expansion. If it is, it's a problem, because it should've been + parsed at this point. We only look at odd-numbered indexes + within the MACRO_MAP_LOCATIONS array, i.e. the spelling locations + of the tokens. */ + bool found_guard = false; + bool found_next = false; + for (unsigned int i = 1; + i < 2 * MACRO_MAP_NUM_MACRO_TOKENS (macro_map); + i += 2) + { + if (MACRO_MAP_LOCATIONS (macro_map)[i] == next_loc_exp) + found_next = true; + if (MACRO_MAP_LOCATIONS (macro_map)[i] == guard_loc_exp) + found_guard = true; + } + + /* The conditional itself must not come from the same expansion, because + we don't want to warn about + #define IF if (x) x++; y++ + and similar. */ + if (!found_next || found_guard) + return; + + if (warning_at (body_loc, OPT_Wmultistatement_macros, + "macro expands to multiple statements")) + inform (guard_loc, "some parts of macro expansion are not guarded by " + "this conditional"); +} diff --git gcc/c-family/c.opt gcc/c-family/c.opt index 37bb236..9dbe211 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -698,6 +698,10 @@ Wmissing-field-initializers C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning EnabledBy(Wextra) Warn about missing fields in struct initializers. +Wmultistatement-macros +C ObjC C++ ObjC++ Var(warn_multistatement_macros) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) +Warn about macros expanding to multiple statements in a body of a conditional such as if, else, while, or for. + Wmultiple-inheritance C++ ObjC++ Var(warn_multiple_inheritance) Warning Warn on direct multiple inheritance. diff --git gcc/c/c-parser.c gcc/c/c-parser.c index 6f954f2..74e8698 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -1218,9 +1218,11 @@ static void c_parser_initval (c_parser *, struct c_expr *, static tree c_parser_compound_statement (c_parser *); static void c_parser_compound_statement_nostart (c_parser *); static void c_parser_label (c_parser *); -static void c_parser_statement (c_parser *, bool *); +static void c_parser_statement (c_parser *, bool *, location_t * = NULL); static void c_parser_statement_after_labels (c_parser *, bool *, vec<tree> * = NULL); +static tree c_parser_c99_block_statement (c_parser *, bool *, + location_t * = NULL); static void c_parser_if_statement (c_parser *, bool *, vec<tree> *); static void c_parser_switch_statement (c_parser *, bool *); static void c_parser_while_statement (c_parser *, bool, bool *); @@ -5204,9 +5206,11 @@ c_parser_label (c_parser *parser) implement -Wparentheses. */ static void -c_parser_statement (c_parser *parser, bool *if_p) +c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels) { c_parser_all_labels (parser); + if (loc_after_labels) + *loc_after_labels = c_parser_peek_token (parser)->location; c_parser_statement_after_labels (parser, if_p, NULL); } @@ -5466,11 +5470,12 @@ c_parser_paren_condition (c_parser *parser) implement -Wparentheses. */ static tree -c_parser_c99_block_statement (c_parser *parser, bool *if_p) +c_parser_c99_block_statement (c_parser *parser, bool *if_p, + location_t *loc_after_labels) { tree block = c_begin_compound_stmt (flag_isoc99); location_t loc = c_parser_peek_token (parser)->location; - c_parser_statement (parser, if_p); + c_parser_statement (parser, if_p, loc_after_labels); return c_end_compound_stmt (loc, block, flag_isoc99); } @@ -5492,6 +5497,7 @@ c_parser_if_body (c_parser *parser, bool *if_p, { tree block = c_begin_compound_stmt (flag_isoc99); location_t body_loc = c_parser_peek_token (parser)->location; + location_t body_loc_after_labels = UNKNOWN_LOCATION; token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); @@ -5508,11 +5514,19 @@ c_parser_if_body (c_parser *parser, bool *if_p, else if (c_parser_next_token_is (parser, CPP_OPEN_BRACE)) add_stmt (c_parser_compound_statement (parser)); else - c_parser_statement_after_labels (parser, if_p); + { + body_loc_after_labels = c_parser_peek_token (parser)->location; + c_parser_statement_after_labels (parser, if_p); + } token_indent_info next_tinfo = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (if_tinfo, body_tinfo, next_tinfo); + if (body_loc_after_labels != UNKNOWN_LOCATION + && next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (body_loc_after_labels, + next_tinfo.location, + if_tinfo.location); return c_end_compound_stmt (body_loc, block, flag_isoc99); } @@ -5530,6 +5544,7 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo, tree block = c_begin_compound_stmt (flag_isoc99); token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); + location_t body_loc_after_labels = UNKNOWN_LOCATION; c_parser_all_labels (parser); if (c_parser_next_token_is (parser, CPP_SEMICOLON)) @@ -5542,11 +5557,19 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo, c_parser_consume_token (parser); } else - c_parser_statement_after_labels (parser, NULL, chain); + { + body_loc_after_labels = c_parser_peek_token (parser)->location; + c_parser_statement_after_labels (parser, NULL, chain); + } token_indent_info next_tinfo = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (else_tinfo, body_tinfo, next_tinfo); + if (body_loc_after_labels != UNKNOWN_LOCATION + && next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (body_loc_after_labels, + next_tinfo.location, + else_tinfo.location); return c_end_compound_stmt (body_loc, block, flag_isoc99); } @@ -5732,7 +5755,12 @@ c_parser_switch_statement (c_parser *parser, bool *if_p) c_start_case (switch_loc, switch_cond_loc, expr, explicit_cast_p); save_break = c_break_label; c_break_label = NULL_TREE; - body = c_parser_c99_block_statement (parser, if_p); + location_t loc_after_labels; + bool open_brace_p = c_parser_peek_token (parser)->type == CPP_OPEN_BRACE; + body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels); + location_t next_loc = c_parser_peek_token (parser)->location; + if (!open_brace_p && c_parser_peek_token (parser)->type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, next_loc, switch_loc); c_finish_case (body, ce.original_type); if (c_break_label) { @@ -5783,7 +5811,8 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p) token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); - body = c_parser_c99_block_statement (parser, if_p); + location_t loc_after_labels; + body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels); c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true); add_stmt (c_end_compound_stmt (loc, block, flag_isoc99)); c_parser_maybe_reclassify_token (parser); @@ -5792,6 +5821,10 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p) = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo); + if (next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, next_tinfo.location, + while_tinfo.location); + c_break_label = save_break; c_cont_label = save_cont; } @@ -6076,7 +6109,8 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p) token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); - body = c_parser_c99_block_statement (parser, if_p); + location_t loc_after_labels; + body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels); if (is_foreach_statement) objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label); @@ -6089,6 +6123,10 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p) = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (for_tinfo, body_tinfo, next_tinfo); + if (next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, next_tinfo.location, + for_tinfo.location); + c_break_label = save_break; c_cont_label = save_cont; } diff --git gcc/cp/parser.c gcc/cp/parser.c index 891341d..25933bb 100644 --- gcc/cp/parser.c +++ gcc/cp/parser.c @@ -2102,7 +2102,7 @@ static void cp_parser_lambda_body /* Statements [gram.stmt.stmt] */ static void cp_parser_statement - (cp_parser *, tree, bool, bool *, vec<tree> * = NULL); + (cp_parser *, tree, bool, bool *, vec<tree> * = NULL, location_t * = NULL); static void cp_parser_label_for_labeled_statement (cp_parser *, tree); static tree cp_parser_expression_statement @@ -10531,7 +10531,8 @@ cp_parser_lambda_body (cp_parser* parser, tree lambda_expr) static void cp_parser_statement (cp_parser* parser, tree in_statement_expr, - bool in_compound, bool *if_p, vec<tree> *chain) + bool in_compound, bool *if_p, vec<tree> *chain, + location_t *loc_after_labels) { tree statement, std_attrs = NULL_TREE; cp_token *token; @@ -10724,6 +10725,10 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr, if (cp_parser_parse_definitely (parser)) return; } + /* All preceding labels have been parsed at this point. */ + if (loc_after_labels != NULL) + *loc_after_labels = statement_location; + /* Look for an expression-statement instead. */ statement = cp_parser_expression_statement (parser, in_statement_expr); @@ -12264,6 +12269,7 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p, { tree statement; location_t body_loc = cp_lexer_peek_token (parser->lexer)->location; + location_t body_loc_after_labels = UNKNOWN_LOCATION; token_indent_info body_tinfo = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); @@ -12293,7 +12299,8 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p, /* Create a compound-statement. */ statement = begin_compound_stmt (0); /* Parse the dependent-statement. */ - cp_parser_statement (parser, NULL_TREE, false, if_p, chain); + cp_parser_statement (parser, NULL_TREE, false, if_p, chain, + &body_loc_after_labels); /* Finish the dummy compound-statement. */ finish_compound_stmt (statement); } @@ -12302,6 +12309,12 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p, = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo); + if (body_loc_after_labels != UNKNOWN_LOCATION + && next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (body_loc_after_labels, + next_tinfo.location, + guard_tinfo.location); + /* Return the statement. */ return statement; } @@ -12320,11 +12333,18 @@ cp_parser_already_scoped_statement (cp_parser* parser, bool *if_p, { token_indent_info body_tinfo = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); + location_t loc_after_labels; - cp_parser_statement (parser, NULL_TREE, false, if_p); + cp_parser_statement (parser, NULL_TREE, false, if_p, NULL, + &loc_after_labels); token_indent_info next_tinfo = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo); + + if (next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, + next_tinfo.location, + guard_tinfo.location); } else { diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index c116882..2fe16dd 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -293,7 +293,7 @@ Objective-C and Objective-C++ Dialects}. -Wmain -Wmaybe-uninitialized -Wmemset-elt-size -Wmemset-transposed-args @gol -Wmisleading-indentation -Wmissing-braces @gol -Wmissing-field-initializers -Wmissing-include-dirs @gol --Wno-multichar -Wnonnull -Wnonnull-compare @gol +-Wno-multichar -Wmultistatement-macros -Wnonnull -Wnonnull-compare @gol -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol -Wnull-dereference -Wodr -Wno-overflow -Wopenmp-simd @gol -Woverride-init-side-effects -Woverlength-strings @gol @@ -3824,6 +3824,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wmemset-transposed-args @gol -Wmisleading-indentation @r{(only for C/C++)} @gol -Wmissing-braces @r{(only for C/ObjC)} @gol +-Wmultistatement-macros @gol -Wnarrowing @r{(only for C++)} @gol -Wnonnull @gol -Wnonnull-compare @gol @@ -4496,6 +4497,29 @@ This warning is enabled by @option{-Wall}. @opindex Wno-missing-include-dirs Warn if a user-supplied include directory does not exist. +@item -Wmultistatement-macros +@opindex Wmultistatement-macros +@opindex Wno-multistatement-macros +Warn about macros expanding to multiple statements in a body of a conditional, +such as @code{if}, @code{else}, @code{for}, or @code{while}. +For example: + +@smallexample +#define DOIT x++; y++ +if (c) + DOIT; +@end smallexample + +will increment @code{y} unconditionally, not just when @code{c} holds. +The can usually be fixed by wrapping the macro in a do-while loop: +@smallexample +#define DOIT do @{ x++; y++; @} while (0) +if (c) + DOIT; +@end smallexample + +This warning is enabled by @option{-Wall} in C and C++. + @item -Wparentheses @opindex Wparentheses @opindex Wno-parentheses diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c index e69de29..89899ed 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c @@ -0,0 +1,118 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(X, Y) \ + tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \ + X = Y; \ + Y = tmp + +#define STUFF \ + if (0) x = y + +#define STUFF2 \ + if (0) x = y; x++ + +#define STUFF3 \ + if (x) /* { dg-message "not guarded" } */ \ + SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define SET(X, Y) \ + (X) = (Y) + +#define STUFF4 \ + if (x) \ + SET(x, y); \ + SET(x, y) + +#define STUFF5 \ + { tmp = x; x = y; } + +#define STUFF6 \ + x++;; + +int x, y, tmp; + +void +fn1 (void) +{ + if (x) /* { dg-message "not guarded" } */ + SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */ +} + +void +fn2 (void) +{ + SWAP(x, y); +} + +void +fn3 (void) +{ + if (x) + { + SWAP(x, y); + } +} + +void +fn4 (void) +{ + if (x) + ({ x = 10; x++; }); +} + +void +fn5 (void) +{ + if (x) /* { dg-message "not guarded" } */ +L1: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L1; +} + +void +fn6 (void) +{ + if (x) + SET (x, y); + SET (tmp, x); +} + +void +fn7 (void) +{ + STUFF; +} + +void +fn8 (void) +{ + STUFF2; +} + +void +fn9 (void) +{ + STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */ +} + +void +fn10 (void) +{ + STUFF4; +} + +void +fn11 (void) +{ + if (x) + STUFF5; +} + +void +fn12 (void) +{ + if (x) + STUFF6; +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c index e69de29..f91e930 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c @@ -0,0 +1,82 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(x, y) \ + tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \ + x = y; \ + y = tmp + +#define M1 \ + switch (x) /* { dg-message "not guarded" } */ \ + case 1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define M2 \ + switch (x) \ + case 1: \ + x++ + +#define M3 \ + switch (x) \ + case 1: \ + x++;; + +#define M4 \ + switch (x) /* { dg-message "not guarded" } */ \ +L1: \ + case 1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define INC \ + x++;; + +int x, y, tmp; + +void +fn0 (void) +{ + switch (x) /* { dg-message "not guarded" } */ + case 1: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + + switch (x) /* { dg-message "not guarded" } */ + case 1: + case 2: + case 3: + case 4: + case 5: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ +} + +void +fn1 (void) +{ + M1; /* { dg-message "in expansion of macro .M1." } */ + M2; + M3; + M4; /* { dg-message "in expansion of macro .M4." } */ + goto L1; +} + +void +fn2 (void) +{ + switch (x) + case 1: + INC + + switch (x) + case 1: + ({ x = 10; x++; }); +} + +void +fn3 (void) +{ + switch (x) + { + case 1: + SWAP (x, y); + } +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c index e69de29..4f4a123 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c @@ -0,0 +1,19 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +enum E { A, B }; + +const char * +foo (enum E e) +{ +#define CASE(X) case X: return #X + switch (e) + { + CASE (A); + CASE (B); + default: + return "<unknown>"; + } +#undef CASE +}; diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c index e69de29..08bbff4d 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c @@ -0,0 +1,137 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(X, Y) \ + tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \ + X = Y; \ + Y = tmp + +#define STUFF \ + if (0) {} else x = y + +#define STUFF2 \ + if (0) {} else x = y; x++ + +#define STUFF3 \ + if (x) \ + {} \ + else /* { dg-message "not guarded" } */ \ + SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define SET(X, Y) \ + (X) = (Y) + +#define STUFF4 \ + if (x) \ + {} \ + else \ + SET(x, y); \ + SET(x, y) + +#define STUFF5 \ + { tmp = x; x = y; } + +#define STUFF6 \ + x++;; + +int x, y, tmp; + +void +fn1 (void) +{ + if (x) + { + } + else /* { dg-message "not guarded" } */ + SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */ +} + +void +fn2 (void) +{ + SWAP(x, y); +} + +void +fn3 (void) +{ + if (x) + { + } + else + { + SWAP(x, y); + } +} + +void +fn4 (void) +{ + if (x) + { + } + else + ({ x = 10; x++; }); +} + +void +fn5 (void) +{ + if (x) + { + } + else /* { dg-message "not guarded" } */ +L1: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L1; +} + +void +fn6 (void) +{ + if (x) + { + } + else + SET (x, y); + SET (tmp, x); +} + +void +fn7 (void) +{ + STUFF; +} + +void +fn8 (void) +{ + STUFF2; +} + +void +fn9 (void) +{ + STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */ +} + +void +fn10 (void) +{ + STUFF4; +} + +void +fn11 (void) +{ + if (x) + STUFF5; +} + +void +fn12 (void) +{ + if (x) + STUFF6; +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c index e69de29..d130a35 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c @@ -0,0 +1,12 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define CHECK(X) if (!(X)) __builtin_abort () + +void +fn (int i) +{ + CHECK (i == 1); + CHECK (i == 2); +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c index e69de29..e5cc9c3 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c @@ -0,0 +1,14 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define FN(C) \ + void \ + fn (void) \ + { \ + C; \ + } + +int i; + +FN (if (i) ++i) diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c index e69de29..03461f8 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c @@ -0,0 +1,18 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define M(N) \ +L ## N: \ + x++; x++ /* { dg-warning "macro expands to multiple statements" } */ + +int x, y, tmp; + +void +fn1 (void) +{ + if (x) /* { dg-message "not guarded" } */ + M (0); /* { dg-message "in expansion of macro .M." } */ + if (x) /* { dg-message "not guarded" } */ + M (1); /* { dg-message "in expansion of macro .M." } */ +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c index e69de29..5ec9cd9 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c @@ -0,0 +1,22 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define M \ + if (x) x++; x++ + +void +f (int x) +{ + M; + M; + M; + M; + M; + M; + M; + M; + M; + M; + M; +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c index e69de29..d661f14 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c @@ -0,0 +1,18 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(X, Y) \ + tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \ + X = Y; \ + Y = tmp + +#define BODY_AND_IF(COND, X, Y) \ + if (COND) SWAP (X, Y) /* { dg-message "in expansion of macro .SWAP." } */ + +void +fn (int x, int y) +{ + int tmp; + BODY_AND_IF (1, x, y); /* { dg-message "in expansion of macro .BODY_AND_IF." } */ +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c index e69de29..33ffc0a 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c @@ -0,0 +1,64 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(x, y) \ + tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \ + x = y; \ + y = tmp + +#define M1 \ + for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define M2 \ + for (i = 0; i < 1; ++i) \ + x++ + +#define M3 \ + for (i = 0; i < 1; ++i) \ + x++;; + +#define M4 \ + for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ \ +L1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define INC \ + x++;; + +int x, y, tmp; + +void +fn0 (void) +{ + int i; + for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + + for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ +L: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L; +} + +void +fn1 (void) +{ + int i; + M1; /* { dg-message "in expansion of macro .M1." } */ + M2; + M3; + M4; /* { dg-message "in expansion of macro .M4." } */ + goto L1; +} + +void +fn2 (void) +{ + for (int i = 0; i < 1; ++i) + INC + + for (int i = 0; i < 1; ++i) + ({ x = 10; x++; }); +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c index e69de29..167ba03 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c @@ -0,0 +1,62 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(x, y) \ + tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \ + x = y; \ + y = tmp + +#define M1 \ + while (x) /* { dg-message "not guarded" } */ \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define M2 \ + while (x) \ + x++ + +#define M3 \ + while (x) \ + x++;; + +#define M4 \ + while (x) /* { dg-message "not guarded" } */ \ +L1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define INC \ + x++;; + +int x, y, tmp; + +void +fn0 (void) +{ + while (x) /* { dg-message "not guarded" } */ + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + + while (x) /* { dg-message "not guarded" } */ +L: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L; +} + +void +fn1 (void) +{ + M1; /* { dg-message "in expansion of macro .M1." } */ + M2; + M3; + M4; /* { dg-message "in expansion of macro .M4." } */ + goto L1; +} + +void +fn2 (void) +{ + while (x) + INC + + while (x) + ({ x = 10; x++; }); +} Marek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-08 16:49 C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) Marek Polacek @ 2017-06-08 17:24 ` David Malcolm 2017-06-08 18:10 ` Martin Sebor 2017-06-13 10:05 ` Marek Polacek 2017-06-10 0:47 ` Jason Merrill 1 sibling, 2 replies; 13+ messages in thread From: David Malcolm @ 2017-06-08 17:24 UTC (permalink / raw) To: Marek Polacek, GCC Patches, Joseph Myers, Martin Sebor, Jason Merrill On Thu, 2017-06-08 at 18:49 +0200, Marek Polacek wrote: > This is the hopefully last incarnation of the patch. The change from > the > last time[0] is simpy that I've added a new test and the warning has > been > renamed to -Wmultistatement-macros. > > David - any another comments? Thanks for working on this; looks useful. The new name is more accurate, but is rather long; oh well. As part of -Wall, users won't typically have to type it, so that's OK. [...] > diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c > index 35321a6..d883330 100644 > --- gcc/c-family/c-warn.c > +++ gcc/c-family/c-warn.c [...] > + if (warning_at (body_loc, OPT_Wmultistatement_macros, > + "macro expands to multiple statements")) > + inform (guard_loc, "some parts of macro expansion are not > guarded by " > + "this conditional"); Is the guard necessarily a "conditional"? I take a "conditional" to mean an "if"; the guard could be a "for" or a "while" (or an "else", which still seems something of a stretch to me to call a "conditional"). Suggestion: word "this conditional" as "this %qs clause" and either (a) rework the code in c-indentation.c's guard_tinfo_to_string so that it's shared between these two warnings (i.e. to go from a RID_ to a const char *), or (b) just pass in a const char * identifying the guard clause token. > diff --git gcc/c-family/c.opt gcc/c-family/c.opt > index 37bb236..9dbe211 100644 > --- gcc/c-family/c.opt > +++ gcc/c-family/c.opt > @@ -698,6 +698,10 @@ Wmissing-field-initializers > C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning > EnabledBy(Wextra) > Warn about missing fields in struct initializers. > > +Wmultistatement-macros > +C ObjC C++ ObjC++ Var(warn_multistatement_macros) Warning > LangEnabledBy(C ObjC C++ ObjC++,Wall) > +Warn about macros expanding to multiple statements in a body of a > conditional such as if, else, while, or for. Likewise; is "conditional" the right word here? Also, whether of not the statements are actually "in" the body of the guard is the issue here. How about: "Warn about unsafe multiple statement macros that appear to be guarded by a clause such as if, else, while, or for, in which only the first statement is actually guarded after the macro is expanded." or somesuch? > diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi > index c116882..2fe16dd 100644 > --- gcc/doc/invoke.texi > +++ gcc/doc/invoke.texi > @@ -4496,6 +4497,29 @@ This warning is enabled by @option{-Wall}. > @opindex Wno-missing-include-dirs > Warn if a user-supplied include directory does not exist. > > +@item -Wmultistatement-macros > +@opindex Wmultistatement-macros > +@opindex Wno-multistatement-macros > +Warn about macros expanding to multiple statements in a body of a > conditional, > +such as @code{if}, @code{else}, @code{for}, or @code{while}. (as above). > +For example: > + > +@smallexample > +#define DOIT x++; y++ > +if (c) > + DOIT; > +@end smallexample > + > +will increment @code{y} unconditionally, not just when @code{c} > holds. > +The can usually be fixed by wrapping the macro in a do-while loop: > +@smallexample > +#define DOIT do @{ x++; y++; @} while (0) > +if (c) > + DOIT; > +@end smallexample > + > +This warning is enabled by @option{-Wall} in C and C++. > + > @item -Wparentheses > @opindex Wparentheses > @opindex Wno-parentheses Hope this is constructive Dave ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-08 17:24 ` David Malcolm @ 2017-06-08 18:10 ` Martin Sebor 2017-06-09 22:03 ` Gerald Pfeifer 2017-06-13 10:05 ` Marek Polacek 1 sibling, 1 reply; 13+ messages in thread From: Martin Sebor @ 2017-06-08 18:10 UTC (permalink / raw) To: David Malcolm, Marek Polacek, GCC Patches, Joseph Myers, Martin Sebor, Jason Merrill On 06/08/2017 11:24 AM, David Malcolm wrote: > On Thu, 2017-06-08 at 18:49 +0200, Marek Polacek wrote: >> This is the hopefully last incarnation of the patch. The change from >> the >> last time[0] is simpy that I've added a new test and the warning has >> been >> renamed to -Wmultistatement-macros. >> >> David - any another comments? > > Thanks for working on this; looks useful. > > The new name is more accurate, but is rather long; oh well. As part of > -Wall, users won't typically have to type it, so that's OK. > > [...] > >> diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c >> index 35321a6..d883330 100644 >> --- gcc/c-family/c-warn.c >> +++ gcc/c-family/c-warn.c > [...] > >> + if (warning_at (body_loc, OPT_Wmultistatement_macros, >> + "macro expands to multiple statements")) >> + inform (guard_loc, "some parts of macro expansion are not >> guarded by " >> + "this conditional"); > > Is the guard necessarily a "conditional"? I take a "conditional" to > mean an "if"; the guard could be a "for" or a "while" (or an "else", > which still seems something of a stretch to me to call a > "conditional"). > > Suggestion: word "this conditional" as "this %qs clause" and either (a) > rework the code in c-indentation.c's guard_tinfo_to_string so that it's > shared between these two warnings (i.e. to go from a RID_ to a const > char *), or (b) just pass in a const char * identifying the guard > clause token. ... > > Likewise; is "conditional" the right word here? Also, whether of not > the statements are actually "in" the body of the guard is the issue > here. > > How about: > > "Warn about unsafe multiple statement macros that appear to be guarded > by a clause such as if, else, while, or for, in which only the first > statement is actually guarded after the macro is expanded." > > or somesuch? FWIW, I agree with David that "conditional" isn't entirely accurate. At the same time, referring to any of do, for, if, or switch as clauses isn't quite precise either(*). In the C language they are the names of selection and iteration statements, and what follows is called the controlling expression (with for being special) and the next thing is a substatement. I think many people will informally call the two a condition or conditional and the body. I don't have strong feelings about the current wording but if it should be tweaked for accuracy I would suggest to use the formal term "controlling expression", similarly to -Wswitch-unreachable. Martin PS [*] To be completely pedantic, the word clause in the C and C++ standards has a precise meaning: it refers to a chapter of the text (such as Scope, Conformance, Language, etc.) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-08 18:10 ` Martin Sebor @ 2017-06-09 22:03 ` Gerald Pfeifer 2017-06-13 13:46 ` Marek Polacek 2017-07-17 8:15 ` Gerald Pfeifer 0 siblings, 2 replies; 13+ messages in thread From: Gerald Pfeifer @ 2017-06-09 22:03 UTC (permalink / raw) To: David Malcolm, Martin Sebor Cc: Marek Polacek, GCC Patches, Joseph Myers, Martin Sebor, Jason Merrill On Thu, 8 Jun 2017, David Malcolm wrote: > How about: > > "Warn about unsafe multiple statement macros that appear to be guarded > by a clause such as if, else, while, or for, in which only the first > statement is actually guarded after the macro is expanded." > > or somesuch? Yes, I like this. On Thu, 8 Jun 2017, Martin Sebor wrote: > I don't have strong feelings about the current wording but if it > should be tweaked for accuracy I would suggest to use the formal > term "controlling expression", similarly to -Wswitch-unreachable. That sounds good to me. Some comments on the original patch: +Warn about macros expanding to multiple statements in a body of a conditional, +such as @code{if}, @code{else}, @code{for}, or @code{while}. "in the body of a $WHATEVER_WE_SHALL_CALL_IT" +The can usually be fixed by wrapping the macro in a do-while loop: Is there a particular reason for not using an if(1) { } statement? Ah, of course, a following else statement would be impacted by that. Do we want to note that in the documentation? +This warning is enabled by @option{-Wall} in C and C++. "for C and C++" instead of "in"? I'm curious to see how many issues this is going to find in real-world code out there! Gerald ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-09 22:03 ` Gerald Pfeifer @ 2017-06-13 13:46 ` Marek Polacek 2017-07-17 8:15 ` Gerald Pfeifer 1 sibling, 0 replies; 13+ messages in thread From: Marek Polacek @ 2017-06-13 13:46 UTC (permalink / raw) To: Gerald Pfeifer Cc: David Malcolm, Martin Sebor, GCC Patches, Joseph Myers, Martin Sebor, Jason Merrill On Sat, Jun 10, 2017 at 12:03:18AM +0200, Gerald Pfeifer wrote: > On Thu, 8 Jun 2017, David Malcolm wrote: > > How about: > > > > "Warn about unsafe multiple statement macros that appear to be guarded > > by a clause such as if, else, while, or for, in which only the first > > statement is actually guarded after the macro is expanded." > > > > or somesuch? > > Yes, I like this. > > On Thu, 8 Jun 2017, Martin Sebor wrote: > > I don't have strong feelings about the current wording but if it > > should be tweaked for accuracy I would suggest to use the formal > > term "controlling expression", similarly to -Wswitch-unreachable. > > That sounds good to me. > > Some comments on the original patch: > > +Warn about macros expanding to multiple statements in a body of a conditional, > +such as @code{if}, @code{else}, @code{for}, or @code{while}. > > "in the body of a $WHATEVER_WE_SHALL_CALL_IT" It now says something other than that, so that mistake is not there anymore. > +The can usually be fixed by wrapping the macro in a do-while loop: > > Is there a particular reason for not using an if(1) { } statement? > > Ah, of course, a following else statement would be impacted by that. > Do we want to note that in the documentation? I don't think so, we only suggest do {} while (0);. > +This warning is enabled by @option{-Wall} in C and C++. > > "for C and C++" instead of "in"? Other parts of invoke.text use both, so I left that as it was. I think both work here. > I'm curious to see how many issues this is going to find in real-world > code out there! Yeah, me too. Marek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-09 22:03 ` Gerald Pfeifer 2017-06-13 13:46 ` Marek Polacek @ 2017-07-17 8:15 ` Gerald Pfeifer 2017-07-17 9:26 ` Marek Polacek 1 sibling, 1 reply; 13+ messages in thread From: Gerald Pfeifer @ 2017-07-17 8:15 UTC (permalink / raw) To: David Malcolm, Martin Sebor Cc: Marek Polacek, gcc-patches, Joseph Myers, Martin Sebor, Jason Merrill On Sat, 10 Jun 2017, Gerald Pfeifer wrote: > I'm curious to see how many issues this is going to find in real-world > code out there! This is a bit anecdotal, but your code did find one real issue in Wine: https://www.winehq.org/pipermail/wine-patches/2017-July/163551.html (and https://www.winehq.org/pipermail/wine-patches/2017-July/163550.html ) The first patch was accepted, the second (but earlier one) not. Gerald ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-07-17 8:15 ` Gerald Pfeifer @ 2017-07-17 9:26 ` Marek Polacek 0 siblings, 0 replies; 13+ messages in thread From: Marek Polacek @ 2017-07-17 9:26 UTC (permalink / raw) To: Gerald Pfeifer Cc: David Malcolm, Martin Sebor, gcc-patches, Joseph Myers, Martin Sebor, Jason Merrill On Mon, Jul 17, 2017 at 10:15:40AM +0200, Gerald Pfeifer wrote: > On Sat, 10 Jun 2017, Gerald Pfeifer wrote: > > I'm curious to see how many issues this is going to find in real-world > > code out there! > > This is a bit anecdotal, but your code did find one real issue in Wine: > > https://www.winehq.org/pipermail/wine-patches/2017-July/163551.html > (and https://www.winehq.org/pipermail/wine-patches/2017-July/163550.html ) > > The first patch was accepted, the second (but earlier one) not. Thanks, good to see that. It's also found some bugs in glibc. Would love to hear about kernel, too. Marek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-08 17:24 ` David Malcolm 2017-06-08 18:10 ` Martin Sebor @ 2017-06-13 10:05 ` Marek Polacek 2017-06-13 15:29 ` Joseph Myers 1 sibling, 1 reply; 13+ messages in thread From: Marek Polacek @ 2017-06-13 10:05 UTC (permalink / raw) To: David Malcolm; +Cc: GCC Patches, Joseph Myers, Martin Sebor, Jason Merrill On Thu, Jun 08, 2017 at 01:24:09PM -0400, David Malcolm wrote: > On Thu, 2017-06-08 at 18:49 +0200, Marek Polacek wrote: > > This is the hopefully last incarnation of the patch. The change from > > the > > last time[0] is simpy that I've added a new test and the warning has > > been > > renamed to -Wmultistatement-macros. > > > > David - any another comments? > > Thanks for working on this; looks useful. > > The new name is more accurate, but is rather long; oh well. As part of > -Wall, users won't typically have to type it, so that's OK. Yeah, I'm not in love with the name either, but it's the best we could come up with. > [...] > > > diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c > > index 35321a6..d883330 100644 > > --- gcc/c-family/c-warn.c > > +++ gcc/c-family/c-warn.c > [...] > > > + if (warning_at (body_loc, OPT_Wmultistatement_macros, > > + "macro expands to multiple statements")) > > + inform (guard_loc, "some parts of macro expansion are not > > guarded by " > > + "this conditional"); > > Is the guard necessarily a "conditional"? I take a "conditional" to > mean an "if"; the guard could be a "for" or a "while" (or an "else", > which still seems something of a stretch to me to call a > "conditional"). > > Suggestion: word "this conditional" as "this %qs clause" and either (a) > rework the code in c-indentation.c's guard_tinfo_to_string so that it's > shared between these two warnings (i.e. to go from a RID_ to a const > char *), or (b) just pass in a const char * identifying the guard > clause token. I prefer (a) so I tweaked guard_tinfo_to_string. Of course, the name is not that accurate anymore... > > diff --git gcc/c-family/c.opt gcc/c-family/c.opt > > index 37bb236..9dbe211 100644 > > --- gcc/c-family/c.opt > > +++ gcc/c-family/c.opt > > @@ -698,6 +698,10 @@ Wmissing-field-initializers > > C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning > > EnabledBy(Wextra) > > Warn about missing fields in struct initializers. > > > > +Wmultistatement-macros > > +C ObjC C++ ObjC++ Var(warn_multistatement_macros) Warning > > LangEnabledBy(C ObjC C++ ObjC++,Wall) > > +Warn about macros expanding to multiple statements in a body of a > > conditional such as if, else, while, or for. > > Likewise; is "conditional" the right word here? Also, whether of not > the statements are actually "in" the body of the guard is the issue > here. > > How about: > > "Warn about unsafe multiple statement macros that appear to be guarded > by a clause such as if, else, while, or for, in which only the first > statement is actually guarded after the macro is expanded." > > or somesuch? I think this is too long for c.opt, so I used: "Warn about unsafe macros expanding to multiple statements used as a body of a clause such as if, else, while, switch, or for." And I forgot to mention "switch" there. > > diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi > > index c116882..2fe16dd 100644 > > --- gcc/doc/invoke.texi > > +++ gcc/doc/invoke.texi > > @@ -4496,6 +4497,29 @@ This warning is enabled by @option{-Wall}. > > @opindex Wno-missing-include-dirs > > Warn if a user-supplied include directory does not exist. > > > > +@item -Wmultistatement-macros > > +@opindex Wmultistatement-macros > > +@opindex Wno-multistatement-macros > > +Warn about macros expanding to multiple statements in a body of a > > conditional, > > +such as @code{if}, @code{else}, @code{for}, or @code{while}. > > (as above). But I used the longer form here. > > +For example: > > + > > +@smallexample > > +#define DOIT x++; y++ > > +if (c) > > + DOIT; > > +@end smallexample > > + > > +will increment @code{y} unconditionally, not just when @code{c} > > holds. > > +The can usually be fixed by wrapping the macro in a do-while loop: > > +@smallexample > > +#define DOIT do @{ x++; y++; @} while (0) > > +if (c) > > + DOIT; > > +@end smallexample > > + > > +This warning is enabled by @option{-Wall} in C and C++. > > + > > @item -Wparentheses > > @opindex Wparentheses > > @opindex Wno-parentheses > > Hope this is constructive As usually ;). Updated version of the patch here. Thanks, Bootstrapped/regtested on x86_64-linux, ok for trunk? 2017-06-13 Marek Polacek <polacek@redhat.com> PR c/80116 * c-common.h (warn_for_multistatement_macros): Declare. * c-warn.c: Include "c-family/c-indentation.h". (warn_for_multistatement_macros): New function. * c.opt (Wmultistatement-macros): New option. * c-indentation.c (guard_tinfo_to_string): No longer static. Change the parameter type to "enum rid". Handle RID_SWITCH. * c-indentation.h (guard_tinfo_to_string): Declare. * c-parser.c (c_parser_if_body): Set the location of the body of the conditional after parsing all the labels. Call warn_for_multistatement_macros. (c_parser_else_body): Likewise. (c_parser_switch_statement): Likewise. (c_parser_while_statement): Likewise. (c_parser_for_statement): Likewise. (c_parser_statement): Add a default argument. Save the location after labels have been parsed. (c_parser_c99_block_statement): Likewise. * parser.c (cp_parser_statement): Add a default argument. Save the location of the expression-statement after labels have been parsed. (cp_parser_implicitly_scoped_statement): Set the location of the body of the conditional after parsing all the labels. Call warn_for_multistatement_macros. (cp_parser_already_scoped_statement): Likewise. * doc/invoke.texi: Document -Wmultistatement-macros. * c-c++-common/Wmultistatement-macros-1.c: New test. * c-c++-common/Wmultistatement-macros-2.c: New test. * c-c++-common/Wmultistatement-macros-3.c: New test. * c-c++-common/Wmultistatement-macros-4.c: New test. * c-c++-common/Wmultistatement-macros-5.c: New test. * c-c++-common/Wmultistatement-macros-6.c: New test. * c-c++-common/Wmultistatement-macros-7.c: New test. * c-c++-common/Wmultistatement-macros-8.c: New test. * c-c++-common/Wmultistatement-macros-9.c: New test. * c-c++-common/Wmultistatement-macros-10.c: New test. * c-c++-common/Wmultistatement-macros-11.c: New test. diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h index 79072e6..f046a37 100644 --- gcc/c-family/c-common.h +++ gcc/c-family/c-common.h @@ -1539,6 +1539,8 @@ extern bool maybe_warn_shift_overflow (location_t, tree, tree); extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **); extern bool diagnose_mismatched_attributes (tree, tree); extern tree do_warn_duplicated_branches_r (tree *, int *, void *); +extern void warn_for_multistatement_macros (location_t, location_t, + location_t, enum rid); /* In c-attribs.c. */ extern bool attribute_takes_identifier_p (const_tree); diff --git gcc/c-family/c-indentation.c gcc/c-family/c-indentation.c index 8300788..7ca21e8 100644 --- gcc/c-family/c-indentation.c +++ gcc/c-family/c-indentation.c @@ -542,10 +542,10 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, /* Return the string identifier corresponding to the given guard token. */ -static const char * -guard_tinfo_to_string (const token_indent_info &guard_tinfo) +const char * +guard_tinfo_to_string (enum rid keyword) { - switch (guard_tinfo.keyword) + switch (keyword) { case RID_FOR: return "for"; @@ -557,6 +557,8 @@ guard_tinfo_to_string (const token_indent_info &guard_tinfo) return "while"; case RID_DO: return "do"; + case RID_SWITCH: + return "switch"; default: gcc_unreachable (); } @@ -605,10 +607,10 @@ warn_for_misleading_indentation (const token_indent_info &guard_tinfo, { if (warning_at (guard_tinfo.location, OPT_Wmisleading_indentation, "this %qs clause does not guard...", - guard_tinfo_to_string (guard_tinfo))) + guard_tinfo_to_string (guard_tinfo.keyword))) inform (next_tinfo.location, "...this statement, but the latter is misleadingly indented" " as if it were guarded by the %qs", - guard_tinfo_to_string (guard_tinfo)); + guard_tinfo_to_string (guard_tinfo.keyword)); } } diff --git gcc/c-family/c-indentation.h gcc/c-family/c-indentation.h index a436697..e4cad26 100644 --- gcc/c-family/c-indentation.h +++ gcc/c-family/c-indentation.h @@ -48,5 +48,7 @@ extern void warn_for_misleading_indentation (const token_indent_info &guard_tinfo, const token_indent_info &body_tinfo, const token_indent_info &next_tinfo); +extern const char * +guard_tinfo_to_string (enum rid keyword); #endif /* ! GCC_C_INDENTATION_H */ diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c index 35321a6..72af573 100644 --- gcc/c-family/c-warn.c +++ gcc/c-family/c-warn.c @@ -31,6 +31,7 @@ along with GCC; see the file COPYING3. If not see #include "asan.h" #include "gcc-rich-location.h" #include "gimplify.h" +#include "c-family/c-indentation.h" /* Print a warning if a constant expression had overflow in folding. Invoke this function on every expression that the language @@ -2401,3 +2402,91 @@ do_warn_duplicated_branches_r (tree *tp, int *, void *) do_warn_duplicated_branches (*tp); return NULL_TREE; } + +/* Implementation of -Wmultistatement-macros. This warning warns about + cases when a macro expands to multiple statements not wrapped in + do {} while (0) or ({ }) and is used as a body of if/else/for/while + conditionals. For example, + + #define DOIT x++; y++ + + if (c) + DOIT; + + will increment y unconditionally. + + BODY_LOC is the location of the first token in the body after labels + have been parsed, NEXT_LOC is the location of the next token after the + body of the conditional has been parsed, and GUARD_LOC is the location + of the conditional. */ + +void +warn_for_multistatement_macros (location_t body_loc, location_t next_loc, + location_t guard_loc, enum rid keyword) +{ + if (!warn_multistatement_macros) + return; + + /* Ain't got time to waste. We only care about macros here. */ + if (!from_macro_expansion_at (body_loc) + || !from_macro_expansion_at (next_loc)) + return; + + /* Let's skip macros defined in system headers. */ + if (in_system_header_at (body_loc) + || in_system_header_at (next_loc)) + return; + + /* Find the actual tokens in the macro definition. BODY_LOC and + NEXT_LOC have to come from the same spelling location, but they + will resolve to different locations in the context of the macro + definition. */ + location_t body_loc_exp + = linemap_resolve_location (line_table, body_loc, + LRK_MACRO_DEFINITION_LOCATION, NULL); + location_t next_loc_exp + = linemap_resolve_location (line_table, next_loc, + LRK_MACRO_DEFINITION_LOCATION, NULL); + location_t guard_loc_exp + = linemap_resolve_location (line_table, guard_loc, + LRK_MACRO_DEFINITION_LOCATION, NULL); + + /* These are some funky cases we don't want to warn about. */ + if (body_loc_exp == guard_loc_exp + || next_loc_exp == guard_loc_exp + || body_loc_exp == next_loc_exp) + return; + + /* Find the macro map for the macro expansion BODY_LOC. */ + const line_map *map = linemap_lookup (line_table, body_loc); + const line_map_macro *macro_map = linemap_check_macro (map); + + /* Now see if the following token is coming from the same macro + expansion. If it is, it's a problem, because it should've been + parsed at this point. We only look at odd-numbered indexes + within the MACRO_MAP_LOCATIONS array, i.e. the spelling locations + of the tokens. */ + bool found_guard = false; + bool found_next = false; + for (unsigned int i = 1; + i < 2 * MACRO_MAP_NUM_MACRO_TOKENS (macro_map); + i += 2) + { + if (MACRO_MAP_LOCATIONS (macro_map)[i] == next_loc_exp) + found_next = true; + if (MACRO_MAP_LOCATIONS (macro_map)[i] == guard_loc_exp) + found_guard = true; + } + + /* The conditional itself must not come from the same expansion, because + we don't want to warn about + #define IF if (x) x++; y++ + and similar. */ + if (!found_next || found_guard) + return; + + if (warning_at (body_loc, OPT_Wmultistatement_macros, + "macro expands to multiple statements")) + inform (guard_loc, "some parts of macro expansion are not guarded by " + "this %qs clause", guard_tinfo_to_string (keyword)); +} diff --git gcc/c-family/c.opt gcc/c-family/c.opt index 37bb236..34b4a61 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -698,6 +698,10 @@ Wmissing-field-initializers C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning EnabledBy(Wextra) Warn about missing fields in struct initializers. +Wmultistatement-macros +C ObjC C++ ObjC++ Var(warn_multistatement_macros) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) +Warn about unsafe macros expanding to multiple statements used as a body of a clause such as if, else, while, switch, or for. + Wmultiple-inheritance C++ ObjC++ Var(warn_multiple_inheritance) Warning Warn on direct multiple inheritance. diff --git gcc/c/c-parser.c gcc/c/c-parser.c index 6f954f2..f8fbc92 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -1218,9 +1218,11 @@ static void c_parser_initval (c_parser *, struct c_expr *, static tree c_parser_compound_statement (c_parser *); static void c_parser_compound_statement_nostart (c_parser *); static void c_parser_label (c_parser *); -static void c_parser_statement (c_parser *, bool *); +static void c_parser_statement (c_parser *, bool *, location_t * = NULL); static void c_parser_statement_after_labels (c_parser *, bool *, vec<tree> * = NULL); +static tree c_parser_c99_block_statement (c_parser *, bool *, + location_t * = NULL); static void c_parser_if_statement (c_parser *, bool *, vec<tree> *); static void c_parser_switch_statement (c_parser *, bool *); static void c_parser_while_statement (c_parser *, bool, bool *); @@ -5204,9 +5206,11 @@ c_parser_label (c_parser *parser) implement -Wparentheses. */ static void -c_parser_statement (c_parser *parser, bool *if_p) +c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels) { c_parser_all_labels (parser); + if (loc_after_labels) + *loc_after_labels = c_parser_peek_token (parser)->location; c_parser_statement_after_labels (parser, if_p, NULL); } @@ -5466,11 +5470,12 @@ c_parser_paren_condition (c_parser *parser) implement -Wparentheses. */ static tree -c_parser_c99_block_statement (c_parser *parser, bool *if_p) +c_parser_c99_block_statement (c_parser *parser, bool *if_p, + location_t *loc_after_labels) { tree block = c_begin_compound_stmt (flag_isoc99); location_t loc = c_parser_peek_token (parser)->location; - c_parser_statement (parser, if_p); + c_parser_statement (parser, if_p, loc_after_labels); return c_end_compound_stmt (loc, block, flag_isoc99); } @@ -5492,6 +5497,7 @@ c_parser_if_body (c_parser *parser, bool *if_p, { tree block = c_begin_compound_stmt (flag_isoc99); location_t body_loc = c_parser_peek_token (parser)->location; + location_t body_loc_after_labels = UNKNOWN_LOCATION; token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); @@ -5508,11 +5514,18 @@ c_parser_if_body (c_parser *parser, bool *if_p, else if (c_parser_next_token_is (parser, CPP_OPEN_BRACE)) add_stmt (c_parser_compound_statement (parser)); else - c_parser_statement_after_labels (parser, if_p); + { + body_loc_after_labels = c_parser_peek_token (parser)->location; + c_parser_statement_after_labels (parser, if_p); + } token_indent_info next_tinfo = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (if_tinfo, body_tinfo, next_tinfo); + if (body_loc_after_labels != UNKNOWN_LOCATION + && next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (body_loc_after_labels, next_tinfo.location, + if_tinfo.location, RID_IF); return c_end_compound_stmt (body_loc, block, flag_isoc99); } @@ -5530,6 +5543,7 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo, tree block = c_begin_compound_stmt (flag_isoc99); token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); + location_t body_loc_after_labels = UNKNOWN_LOCATION; c_parser_all_labels (parser); if (c_parser_next_token_is (parser, CPP_SEMICOLON)) @@ -5542,11 +5556,18 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo, c_parser_consume_token (parser); } else - c_parser_statement_after_labels (parser, NULL, chain); + { + body_loc_after_labels = c_parser_peek_token (parser)->location; + c_parser_statement_after_labels (parser, NULL, chain); + } token_indent_info next_tinfo = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (else_tinfo, body_tinfo, next_tinfo); + if (body_loc_after_labels != UNKNOWN_LOCATION + && next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (body_loc_after_labels, next_tinfo.location, + else_tinfo.location, RID_ELSE); return c_end_compound_stmt (body_loc, block, flag_isoc99); } @@ -5732,7 +5753,13 @@ c_parser_switch_statement (c_parser *parser, bool *if_p) c_start_case (switch_loc, switch_cond_loc, expr, explicit_cast_p); save_break = c_break_label; c_break_label = NULL_TREE; - body = c_parser_c99_block_statement (parser, if_p); + location_t loc_after_labels; + bool open_brace_p = c_parser_peek_token (parser)->type == CPP_OPEN_BRACE; + body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels); + location_t next_loc = c_parser_peek_token (parser)->location; + if (!open_brace_p && c_parser_peek_token (parser)->type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, next_loc, switch_loc, + RID_SWITCH); c_finish_case (body, ce.original_type); if (c_break_label) { @@ -5783,7 +5810,8 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p) token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); - body = c_parser_c99_block_statement (parser, if_p); + location_t loc_after_labels; + body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels); c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true); add_stmt (c_end_compound_stmt (loc, block, flag_isoc99)); c_parser_maybe_reclassify_token (parser); @@ -5792,6 +5820,10 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p) = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo); + if (next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, next_tinfo.location, + while_tinfo.location, RID_WHILE); + c_break_label = save_break; c_cont_label = save_cont; } @@ -6076,7 +6108,8 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p) token_indent_info body_tinfo = get_token_indent_info (c_parser_peek_token (parser)); - body = c_parser_c99_block_statement (parser, if_p); + location_t loc_after_labels; + body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels); if (is_foreach_statement) objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label); @@ -6089,6 +6122,10 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p) = get_token_indent_info (c_parser_peek_token (parser)); warn_for_misleading_indentation (for_tinfo, body_tinfo, next_tinfo); + if (next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, next_tinfo.location, + for_tinfo.location, RID_FOR); + c_break_label = save_break; c_cont_label = save_cont; } diff --git gcc/cp/parser.c gcc/cp/parser.c index d02ad36..1107c03 100644 --- gcc/cp/parser.c +++ gcc/cp/parser.c @@ -2102,7 +2102,7 @@ static void cp_parser_lambda_body /* Statements [gram.stmt.stmt] */ static void cp_parser_statement - (cp_parser *, tree, bool, bool *, vec<tree> * = NULL); + (cp_parser *, tree, bool, bool *, vec<tree> * = NULL, location_t * = NULL); static void cp_parser_label_for_labeled_statement (cp_parser *, tree); static tree cp_parser_expression_statement @@ -10535,7 +10535,8 @@ cp_parser_lambda_body (cp_parser* parser, tree lambda_expr) static void cp_parser_statement (cp_parser* parser, tree in_statement_expr, - bool in_compound, bool *if_p, vec<tree> *chain) + bool in_compound, bool *if_p, vec<tree> *chain, + location_t *loc_after_labels) { tree statement, std_attrs = NULL_TREE; cp_token *token; @@ -10728,6 +10729,10 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr, if (cp_parser_parse_definitely (parser)) return; } + /* All preceding labels have been parsed at this point. */ + if (loc_after_labels != NULL) + *loc_after_labels = statement_location; + /* Look for an expression-statement instead. */ statement = cp_parser_expression_statement (parser, in_statement_expr); @@ -12268,6 +12273,7 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p, { tree statement; location_t body_loc = cp_lexer_peek_token (parser->lexer)->location; + location_t body_loc_after_labels = UNKNOWN_LOCATION; token_indent_info body_tinfo = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); @@ -12297,7 +12303,8 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p, /* Create a compound-statement. */ statement = begin_compound_stmt (0); /* Parse the dependent-statement. */ - cp_parser_statement (parser, NULL_TREE, false, if_p, chain); + cp_parser_statement (parser, NULL_TREE, false, if_p, chain, + &body_loc_after_labels); /* Finish the dummy compound-statement. */ finish_compound_stmt (statement); } @@ -12306,6 +12313,11 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p, = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo); + if (body_loc_after_labels != UNKNOWN_LOCATION + && next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (body_loc_after_labels, next_tinfo.location, + guard_tinfo.location, guard_tinfo.keyword); + /* Return the statement. */ return statement; } @@ -12324,11 +12336,18 @@ cp_parser_already_scoped_statement (cp_parser* parser, bool *if_p, { token_indent_info body_tinfo = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); + location_t loc_after_labels; - cp_parser_statement (parser, NULL_TREE, false, if_p); + cp_parser_statement (parser, NULL_TREE, false, if_p, NULL, + &loc_after_labels); token_indent_info next_tinfo = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo); + + if (next_tinfo.type != CPP_SEMICOLON) + warn_for_multistatement_macros (loc_after_labels, next_tinfo.location, + guard_tinfo.location, + guard_tinfo.keyword); } else { diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index 5d41649..ec59682 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -293,7 +293,7 @@ Objective-C and Objective-C++ Dialects}. -Wmain -Wmaybe-uninitialized -Wmemset-elt-size -Wmemset-transposed-args @gol -Wmisleading-indentation -Wmissing-braces @gol -Wmissing-field-initializers -Wmissing-include-dirs @gol --Wno-multichar -Wnonnull -Wnonnull-compare @gol +-Wno-multichar -Wmultistatement-macros -Wnonnull -Wnonnull-compare @gol -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol -Wnull-dereference -Wodr -Wno-overflow -Wopenmp-simd @gol -Woverride-init-side-effects -Woverlength-strings @gol @@ -3824,6 +3824,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wmemset-transposed-args @gol -Wmisleading-indentation @r{(only for C/C++)} @gol -Wmissing-braces @r{(only for C/ObjC)} @gol +-Wmultistatement-macros @gol -Wnarrowing @r{(only for C++)} @gol -Wnonnull @gol -Wnonnull-compare @gol @@ -4496,6 +4497,32 @@ This warning is enabled by @option{-Wall}. @opindex Wno-missing-include-dirs Warn if a user-supplied include directory does not exist. +@item -Wmultistatement-macros +@opindex Wmultistatement-macros +@opindex Wno-multistatement-macros +Warn about unsafe multiple statement macros that appear to be guarded +by a clause such as @code{if}, @code{else}, @code{for}, @code{switch}, or +@code{while}, in which only the first statement is actually guarded after +the macro is expanded. + +For example: + +@smallexample +#define DOIT x++; y++ +if (c) + DOIT; +@end smallexample + +will increment @code{y} unconditionally, not just when @code{c} holds. +The can usually be fixed by wrapping the macro in a do-while loop: +@smallexample +#define DOIT do @{ x++; y++; @} while (0) +if (c) + DOIT; +@end smallexample + +This warning is enabled by @option{-Wall} in C and C++. + @item -Wparentheses @opindex Wparentheses @opindex Wno-parentheses diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c index e69de29..cdecbb4 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-1.c @@ -0,0 +1,118 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(X, Y) \ + tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \ + X = Y; \ + Y = tmp + +#define STUFF \ + if (0) x = y + +#define STUFF2 \ + if (0) x = y; x++ + +#define STUFF3 \ + if (x) /* { dg-message "not guarded by this 'if' clause" } */ \ + SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define SET(X, Y) \ + (X) = (Y) + +#define STUFF4 \ + if (x) \ + SET(x, y); \ + SET(x, y) + +#define STUFF5 \ + { tmp = x; x = y; } + +#define STUFF6 \ + x++;; + +int x, y, tmp; + +void +fn1 (void) +{ + if (x) /* { dg-message "not guarded by this 'if' clause" } */ + SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */ +} + +void +fn2 (void) +{ + SWAP(x, y); +} + +void +fn3 (void) +{ + if (x) + { + SWAP(x, y); + } +} + +void +fn4 (void) +{ + if (x) + ({ x = 10; x++; }); +} + +void +fn5 (void) +{ + if (x) /* { dg-message "not guarded by this 'if' clause" } */ +L1: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L1; +} + +void +fn6 (void) +{ + if (x) + SET (x, y); + SET (tmp, x); +} + +void +fn7 (void) +{ + STUFF; +} + +void +fn8 (void) +{ + STUFF2; +} + +void +fn9 (void) +{ + STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */ +} + +void +fn10 (void) +{ + STUFF4; +} + +void +fn11 (void) +{ + if (x) + STUFF5; +} + +void +fn12 (void) +{ + if (x) + STUFF6; +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c index e69de29..766ed25 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-10.c @@ -0,0 +1,82 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(x, y) \ + tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \ + x = y; \ + y = tmp + +#define M1 \ + switch (x) /* { dg-message "not guarded by this 'switch' clause" } */ \ + case 1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define M2 \ + switch (x) \ + case 1: \ + x++ + +#define M3 \ + switch (x) \ + case 1: \ + x++;; + +#define M4 \ + switch (x) /* { dg-message "not guarded by this 'switch' clause" } */ \ +L1: \ + case 1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define INC \ + x++;; + +int x, y, tmp; + +void +fn0 (void) +{ + switch (x) /* { dg-message "not guarded by this 'switch' clause" } */ + case 1: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + + switch (x) /* { dg-message "not guarded by this 'switch' clause" } */ + case 1: + case 2: + case 3: + case 4: + case 5: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ +} + +void +fn1 (void) +{ + M1; /* { dg-message "in expansion of macro .M1." } */ + M2; + M3; + M4; /* { dg-message "in expansion of macro .M4." } */ + goto L1; +} + +void +fn2 (void) +{ + switch (x) + case 1: + INC + + switch (x) + case 1: + ({ x = 10; x++; }); +} + +void +fn3 (void) +{ + switch (x) + { + case 1: + SWAP (x, y); + } +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c index e69de29..4f4a123 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-11.c @@ -0,0 +1,19 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +enum E { A, B }; + +const char * +foo (enum E e) +{ +#define CASE(X) case X: return #X + switch (e) + { + CASE (A); + CASE (B); + default: + return "<unknown>"; + } +#undef CASE +}; diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c index e69de29..9fef901 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-2.c @@ -0,0 +1,137 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(X, Y) \ + tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \ + X = Y; \ + Y = tmp + +#define STUFF \ + if (0) {} else x = y + +#define STUFF2 \ + if (0) {} else x = y; x++ + +#define STUFF3 \ + if (x) \ + {} \ + else /* { dg-message "not guarded by this 'else' clause" } */ \ + SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define SET(X, Y) \ + (X) = (Y) + +#define STUFF4 \ + if (x) \ + {} \ + else \ + SET(x, y); \ + SET(x, y) + +#define STUFF5 \ + { tmp = x; x = y; } + +#define STUFF6 \ + x++;; + +int x, y, tmp; + +void +fn1 (void) +{ + if (x) + { + } + else /* { dg-message "not guarded by this 'else' clause" } */ + SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */ +} + +void +fn2 (void) +{ + SWAP(x, y); +} + +void +fn3 (void) +{ + if (x) + { + } + else + { + SWAP(x, y); + } +} + +void +fn4 (void) +{ + if (x) + { + } + else + ({ x = 10; x++; }); +} + +void +fn5 (void) +{ + if (x) + { + } + else /* { dg-message "not guarded by this 'else' clause" } */ +L1: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L1; +} + +void +fn6 (void) +{ + if (x) + { + } + else + SET (x, y); + SET (tmp, x); +} + +void +fn7 (void) +{ + STUFF; +} + +void +fn8 (void) +{ + STUFF2; +} + +void +fn9 (void) +{ + STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */ +} + +void +fn10 (void) +{ + STUFF4; +} + +void +fn11 (void) +{ + if (x) + STUFF5; +} + +void +fn12 (void) +{ + if (x) + STUFF6; +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c index e69de29..d130a35 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-3.c @@ -0,0 +1,12 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define CHECK(X) if (!(X)) __builtin_abort () + +void +fn (int i) +{ + CHECK (i == 1); + CHECK (i == 2); +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c index e69de29..e5cc9c3 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-4.c @@ -0,0 +1,14 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define FN(C) \ + void \ + fn (void) \ + { \ + C; \ + } + +int i; + +FN (if (i) ++i) diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c index e69de29..0ac84f5 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-5.c @@ -0,0 +1,18 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define M(N) \ +L ## N: \ + x++; x++ /* { dg-warning "macro expands to multiple statements" } */ + +int x, y, tmp; + +void +fn1 (void) +{ + if (x) /* { dg-message "not guarded by this 'if' clause" } */ + M (0); /* { dg-message "in expansion of macro .M." } */ + if (x) /* { dg-message "not guarded by this 'if' clause" } */ + M (1); /* { dg-message "in expansion of macro .M." } */ +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c index e69de29..5ec9cd9 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-6.c @@ -0,0 +1,22 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define M \ + if (x) x++; x++ + +void +f (int x) +{ + M; + M; + M; + M; + M; + M; + M; + M; + M; + M; + M; +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c index e69de29..d661f14 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-7.c @@ -0,0 +1,18 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(X, Y) \ + tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \ + X = Y; \ + Y = tmp + +#define BODY_AND_IF(COND, X, Y) \ + if (COND) SWAP (X, Y) /* { dg-message "in expansion of macro .SWAP." } */ + +void +fn (int x, int y) +{ + int tmp; + BODY_AND_IF (1, x, y); /* { dg-message "in expansion of macro .BODY_AND_IF." } */ +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c index e69de29..06522a7 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-8.c @@ -0,0 +1,64 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(x, y) \ + tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \ + x = y; \ + y = tmp + +#define M1 \ + for (i = 0; i < 1; ++i) /* { dg-message "not guarded by this 'for' clause" } */ \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define M2 \ + for (i = 0; i < 1; ++i) \ + x++ + +#define M3 \ + for (i = 0; i < 1; ++i) \ + x++;; + +#define M4 \ + for (i = 0; i < 1; ++i) /* { dg-message "not guarded by this 'for' clause" } */ \ +L1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define INC \ + x++;; + +int x, y, tmp; + +void +fn0 (void) +{ + int i; + for (i = 0; i < 1; ++i) /* { dg-message "not guarded by this 'for' clause" } */ + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + + for (i = 0; i < 1; ++i) /* { dg-message "not guarded by this 'for' clause" } */ +L: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L; +} + +void +fn1 (void) +{ + int i; + M1; /* { dg-message "in expansion of macro .M1." } */ + M2; + M3; + M4; /* { dg-message "in expansion of macro .M4." } */ + goto L1; +} + +void +fn2 (void) +{ + for (int i = 0; i < 1; ++i) + INC + + for (int i = 0; i < 1; ++i) + ({ x = 10; x++; }); +} diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c index e69de29..350c4f9f 100644 --- gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c +++ gcc/testsuite/c-c++-common/Wmultistatement-macros-9.c @@ -0,0 +1,62 @@ +/* PR c/80116 */ +/* { dg-options "-Wmultistatement-macros" } */ +/* { dg-do compile } */ + +#define SWAP(x, y) \ + tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \ + x = y; \ + y = tmp + +#define M1 \ + while (x) /* { dg-message "not guarded by this 'while' claus" } */ \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define M2 \ + while (x) \ + x++ + +#define M3 \ + while (x) \ + x++;; + +#define M4 \ + while (x) /* { dg-message "not guarded by this 'while' claus" } */ \ +L1: \ + SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */ + +#define INC \ + x++;; + +int x, y, tmp; + +void +fn0 (void) +{ + while (x) /* { dg-message "not guarded by this 'while' claus" } */ + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + + while (x) /* { dg-message "not guarded by this 'while' claus" } */ +L: + SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */ + goto L; +} + +void +fn1 (void) +{ + M1; /* { dg-message "in expansion of macro .M1." } */ + M2; + M3; + M4; /* { dg-message "in expansion of macro .M4." } */ + goto L1; +} + +void +fn2 (void) +{ + while (x) + INC + + while (x) + ({ x = 10; x++; }); +} Marek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-13 10:05 ` Marek Polacek @ 2017-06-13 15:29 ` Joseph Myers 2017-06-19 10:01 ` Marek Polacek 0 siblings, 1 reply; 13+ messages in thread From: Joseph Myers @ 2017-06-13 15:29 UTC (permalink / raw) To: Marek Polacek; +Cc: David Malcolm, GCC Patches, Martin Sebor, Jason Merrill On Tue, 13 Jun 2017, Marek Polacek wrote: > * c-parser.c (c_parser_if_body): Set the location of the > body of the conditional after parsing all the labels. Call > warn_for_multistatement_macros. > (c_parser_else_body): Likewise. > (c_parser_switch_statement): Likewise. > (c_parser_while_statement): Likewise. > (c_parser_for_statement): Likewise. > (c_parser_statement): Add a default argument. Save the location > after labels have been parsed. > (c_parser_c99_block_statement): Likewise. The gcc/c/ changes are OK. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-13 15:29 ` Joseph Myers @ 2017-06-19 10:01 ` Marek Polacek 2017-06-26 9:40 ` Marek Polacek 0 siblings, 1 reply; 13+ messages in thread From: Marek Polacek @ 2017-06-19 10:01 UTC (permalink / raw) To: Joseph Myers; +Cc: David Malcolm, GCC Patches, Martin Sebor, Jason Merrill On Tue, Jun 13, 2017 at 03:29:32PM +0000, Joseph Myers wrote: > On Tue, 13 Jun 2017, Marek Polacek wrote: > > > * c-parser.c (c_parser_if_body): Set the location of the > > body of the conditional after parsing all the labels. Call > > warn_for_multistatement_macros. > > (c_parser_else_body): Likewise. > > (c_parser_switch_statement): Likewise. > > (c_parser_while_statement): Likewise. > > (c_parser_for_statement): Likewise. > > (c_parser_statement): Add a default argument. Save the location > > after labels have been parsed. > > (c_parser_c99_block_statement): Likewise. > > The gcc/c/ changes are OK. Thanks. David, do you have any more comments on the patch? Marek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-19 10:01 ` Marek Polacek @ 2017-06-26 9:40 ` Marek Polacek 2017-06-26 13:13 ` David Malcolm 0 siblings, 1 reply; 13+ messages in thread From: Marek Polacek @ 2017-06-26 9:40 UTC (permalink / raw) To: Joseph Myers; +Cc: David Malcolm, GCC Patches, Martin Sebor, Jason Merrill On Mon, Jun 19, 2017 at 12:01:06PM +0200, Marek Polacek wrote: > On Tue, Jun 13, 2017 at 03:29:32PM +0000, Joseph Myers wrote: > > On Tue, 13 Jun 2017, Marek Polacek wrote: > > > > > * c-parser.c (c_parser_if_body): Set the location of the > > > body of the conditional after parsing all the labels. Call > > > warn_for_multistatement_macros. > > > (c_parser_else_body): Likewise. > > > (c_parser_switch_statement): Likewise. > > > (c_parser_while_statement): Likewise. > > > (c_parser_for_statement): Likewise. > > > (c_parser_statement): Add a default argument. Save the location > > > after labels have been parsed. > > > (c_parser_c99_block_statement): Likewise. > > > > The gcc/c/ changes are OK. > > Thanks. > > David, do you have any more comments on the patch? Seems not, so I'll commit the patch today. Marek ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-26 9:40 ` Marek Polacek @ 2017-06-26 13:13 ` David Malcolm 0 siblings, 0 replies; 13+ messages in thread From: David Malcolm @ 2017-06-26 13:13 UTC (permalink / raw) To: Marek Polacek, Joseph Myers; +Cc: GCC Patches, Martin Sebor, Jason Merrill On Mon, 2017-06-26 at 11:40 +0200, Marek Polacek wrote: > On Mon, Jun 19, 2017 at 12:01:06PM +0200, Marek Polacek wrote: > > On Tue, Jun 13, 2017 at 03:29:32PM +0000, Joseph Myers wrote: > > > On Tue, 13 Jun 2017, Marek Polacek wrote: > > > > > > > * c-parser.c (c_parser_if_body): Set the location of > > > > the > > > > body of the conditional after parsing all the labels. > > > > Call > > > > warn_for_multistatement_macros. > > > > (c_parser_else_body): Likewise. > > > > (c_parser_switch_statement): Likewise. > > > > (c_parser_while_statement): Likewise. > > > > (c_parser_for_statement): Likewise. > > > > (c_parser_statement): Add a default argument. Save the > > > > location > > > > after labels have been parsed. > > > > (c_parser_c99_block_statement): Likewise. > > > > > > The gcc/c/ changes are OK. > > > > Thanks. > > > > David, do you have any more comments on the patch? > > Seems not, so I'll commit the patch today. > Oops; sorry. I think all I had were those relatively minor comments about the wording in the description/invoke.texi, but I have no objection to this going into trunk as-is. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) 2017-06-08 16:49 C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) Marek Polacek 2017-06-08 17:24 ` David Malcolm @ 2017-06-10 0:47 ` Jason Merrill 1 sibling, 0 replies; 13+ messages in thread From: Jason Merrill @ 2017-06-10 0:47 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Joseph Myers, David Malcolm, Martin Sebor On Thu, Jun 8, 2017 at 9:49 AM, Marek Polacek <polacek@redhat.com> wrote: > Jason - how about the C++ parts? The C++ parts are OK. Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-07-17 9:26 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-08 16:49 C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) Marek Polacek 2017-06-08 17:24 ` David Malcolm 2017-06-08 18:10 ` Martin Sebor 2017-06-09 22:03 ` Gerald Pfeifer 2017-06-13 13:46 ` Marek Polacek 2017-07-17 8:15 ` Gerald Pfeifer 2017-07-17 9:26 ` Marek Polacek 2017-06-13 10:05 ` Marek Polacek 2017-06-13 15:29 ` Joseph Myers 2017-06-19 10:01 ` Marek Polacek 2017-06-26 9:40 ` Marek Polacek 2017-06-26 13:13 ` David Malcolm 2017-06-10 0:47 ` Jason Merrill
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 read-only IMAP folder(s) and NNTP newsgroup(s).