From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45290 invoked by alias); 4 Sep 2017 17:34:58 -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 45198 invoked by uid 89); 4 Sep 2017 17:34:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=BAYES_00,BODY_8BITS,GARBLED_BODY,GIT_PATCH_1,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=H*Ad:U*dvyukov 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; Mon, 04 Sep 2017 17:34:52 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B0BBD13D1A; Mon, 4 Sep 2017 17:34:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B0BBD13D1A Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jakub@redhat.com Received: from tucnak.zalov.cz (ovpn-116-33.ams2.redhat.com [10.36.116.33]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3F92D9EACA; Mon, 4 Sep 2017 17:34:49 +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 v84HYlII013248; Mon, 4 Sep 2017 19:34:47 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id v84HYjax013247; Mon, 4 Sep 2017 19:34:45 +0200 Date: Mon, 04 Sep 2017 17:34:00 -0000 From: Jakub Jelinek To: =?utf-8?B?5ZC05r2N5rWgKOatpOW9vCk=?= Cc: Dmitry Vyukov , gcc-patches , Jeff Law , wishwu007 Subject: Re: Add support to trace comparison instructions and switch statements Message-ID: <20170904173445.GX2323@tucnak> Reply-To: Jakub Jelinek References: <234840fd-a06a-4dfd-a1c5-254e26144754.weixi.wwx@antfin.com> <887030ab-6bae-4dd5-b347-05bfad034603.weixi.wwx@antfin.com> <20170901162318.GN2323@tucnak> <20170903100121.GU2323@tucnak> <14d63d4d-aef0-4a82-b553-67d98da3cf42.weixi.wwx@antfin.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00202.txt.bz2 On Mon, Sep 04, 2017 at 09:36:40PM +0800, 吴潍浠(此彼) wrote: > gcc/ChangeLog: > > 2017-09-04 Wish Wu > > * asan.c (initialize_sanitizer_builtins): > * builtin-types.def (BT_FN_VOID_UINT8_UINT8): > (BT_FN_VOID_UINT16_UINT16): > (BT_FN_VOID_UINT32_UINT32): > (BT_FN_VOID_FLOAT_FLOAT): > (BT_FN_VOID_DOUBLE_DOUBLE): > (BT_FN_VOID_UINT64_PTR): > * common.opt: > * flag-types.h (enum sanitize_coverage_code): > * opts.c (COVERAGE_SANITIZER_OPT): > (get_closest_sanitizer_option): > (parse_sanitizer_options): > (common_handle_option): > * sancov.c (instrument_cond): > (instrument_switch): > (sancov_pass): > * sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_CMP1): > (BUILT_IN_SANITIZER_COV_TRACE_CMP2): > (BUILT_IN_SANITIZER_COV_TRACE_CMP4): > (BUILT_IN_SANITIZER_COV_TRACE_CMP8): > (BUILT_IN_SANITIZER_COV_TRACE_CMPF): > (BUILT_IN_SANITIZER_COV_TRACE_CMPD): > (BUILT_IN_SANITIZER_COV_TRACE_SWITCH): mklog just generates a template, you need to fill in the details on what has been changed or added or removed. See other ChangeLog entries etc. to see what is expected. > For code : > void bar (void); > void > foo (int x) > { > if (x == 21 || x == 64 || x == 98 || x == 135) > bar (); > } > GIMPLE IL on x86_64: > 1 > 2 ;; Function foo (foo, funcdef_no=0, decl_uid=2161, cgraph_uid=0, symbol_order=0) > 3 > 4 foo (int x) > 5 { ... That is with -O0 though? With -O2 you'll see that it changes. IMNSHO you really want to also handle the GIMPLE_ASSIGN with tcc_comparison class rhs_code. Shouldn't be that hard to handle that within instrument_cond, just the way how you extract lhs and rhs from the insn will differ based on if it is a GIMPLE_COND or GIMPLE_ASSIGN (and in that case also for tcc_comparison rhs_code or for COND_EXPR with tcc_comparison first operand). And I really think we should change the 2 LOGICAL_OP_NON_SHORT_CIRCUIT uses in fold-const.c and one in tree-ssa-ifcombine.c with LOGICAL_OP_NON_SHORT_CIRCUIT && !flag_sanitize_coverage with a comment that for sanitize coverage we want to avoid this optimization because it negatively affects it. @@ -1611,38 +1631,51 @@ parse_sanitizer_options (const char *p, location_t } /* Check to see if the string matches an option class name. */ - for (i = 0; sanitizer_opts[i].name != NULL; ++i) - if (len == sanitizer_opts[i].len - && memcmp (p, sanitizer_opts[i].name, len) == 0) + for (i = 0; opts[i].name != NULL; ++i) + if (len == opts[i].len + && memcmp (p, opts[i].name, len) == 0) { - /* Handle both -fsanitize and -fno-sanitize cases. */ - if (value && sanitizer_opts[i].flag == ~0U) + if (code == OPT_fsanitize_coverage_) { - if (code == OPT_fsanitize_) - { - if (complain) - error_at (loc, "%<-fsanitize=all%> option is not valid"); - } + if (value) + flags |= opts[i].flag; else - flags |= ~(SANITIZE_THREAD | SANITIZE_LEAK - | SANITIZE_UNREACHABLE | SANITIZE_RETURN); + flags &= ~opts[i].flag; + found = true; + break; } - else if (value) + else { - /* Do not enable -fsanitize-recover=unreachable and - -fsanitize-recover=return if -fsanitize-recover=undefined - is selected. */ - if (code == OPT_fsanitize_recover_ - && sanitizer_opts[i].flag == SANITIZE_UNDEFINED) - flags |= (SANITIZE_UNDEFINED - & ~(SANITIZE_UNREACHABLE | SANITIZE_RETURN)); + /* Handle both -fsanitize and -fno-sanitize cases. */ + if (value && opts[i].flag == ~0U) + { + if (code == OPT_fsanitize_) + { + if (complain) + error_at (loc, + "%<-fsanitize=all%> option is not valid"); + } + else + flags |= ~(SANITIZE_THREAD | SANITIZE_LEAK + | SANITIZE_UNREACHABLE | SANITIZE_RETURN); + } + else if (value) + { + /* Do not enable -fsanitize-recover=unreachable and + -fsanitize-recover=return if -fsanitize-recover=undefined + is selected. */ + if (code == OPT_fsanitize_recover_ + && opts[i].flag == SANITIZE_UNDEFINED) + flags |= (SANITIZE_UNDEFINED + & ~(SANITIZE_UNREACHABLE | SANITIZE_RETURN)); + else + flags |= opts[i].flag; + } else - flags |= sanitizer_opts[i].flag; + flags &= ~opts[i].flag; + found = true; + break; } - else - flags &= ~sanitizer_opts[i].flag; - found = true; - break; } I don't see a need for this hunk. For code == OPT_fsanitize_coverage_ (where you know that there is no entry with ~0U flag value and also know that code is not OPT_fsanitize_recover_) I think it will just DTRT without any changes. namespace { There should be a function comment before the function here, explaining what the function is for. +static void +instrument_cond (gimple_stmt_iterator *gsi, gimple *stmt) +{ + tree lhs = gimple_cond_lhs (stmt); + tree rhs = gimple_cond_rhs (stmt); + tree lhs_type = TREE_TYPE (lhs); + tree rhs_type = TREE_TYPE (rhs); + + HOST_WIDE_INT size_in_bytes = MAX (int_size_in_bytes (lhs_type), + int_size_in_bytes (rhs_type)); + if (size_in_bytes == -1) + return; As I said, GIMPLE_COND operands should have the same (or compatible) type, so there is no need to use rhs_type at all, nor test it. And there is no need to test size_in_bytes before the if, just do it right before the switch in there (and no need to special case -1, because it is like any other default: handled by return;). + else if (SCALAR_FLOAT_TYPE_P (lhs_type) && SCALAR_FLOAT_TYPE_P (rhs_type)) Again, no need to test both. + { + if (TYPE_MODE (lhs_type) == TYPE_MODE (double_type_node) + || TYPE_MODE (rhs_type) == TYPE_MODE (double_type_node)) Or here. + { + fncode = BUILT_IN_SANITIZER_COV_TRACE_CMPD; + to_type = double_type_node; + } + else if (TYPE_MODE (lhs_type) == TYPE_MODE (float_type_node) + || TYPE_MODE (rhs_type) == TYPE_MODE (float_type_node)) Or here. Just use type instead of lhs_type. + { + fncode = BUILT_IN_SANITIZER_COV_TRACE_CMPF; + to_type = float_type_node; + } + + } + if (to_type != NULL_TREE) + { + gimple_seq seq = NULL; + + gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs)); + tree clhs = gimple_assign_lhs (gimple_seq_last_stmt (seq)); + + gimple_seq_add_stmt (&seq, build_type_cast (to_type, rhs)); + tree crhs = gimple_assign_lhs (gimple_seq_last_stmt (seq)); If the var already has the right type, that will just create waste that further opts will need to clean up. Better: if (!useless_type_conversion_p (to_type, lhs_type)) // or s/lhs_// if you do the above { gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs)); lhs = gimple_assign_lhs (gimple_seq_last_stmt (seq)); gimple_seq_add_stmt (&seq, build_type_cast (to_type, rhs)); rhs = gimple_assign_rhs (gimple_seq_last_stmt (seq)); } or perhaps even if (!useless_type_conversion_p (to_type, type)) { if (TREE_CODE (lhs) == INTEGER_CST) lhs = fold_convert (to_type, lhs); else { gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs)); lhs = gimple_assign_lhs (gimple_seq_last_stmt (seq)); } ... } + tree index = gimple_switch_index (switch_stmt); + + HOST_WIDE_INT size_in_bytes = int_size_in_bytes (TREE_TYPE (index)); + if (size_in_bytes == -1) + return; Well, you want to punt not just when it is -1, but also when it is > 8. + + unsigned i, n = gimple_switch_num_labels (switch_stmt), num = 0; + for (i = 0; i < n; ++i) I think gimple_switch_label (switch_stmt, 0) must be the default label and thus have no low/high case, so you should use for (i = 1; i < n; ++i). + for (i = 0; i < n; ++i) Ditto. Jakub