From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27828 invoked by alias); 13 Jun 2017 10:05:17 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 26841 invoked by uid 89); 13 Jun 2017 10:05:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 13 Jun 2017 10:05:10 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D04ED2B0A79; Tue, 13 Jun 2017 10:05:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D04ED2B0A79 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=polacek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D04ED2B0A79 Received: from redhat.com ([10.40.205.13]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v5DA59Q1011062 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 13 Jun 2017 06:05:11 -0400 Date: Tue, 13 Jun 2017 10:05:00 -0000 From: Marek Polacek To: David Malcolm Cc: GCC Patches , Joseph Myers , Martin Sebor , Jason Merrill Subject: Re: C/C++ PATCH to implement -Wmultistatement-macros (PR c/80116) Message-ID: <20170613100508.GZ3413@redhat.com> References: <20170608164936.GV3413@redhat.com> <1496942649.7551.150.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1496942649.7551.150.camel@redhat.com> User-Agent: Mutt/1.8.0 (2017-02-23) X-SW-Source: 2017-06/txt/msg00880.txt.bz2 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 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 **); 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 * = NULL); +static tree c_parser_c99_block_statement (c_parser *, bool *, + location_t * = NULL); static void c_parser_if_statement (c_parser *, bool *, vec *); 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 * = NULL); + (cp_parser *, tree, bool, bool *, vec * = 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 *chain) + bool in_compound, bool *if_p, vec *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 ""; + } +#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