From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20890 invoked by alias); 30 Nov 2017 23:17:08 -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 20878 invoked by uid 89); 30 Nov 2017 23:17:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.7 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,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; Thu, 30 Nov 2017 23:17:05 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5FB995F73B; Thu, 30 Nov 2017 23:17:03 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-116-77.ams2.redhat.com [10.36.116.77]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 11B6160BE1; Thu, 30 Nov 2017 23:16:59 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id vAUNGv6l028851; Fri, 1 Dec 2017 00:16:57 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id vAUNGtK1028850; Fri, 1 Dec 2017 00:16:55 +0100 Date: Thu, 30 Nov 2017 23:24:00 -0000 From: Jakub Jelinek To: Richard Biener , Jeff Law , Jason Merrill , "Joseph S. Myers" , Marek Polacek Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] Improve -Wimplicit-fallthrough for nested switches (PR c/79153) Message-ID: <20171130231655.GT2353@tucnak> Reply-To: Jakub Jelinek MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.7.1 (2016-10-04) X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg02633.txt.bz2 Hi! A nested switch that can fallthrough (either because it has break stmts or because it doesn't have default: and doesn't cover all cases) isn't unfortunately reported with -Wimplicit-fallthrough, because we first gimplify the nested switch and then we just see a label added implicitly for break; or for the default: case if it was missing and the cases don't cover all possible values and punt on that. The following patch fixes it by telling the -Wimplicit-fallthrough code about the special labels added for break; or missing default: through a new SWITCH_BREAK_LABEL_P flag. Additionally it makes sure that GIMPLE_LABEL is emitted still inside of the switch's body sequence and if it is a nested switch, it wraps the GIMPLE_SWITCH with the body that ends in a GIMPLE_LABEL with SWITCH_BREAK_LABEL_P in a GIMPLE_BIND (without vars or blocks). With this, the -Wimplicit-fallthrough code can recognize this and treat the whole switch that can fall through as a single statement for the purpose of the warning. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2017-11-30 Jakub Jelinek PR c/79153 * tree.h (SWITCH_BREAK_LABEL_P): Define. * gimplify.c (collect_fallthrough_labels): Handle GIMPLE_BIND starting with a GIMPLE_SWITCH and ending with GIMPLE_LABEL with SWITCH_BREAK_LABEL_P set on the label. (gimplify_switch_expr): Set SWITCH_BREAK_LABEL_P on the label added for default case if it was missing and not all cases covered. Wrap GIMPLE_SWITCH and the switch_body_seq into a GIMPLE_BIND if switch_body_seq ends with a GIMPLE_LABEL with SWITCH_BREAK_LABEL_P set on the label. * tree-chrec.c (evolution_function_is_univariate_p): Add return true; to avoid -Wimplicit-fallthrough warning. * config/i386/i386.c (ix86_expand_special_args_builtin): Add FALLTHRU comment to avoid -Wimplicit-fallthrough warning. c/ * c-parser.c: Include tree-iterator.h. (c_parser_switch_statement): Emit LABEL_EXPR for the break label into SWITCH_BODY instead of after it and set SWITCH_BREAK_LABEL_P on it. cp/ * cp-gimplify.c (genericize_switch_stmt): Emit LABEL_EXPR for the break label into SWITCH_BODY instead of after it and set SWITCH_BREAK_LABEL_P on it. * parser.c (cp_parser_objc_expression): Add FALLTHRU comment to avoid -Wimplicit-fallthrough warning. fortran/ * match.c (gfc_match): Add FALLTHRU comment to avoid -Wimplicit-fallthrough warning. testsuite/ * c-c++-common/Wimplicit-fallthrough-7.c: Adjust expected warning line. * c-c++-common/Wimplicit-fallthrough-36.c: New test. --- gcc/tree.h.jj 2017-11-28 22:21:17.000000000 +0100 +++ gcc/tree.h 2017-11-30 17:55:35.299197618 +0100 @@ -766,6 +766,11 @@ extern void omp_clause_range_check_faile #define FALLTHROUGH_LABEL_P(NODE) \ (LABEL_DECL_CHECK (NODE)->base.private_flag) +/* Set on the artificial label created for break; stmt from a switch. + This is used to implement -Wimplicit-fallthrough. */ +#define SWITCH_BREAK_LABEL_P(NODE) \ + (LABEL_DECL_CHECK (NODE)->base.protected_flag) + /* Nonzero means this expression is volatile in the C sense: its address should be of type `volatile WHATEVER *'. In other words, the declared item is volatile qualified. --- gcc/gimplify.c.jj 2017-11-28 12:11:40.000000000 +0100 +++ gcc/gimplify.c 2017-11-30 19:04:32.694173903 +0100 @@ -1897,6 +1897,27 @@ collect_fallthrough_labels (gimple_stmt_ do { + if (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_BIND) + { + /* Recognize the special GIMPLE_BIND added by gimplify_switch_expr, + which starts on a GIMPLE_SWITCH and ends with a break label. + Handle that as a single statement that can fall through. */ + gbind *bind = as_a (gsi_stmt (*gsi_p)); + gimple *first = gimple_seq_first_stmt (gimple_bind_body (bind)); + gimple *last = gimple_seq_last_stmt (gimple_bind_body (bind)); + if (last + && gimple_code (first) == GIMPLE_SWITCH + && gimple_code (last) == GIMPLE_LABEL) + { + tree label = gimple_label_label (as_a (last)); + if (SWITCH_BREAK_LABEL_P (label)) + { + prev = bind; + gsi_next (gsi_p); + continue; + } + } + } if (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_BIND || gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_TRY) { @@ -2315,6 +2336,7 @@ gimplify_switch_expr (tree *expr_p, gimp preprocess_case_label_vec_for_gimple (labels, index_type, &default_case); + bool add_bind = false; if (!default_case) { glabel *new_default; @@ -2322,14 +2344,46 @@ gimplify_switch_expr (tree *expr_p, gimp default_case = build_case_label (NULL_TREE, NULL_TREE, create_artificial_label (UNKNOWN_LOCATION)); + if (old_in_switch_expr) + { + SWITCH_BREAK_LABEL_P (CASE_LABEL (default_case)) = 1; + add_bind = true; + } new_default = gimple_build_label (CASE_LABEL (default_case)); gimplify_seq_add_stmt (&switch_body_seq, new_default); } + else if (old_in_switch_expr) + { + gimple *last = gimple_seq_last_stmt (switch_body_seq); + if (last && gimple_code (last) == GIMPLE_LABEL) + { + tree label = gimple_label_label (as_a (last)); + if (SWITCH_BREAK_LABEL_P (label)) + add_bind = true; + } + } switch_stmt = gimple_build_switch (SWITCH_COND (switch_expr), - default_case, labels); - gimplify_seq_add_stmt (pre_p, switch_stmt); - gimplify_seq_add_seq (pre_p, switch_body_seq); + default_case, labels); + /* For the benefit of -Wimplicit-fallthrough, if switch_body_seq + ends with a GIMPLE_LABEL holding SWITCH_BREAK_LABEL_P LABEL_DECL, + wrap the GIMPLE_SWITCH up to that GIMPLE_LABEL into a GIMPLE_BIND, + so that we can easily find the start and end of the switch + statement. */ + if (add_bind) + { + gimple_seq bind_body = NULL; + gimplify_seq_add_stmt (&bind_body, switch_stmt); + gimple_seq_add_seq (&bind_body, switch_body_seq); + gbind *bind = gimple_build_bind (NULL_TREE, bind_body, NULL_TREE); + gimple_set_location (bind, EXPR_LOCATION (switch_expr)); + gimplify_seq_add_stmt (pre_p, bind); + } + else + { + gimplify_seq_add_stmt (pre_p, switch_stmt); + gimplify_seq_add_seq (pre_p, switch_body_seq); + } labels.release (); } else --- gcc/tree-chrec.c.jj 2017-10-11 22:37:51.000000000 +0200 +++ gcc/tree-chrec.c 2017-11-30 20:39:01.613902149 +0100 @@ -1161,6 +1161,7 @@ evolution_function_is_univariate_p (cons return false; break; } + return true; default: return true; --- gcc/config/i386/i386.c.jj 2017-11-30 09:42:46.000000000 +0100 +++ gcc/config/i386/i386.c 2017-11-30 20:40:02.484133842 +0100 @@ -34985,6 +34985,7 @@ ix86_expand_special_args_builtin (const default: break; } + /* FALLTHRU */ case V64QI_FTYPE_PCCHAR_V64QI_UDI: case V32QI_FTYPE_PCCHAR_V32QI_USI: case V16QI_FTYPE_PCCHAR_V16QI_UHI: --- gcc/c/c-parser.c.jj 2017-11-30 12:13:49.000000000 +0100 +++ gcc/c/c-parser.c 2017-11-30 18:35:03.735146924 +0100 @@ -67,6 +67,7 @@ along with GCC; see the file COPYING3. #include "run-rtl-passes.h" #include "intl.h" #include "c-family/name-hint.h" +#include "tree-iterator.h" /* We need to walk over decls with incomplete struct/union/enum types after parsing the whole translation unit. @@ -5846,14 +5847,15 @@ c_parser_switch_statement (c_parser *par 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) { location_t here = c_parser_peek_token (parser)->location; tree t = build1 (LABEL_EXPR, void_type_node, c_break_label); SET_EXPR_LOCATION (t, here); - add_stmt (t); + SWITCH_BREAK_LABEL_P (c_break_label) = 1; + append_to_statement_list_force (t, &body); } + c_finish_case (body, ce.original_type); c_break_label = save_break; add_stmt (c_end_compound_stmt (switch_loc, block, flag_isoc99)); c_parser_maybe_reclassify_token (parser); --- gcc/cp/cp-gimplify.c.jj 2017-11-28 22:23:34.000000000 +0100 +++ gcc/cp/cp-gimplify.c 2017-11-30 19:26:36.432776337 +0100 @@ -330,11 +330,13 @@ genericize_switch_stmt (tree *stmt_p, in cp_walk_tree (&type, cp_genericize_r, data, NULL); *walk_subtrees = 0; + if (TREE_USED (break_block)) + SWITCH_BREAK_LABEL_P (break_block) = 1; + finish_bc_block (&body, bc_break, break_block); *stmt_p = build2_loc (stmt_locus, SWITCH_EXPR, type, cond, body); SWITCH_ALL_CASES_P (*stmt_p) = SWITCH_STMT_ALL_CASES_P (stmt); gcc_checking_assert (!SWITCH_STMT_NO_BREAK_P (stmt) || !TREE_USED (break_block)); - finish_bc_block (stmt_p, bc_break, break_block); } /* Genericize a CONTINUE_STMT node *STMT_P. */ --- gcc/cp/parser.c.jj 2017-11-30 09:42:44.000000000 +0100 +++ gcc/cp/parser.c 2017-11-30 20:40:37.868687216 +0100 @@ -29003,6 +29003,7 @@ cp_parser_objc_expression (cp_parser* pa default: break; } + /* FALLTHRU */ default: error_at (kwd->location, "misplaced %<@%D%> Objective-C++ construct", --- gcc/fortran/match.c.jj 2017-10-28 09:00:47.000000000 +0200 +++ gcc/fortran/match.c 2017-11-30 20:41:45.464834013 +0100 @@ -1240,6 +1240,7 @@ loop: default: gfc_internal_error ("gfc_match(): Bad match code %c", c); } + /* FALLTHRU */ default: --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-7.c.jj 2017-04-28 22:17:03.000000000 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-7.c 2017-11-30 19:14:47.535557117 +0100 @@ -51,9 +51,9 @@ f (int i) { case 1: { - switch (i + 2) + switch (i + 2) /* { dg-warning "statement may fall through" } */ case 4: - bar (1); /* { dg-warning "statement may fall through" } */ + bar (1); case 5: bar (5); return; --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-36.c.jj 2017-11-30 19:11:32.029979082 +0100 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-36.c 2017-11-30 19:11:07.000000000 +0100 @@ -0,0 +1,72 @@ +/* PR c/79153 */ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough" } */ + +int +test (int v1, int v2) +{ + switch (v1) + { + case 3: + switch (v2) /* { dg-warning "this statement may fall through" } */ + { + case 1: + return 28; + case 2: + return 38; + case 3: + return 88; + default: + break; + } + case 4: /* { dg-message "here" } */ + return 168; + case 5: + switch (v2) /* { dg-warning "this statement may fall through" } */ + { + case 4: + break; + case 5: + return 38; + case 6: + return 88; + } + case 6: /* { dg-message "here" } */ + return 169; + case 7: + switch (v2) /* { dg-warning "this statement may fall through" } */ + { + case 7: + return 38; + case 8: + return 88; + } + case 8: /* { dg-message "here" } */ + return 170; + case 9: + switch (v2) /* { dg-bogus "this statement may fall through" } */ + { + case 9: + return 38; + case 10: + return 88; + default: + return 89; + } + case 10: + return 171; + case 11: + switch (v2) /* { dg-bogus "this statement may fall through" } */ + { + case -__INT_MAX__ - 1 ... 31: + return 38; + case 32: + return 88; + case 33 ... __INT_MAX__: + return 89; + } + case 12: + return 172; + } + return -1; +} Jakub