From patchwork Tue Nov 13 16:40:14 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 198740 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 0D54A2C00B0 for ; Wed, 14 Nov 2012 03:41:21 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1353429683; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:Received:Date:From:To:Cc:Subject:Message-ID:Reply-To: References:MIME-Version:Content-Type:Content-Disposition: In-Reply-To:User-Agent:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=p0au22PRG3Vou3b2+d3taMr8K8E=; b=dOSdUfJHaSbVEXZ flUPl8fwg1uU11QBRRRTxtx755rrsA+pUAU3hgy2t887Zm3hMC6J2bjvhUAISUIf /M2MJWRR8sIgqaK55po/d71KtsXQjB0msw6NLaFJipe92fUD4KBSngj15SjBlLtj 1wy0WEBoXlV6pHZRfxVE4rU5RVKs= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Received:Received:Date:From:To:Cc:Subject:Message-ID:Reply-To:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:User-Agent:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=K7/1R1ll6L4Kw4pokkY1QubUso1BChk52Jj5dvcQUyxdhKtFE1IMBONdQ3vmeH xO5CAuWoSxXzaxaNSInizmqck55YFPI21arei7siw9rRgVCmdw5KsRWM0UNJDMQZ lKoVcsrrpoJLyYj/M2DWtPUmq/d+tzLx1NeRNh7hpqxCs=; Received: (qmail 8458 invoked by alias); 13 Nov 2012 16:41:14 -0000 Received: (qmail 8147 invoked by uid 22791); 13 Nov 2012 16:41:11 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, SPF_HELO_PASS, TW_TM, TW_VP X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 13 Nov 2012 16:41:01 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qADGesMJ031778 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 13 Nov 2012 11:40:54 -0500 Received: from zalov.redhat.com (vpn1-4-95.ams2.redhat.com [10.36.4.95]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qADGeFvD015708 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 13 Nov 2012 11:40:35 -0500 Received: from zalov.cz (localhost [127.0.0.1]) by zalov.redhat.com (8.14.5/8.14.5) with ESMTP id qADGeEIe010591; Tue, 13 Nov 2012 17:40:14 +0100 Received: (from jakub@localhost) by zalov.cz (8.14.5/8.14.5/Submit) id qADGeEev010590; Tue, 13 Nov 2012 17:40:14 +0100 Date: Tue, 13 Nov 2012 17:40:14 +0100 From: Jakub Jelinek To: Wei Mi Cc: GCC Patches , David Li , Diego Novillo , Dodji Seketeli , Dmitry Vyukov Subject: Re: [tsan] ThreadSanitizer instrumentation part Message-ID: <20121113164014.GY1886@tucnak.redhat.com> Reply-To: Jakub Jelinek References: <20121103183906.GF1881@tucnak.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Mon, Nov 05, 2012 at 04:37:41PM -0800, Wei Mi wrote: > Thanks for the comments. I fix most of them except the setting of > TODO_.... The new patch.txt is attached. Please update the patch against trunk, it doesn't apply cleanly because of the asan commit. I took the liberty to do at least some formatting cleanups and other small tweaks against your patch to tsan.c. > >> + TODO_verify_all | TODO_update_ssa > > > > Ideally you shouldn't need TODO_update_ssa. > > > > I got error when I removed TODO_update_ssa, so I kept it. If it were just for the instrumentations, then you can easily update the vdef/vuse yourself, e.g. when inserting before a memory write, you can copy over the gimple_vuse of that write to gimple_vuse of the instrumentation call, create a new SSA_NAME for the gimple_vdef and and set the write's gimple_vuse to that new SSA_NAME. For call instrumentation it would be a tiny bit harder, but for the instrumentation of function entry/exit you'd need to find out the current vop at that point. So perhaps we can live with that, at least for now. > >> + | TODO_update_address_taken /* todo_flags_finish */ > > > > And why this? > > > > If we generate tsan_read(&a) for a non-address taken static variable > a, we need to change a to be address taken, right? That is complete misunderstanding of what update_address_taken does. It removes TREE_ADDRESSABLE from addressables that are no longer addressable, rather than adding TREE_ADDRESSABLE bits. For the latter there is mark_addressable function. > > Wrap builtin_decl_implicit in get_tsan_builtin_decl. If > builtin_decl_implicit return invalid decl, output error message and > then exit. I've moved that over just to the gate, eventually there should be a routine that will initialize the builtins if they aren't by the FE. > > Please avoid *'s at the beginning of comment continuation lines. > > Use is_ctrl_altering_stmt (stmt) to check whether the call must > > be the last stmt in a block or not. > > And, don't expect there is a single_succ_edge, there could be > > no edge at all (e.g. noreturn call), or there could be multiple > > edges. > > > > Fixed. Iterate every successive edge of current bb and insert stmt on > each edge. But wrongly, for one adding the same stmt to multiple basic blocks is going to crash terribly, but also you IMHO want to insert just on fallthru edge, if the routine throws or longjmps, the result isn't written. Jakub --- gcc/tsan.c.jj 2012-11-13 16:46:21.000000000 +0100 +++ gcc/tsan.c 2012-11-13 17:22:56.054837754 +0100 @@ -1,4 +1,4 @@ -/* GCC instrumentation plugin for ThreadSanitizer. +/* GCC instrumentation plugin for ThreadSanitizer. Copyright (C) 2011, 2012 Free Software Foundation, Inc. Contributed by Dmitry Vyukov @@ -44,36 +44,27 @@ along with GCC; see the file COPYING3. void __tsan_read/writeX (void *addr); */ static tree -get_tsan_builtin_decl (enum built_in_function fcode) -{ - tree decl = builtin_decl_implicit (fcode); - if (decl == NULL_TREE) - internal_error ("undefined builtin %s", built_in_names[fcode]); - return decl; -} - -static tree get_memory_access_decl (bool is_write, unsigned size) { enum built_in_function fcode; if (size <= 1) fcode = is_write ? BUILT_IN_TSAN_WRITE_1 - : BUILT_IN_TSAN_READ_1; + : BUILT_IN_TSAN_READ_1; else if (size <= 3) - fcode = is_write ? BUILT_IN_TSAN_WRITE_2 - : BUILT_IN_TSAN_READ_2; + fcode = is_write ? BUILT_IN_TSAN_WRITE_2 + : BUILT_IN_TSAN_READ_2; else if (size <= 7) - fcode = is_write ? BUILT_IN_TSAN_WRITE_4 - : BUILT_IN_TSAN_READ_4; + fcode = is_write ? BUILT_IN_TSAN_WRITE_4 + : BUILT_IN_TSAN_READ_4; else if (size <= 15) - fcode = is_write ? BUILT_IN_TSAN_WRITE_8 - : BUILT_IN_TSAN_READ_8; + fcode = is_write ? BUILT_IN_TSAN_WRITE_8 + : BUILT_IN_TSAN_READ_8; else - fcode = is_write ? BUILT_IN_TSAN_WRITE_16 - : BUILT_IN_TSAN_READ_16; + fcode = is_write ? BUILT_IN_TSAN_WRITE_16 + : BUILT_IN_TSAN_READ_16; - return get_tsan_builtin_decl (fcode); + return builtin_decl_implicit (fcode); } /* Check as to whether EXPR refers to a store to vptr. */ @@ -81,14 +72,14 @@ get_memory_access_decl (bool is_write, u static tree is_vptr_store (gimple stmt, tree expr, bool is_write) { - if (is_write == true + if (is_write == true && gimple_assign_single_p (stmt) && TREE_CODE (expr) == COMPONENT_REF) { tree field = TREE_OPERAND (expr, 1); if (TREE_CODE (field) == FIELD_DECL - && DECL_VIRTUAL_P (field)) - return gimple_assign_rhs1 (stmt); + && DECL_VIRTUAL_P (field)) + return gimple_assign_rhs1 (stmt); } return NULL; } @@ -96,7 +87,7 @@ is_vptr_store (gimple stmt, tree expr, b /* Checks as to whether EXPR refers to constant var/field/param. Don't bother to instrument them. */ -static bool +static bool is_load_of_const_p (tree expr, bool is_write) { if (is_write) @@ -108,7 +99,7 @@ is_load_of_const_p (tree expr, bool is_w || TREE_CODE (expr) == FIELD_DECL) { if (TREE_READONLY (expr)) - return true; + return true; } return false; } @@ -116,18 +107,14 @@ is_load_of_const_p (tree expr, bool is_w /* Instruments EXPR if needed. If any instrumentation is inserted, * return true. */ -static bool +static bool instrument_expr (gimple_stmt_iterator gsi, tree expr, bool is_write) { enum tree_code tcode; - unsigned fld_off, fld_size; tree base, rhs, expr_type, expr_ptr, builtin_decl; - basic_block bb, succ_bb; - edge_iterator ei; - edge e; + basic_block bb; HOST_WIDE_INT size; gimple stmt, g; - gimple_stmt_iterator start_gsi; location_t loc; base = get_base_address (expr); @@ -144,8 +131,8 @@ instrument_expr (gimple_stmt_iterator gs (DECL_P (expr) && DECL_ARTIFICIAL (expr)) /* The var does not live in memory -> no possibility of races. */ || (tcode == VAR_DECL - && !TREE_ADDRESSABLE (expr) - && TREE_STATIC (expr) == 0) + && !TREE_ADDRESSABLE (expr) + && TREE_STATIC (expr) == 0) /* Not implemented. */ || TREE_CODE (TREE_TYPE (expr)) == RECORD_TYPE /* Not implemented. */ @@ -156,22 +143,21 @@ instrument_expr (gimple_stmt_iterator gs || is_load_of_const_p (expr, is_write)) return false; - if (tcode == COMPONENT_REF) - { - tree field = TREE_OPERAND (expr, 1); - if (TREE_CODE (field) == FIELD_DECL) - { - fld_off = TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); - fld_size = TREE_INT_CST_LOW (DECL_SIZE (field)); - if (((fld_off % BITS_PER_UNIT) != 0) - || ((fld_size % BITS_PER_UNIT) != 0)) - { - /* As of now it crashes compilation. - TODO: handle bit-fields as if touching the whole field. */ - return false; - } - } - } + size = int_size_in_bytes (TREE_TYPE (expr)); + if (size == -1) + return false; + + /* For now just avoid instrumenting bit field acceses. + TODO: handle bit-fields as if touching the whole field. */ + HOST_WIDE_INT bitsize, bitpos; + tree offset; + enum machine_mode mode; + int volatilep = 0, unsignedp = 0; + get_inner_reference (expr, &bitsize, &bitpos, &offset, + &mode, &unsignedp, &volatilep, false); + if (bitpos % (size * BITS_PER_UNIT) + || bitsize != size * BITS_PER_UNIT) + return false; /* TODO: handle other cases (FIELD_DECL, ARRAY_RANGE_REF, TARGET_MEM_REF, ADDR_EXPR). */ @@ -186,44 +172,42 @@ instrument_expr (gimple_stmt_iterator gs loc = gimple_location (stmt); rhs = is_vptr_store (stmt, expr, is_write); gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr)); - expr_ptr = build_addr (unshare_expr (expr), - current_function_decl); + expr_ptr = build_fold_addr_expr (unshare_expr (expr)); if (rhs == NULL) { expr_type = TREE_TYPE (expr); while (TREE_CODE (expr_type) == ARRAY_TYPE) - expr_type = TREE_TYPE (expr_type); - size = int_size_in_bytes (expr_type); + expr_type = TREE_TYPE (expr_type); + size = int_size_in_bytes (expr_type); g = gimple_build_call (get_memory_access_decl (is_write, size), - 1, expr_ptr); + 1, expr_ptr); } else { - builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_VPTR_UPDATE); + builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE); g = gimple_build_call (builtin_decl, 1, expr_ptr); } - gimple_set_location (g, loc); + gimple_set_location (g, loc); /* Instrumentation for assignment of a function result must be inserted after the call. Instrumentation for reads of function arguments must be inserted before the call. That's because the call can contain synchronization. */ - if (is_gimple_call (stmt) && is_write) + if (is_gimple_call (stmt) && is_write) { /* If the call can throw, it must be the last stmt in - a basicblock, so the instrumented stmts need to be - inserted in successor bbs. */ - if (is_ctrl_altering_stmt (stmt)) - { - bb = gsi_bb (gsi); - FOR_EACH_EDGE (e, ei, bb->succs) - { - succ_bb = split_edge (e); - start_gsi = gsi_start_bb (succ_bb); - gsi_insert_after (&start_gsi, g, GSI_NEW_STMT); - } - } + a basic block, so the instrumented stmts need to be + inserted in successor bbs. */ + if (is_ctrl_altering_stmt (stmt)) + { + edge e; + + bb = gsi_bb (gsi); + e = find_fallthru_edge (bb->succs); + if (e) + gsi_insert_seq_on_edge_immediate (e, g); + } else - gsi_insert_after (&gsi, g, GSI_NEW_STMT); + gsi_insert_after (&gsi, g, GSI_NEW_STMT); } else gsi_insert_before (&gsi, g, GSI_SAME_STMT); @@ -242,22 +226,22 @@ instrument_gimple (gimple_stmt_iterator bool instrumented = false; stmt = gsi_stmt (gsi); - if (is_gimple_call (stmt) - && (gimple_call_fndecl (stmt) - != get_tsan_builtin_decl (BUILT_IN_TSAN_INIT))) - return true; + if (is_gimple_call (stmt) + && (gimple_call_fndecl (stmt) + != builtin_decl_implicit (BUILT_IN_TSAN_INIT))) + return true; else if (is_gimple_assign (stmt)) { if (gimple_store_p (stmt)) - { - lhs = gimple_assign_lhs (stmt); - instrumented = instrument_expr (gsi, lhs, true); - } + { + lhs = gimple_assign_lhs (stmt); + instrumented = instrument_expr (gsi, lhs, true); + } if (gimple_assign_load_p (stmt)) - { - rhs = gimple_assign_rhs1 (stmt); - instrumented = instrument_expr (gsi, rhs, false); - } + { + rhs = gimple_assign_rhs1 (stmt); + instrumented = instrument_expr (gsi, rhs, false); + } } return instrumented; } @@ -265,7 +249,7 @@ instrument_gimple (gimple_stmt_iterator /* Instruments all interesting memory accesses in the current function. * Return true if func entry/exit should be instrumented. */ -static bool +static bool instrument_memory_accesses (void) { basic_block bb; @@ -291,15 +275,15 @@ instrument_func_entry (void) succ_bb = single_succ (ENTRY_BLOCK_PTR); gsi = gsi_after_labels (succ_bb); - builtin_decl = get_tsan_builtin_decl (BUILT_IN_RETURN_ADDRESS); + builtin_decl = builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS); g = gimple_build_call (builtin_decl, 1, integer_zero_node); - ret_addr = make_ssa_name (ptr_type_node, NULL); + ret_addr = make_ssa_name (ptr_type_node, NULL); gimple_call_set_lhs (g, ret_addr); gimple_set_location (g, cfun->function_start_locus); gsi_insert_before (&gsi, g, GSI_SAME_STMT); - builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_FUNC_ENTRY); - g = gimple_build_call (builtin_decl, 1, ret_addr); + builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_FUNC_ENTRY); + g = gimple_build_call (builtin_decl, 1, ret_addr); gimple_set_location (g, cfun->function_start_locus); gsi_insert_before (&gsi, g, GSI_SAME_STMT); } @@ -325,7 +309,7 @@ instrument_func_exit (void) stmt = gsi_stmt (gsi); gcc_assert (gimple_code (stmt) == GIMPLE_RETURN); loc = gimple_location (stmt); - builtin_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_FUNC_EXIT); + builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_FUNC_EXIT); g = gimple_build_call (builtin_decl, 0); gimple_set_location (g, loc); gsi_insert_before (&gsi, g, GSI_SAME_STMT); @@ -350,70 +334,71 @@ tsan_pass (void) static bool tsan_gate (void) { - return flag_tsan != 0; + return flag_tsan != 0 && builtin_decl_implicit_p (BUILT_IN_TSAN_INIT); } /* Inserts __tsan_init () into the list of CTORs. */ -void +void tsan_finish_file (void) { tree ctor_statements; tree init_decl; ctor_statements = NULL_TREE; - init_decl = get_tsan_builtin_decl (BUILT_IN_TSAN_INIT); + init_decl = builtin_decl_implicit (BUILT_IN_TSAN_INIT); append_to_statement_list (build_call_expr (init_decl, 0), - &ctor_statements); + &ctor_statements); cgraph_build_static_cdtor ('I', ctor_statements, - MAX_RESERVED_INIT_PRIORITY - 1); + MAX_RESERVED_INIT_PRIORITY - 1); } /* The pass descriptor. */ -struct gimple_opt_pass pass_tsan = +struct gimple_opt_pass pass_tsan = { { GIMPLE_PASS, - "tsan", /* name */ - tsan_gate, /* gate */ - tsan_pass, /* execute */ - NULL, /* sub */ - NULL, /* next */ - 0, /* static_pass_number */ - TV_NONE, /* tv_id */ - PROP_ssa | PROP_cfg, /* properties_required */ - 0, /* properties_provided */ - 0, /* properties_destroyed */ - 0, /* todo_flags_start */ + "tsan", /* name */ + OPTGROUP_NONE, /* optinfo_flags */ + tsan_gate, /* gate */ + tsan_pass, /* execute */ + NULL, /* sub */ + NULL, /* next */ + 0, /* static_pass_number */ + TV_NONE, /* tv_id */ + PROP_ssa | PROP_cfg, /* properties_required */ + 0, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ TODO_verify_all | TODO_update_ssa - | TODO_update_address_taken /* todo_flags_finish */ + | TODO_update_address_taken /* todo_flags_finish */ } }; -static bool +static bool tsan_gate_O0 (void) -{ - return flag_tsan != 0 && !optimize; -} +{ + return flag_tsan != 0 && !optimize; +} -struct gimple_opt_pass pass_tsan_O0 = +struct gimple_opt_pass pass_tsan_O0 = { { GIMPLE_PASS, - "tsan0", /* name */ - tsan_gate_O0, /* gate */ - tsan_pass, /* execute */ - NULL, /* sub */ - NULL, /* next */ - 0, /* static_pass_number */ - TV_NONE, /* tv_id */ - PROP_ssa | PROP_cfg, /* properties_required */ - 0, /* properties_provided */ - 0, /* properties_destroyed */ - 0, /* todo_flags_start */ + "tsan0", /* name */ + OPTGROUP_NONE, /* optinfo_flags */ + tsan_gate_O0, /* gate */ + tsan_pass, /* execute */ + NULL, /* sub */ + NULL, /* next */ + 0, /* static_pass_number */ + TV_NONE, /* tv_id */ + PROP_ssa | PROP_cfg, /* properties_required */ + 0, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ TODO_verify_all | TODO_update_ssa - | TODO_update_address_taken /* todo_flags_finish */ + | TODO_update_address_taken /* todo_flags_finish */ } }; -