From patchwork Thu Dec 22 17:03:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Martin_Li=C5=A1ka?= X-Patchwork-Id: 88885 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp2909676qgi; Thu, 22 Dec 2016 09:04:21 -0800 (PST) X-Received: by 10.99.22.65 with SMTP id 1mr18650033pgw.70.1482426261338; Thu, 22 Dec 2016 09:04:21 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id 127si7044334pgg.4.2016.12.22.09.04.21 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Dec 2016 09:04:21 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-444991-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-444991-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-444991-patch=linaro.org@gcc.gnu.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=ysqWlir+lF9A3gYIy idqlqaKtitqx+jAh/l/Ns1r0gXXBd7APoL/7MhYy/RsXUEP3rGTagGSXfq1rAdZ+ LUxG68OYDbOVkA6zkUD/EpK0jgzCKGAMRwKA9SFnomCZ7LQ/HQU0a5fCLotjvyKV 7fDZ7T1f0/6IHXVh6UaCbixBF4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=MFWQq8Lb9a0ODadCHq0REEV 4BDU=; b=G8QAcgFa7SJAvTfof4WtQvZ9yOdNfNay02ibjs9Upv7P/0jYMu9+ipZ tSIHftjWDTIeMCvlaKadY5bp4VYTrzflxmFT+w9clvKBs/jvgAeq1LERfV28xBH4 46yMiHoo3ISls0yD/4eXkegjnjXT4wqg++CkXVWMd2JiK3sWkIsE= Received: (qmail 103742 invoked by alias); 22 Dec 2016 17:04:05 -0000 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 Received: (qmail 103733 invoked by uid 89); 22 Dec 2016 17:04:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=BAYES_00, SPF_PASS, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 spammy=0.00, Speed, hash_map, rewritten X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 22 Dec 2016 17:03:53 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5866EABB4; Thu, 22 Dec 2016 17:03:51 +0000 (UTC) Subject: Re: [PATCH] Speed-up use-after-scope (re-writing to SSA) (version 2) To: Jakub Jelinek References: <20161102125609.GQ3541@tucnak.redhat.com> <20161102130612.GR3541@tucnak.redhat.com> <774a5d54-30f6-3212-ea4c-21e751356055@suse.cz> <20161116130733.GT3541@tucnak.redhat.com> <469bf86a-e43c-c571-66e4-87db78b6fb11@suse.cz> <20161116162841.GX3541@tucnak.redhat.com> <20161221085200.GS21933@tucnak> Cc: Richard Biener , GCC Patches From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <4ec48432-9df6-154a-1b13-065b9772cbbf@suse.cz> Date: Thu, 22 Dec 2016 18:03:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20161221085200.GS21933@tucnak> X-IsSubscribed: yes On 12/21/2016 09:52 AM, Jakub Jelinek wrote: > On Tue, Dec 20, 2016 at 12:26:41PM +0100, Martin Liška wrote: >> Ok, llvm folks are unwilling to accept the new API function, thus I've decided to come up >> with approach suggested by Jakub. Briefly, when expanding ASAN_POISON internal function, >> we create a new variable (with the same name as the original one). The variable is poisoned >> at the location of the ASAN_POISON and all usages just call ASAN_CHECK that would trigger >> use-after-scope run-time error. Situation where ASAN_POISON has a LHS is very rare and >> is very likely to be a bug. Thus suggested not super-optimized approach should not be >> problematic. > > Do you have a testcase for the case where there is a write to the var after > poison that is then made non-addressable? use-after-scope-9.c only covers > the read. > >> I'm not sure about the introduction of 'create_var' function, maybe we would need some >> refactoring. Thoughts? > > It doesn't belong to gimple-expr.c and the name is way too generic, we have > many create var functions already. And this one is very specialized. > >> 2016-12-19 Martin Liska >> >> * asan.c (asan_expand_poison_ifn): New function. >> * asan.h (asan_expand_poison_ifn): Likewise. > > Too many spaces. > >> + tree shadow_var = create_var (TREE_TYPE (poisoned_var), >> + IDENTIFIER_POINTER (DECL_NAME (var_decl))); > > For the shadow var creation, IMHO you should > 1) use a hash table, once you add a shadow variable for a certain variable > for the first time, reuse it for all the other cases; you can have many > ASAN_POISON () calls for the same underlying variable Thanks for review. Done by hash_map. > 2) as I said, use just a function in sanopt.c for this, > create_asan_shadow_var or whatever Also done. > 3) I think you just want to do copy_node, plus roughly what > copy_decl_for_dup_finish does (and set DECL_ARTIFICIAL and > DECL_IGNORED_P) - except that you don't have copy_body_data > so you can't use it directly (well, you could create copy_body_data > just for that purpose and set src_fn and dst_fn to current_function_decl > and the rest to NULL) I decided to use the function with prepared copy_body_data ;) > > I'd really like to see the storing to poisoned var becoming non-addressable > in action (if it can ever happen, so it isn't just theoretical) to look at > what it does. Well, having following sample: int main (int argc, char **argv) { int *ptr = 0; { int a; ptr = &a; *ptr = 12345; } *ptr = 12345; return *ptr; } Right after rewriting into SSA it looks as follows: main (int argc, char * * argv) { int a; int * ptr; int _8; [0.00%]: a_9 = 12345; a_10 = ASAN_POISON (); a_11 = 12345; _8 = a_11; return _8; } Thus, I guess it not possible to do a write. Following v2 can bootstrap on ppc64le-redhat-linux and survives regression tests. Martin > > Jakub > >From 66fabb9d15ebfb21e25b4fc81bad8deb6877e198 Mon Sep 17 00:00:00 2001 From: marxin Date: Mon, 19 Dec 2016 15:36:11 +0100 Subject: [PATCH] Speed up use-after-scope (v2): rewrite into SSA gcc/ChangeLog: 2016-12-22 Martin Liska * asan.c (create_asan_shadow_var): New function. (asan_expand_poison_ifn): Likewise. * asan.h (asan_expand_poison_ifn): New declaration. * internal-fn.c (expand_ASAN_POISON): Likewise. * internal-fn.def (ASAN_POISON): New builtin. * sanopt.c (pass_sanopt::execute): Expand asan_expand_poison_ifn. * tree-inline.c (copy_decl_for_dup_finish): Make function external. * tree-inline.h (copy_decl_for_dup_finish): Likewise. * tree-ssa.c (is_asan_mark_p): New function. (execute_update_addresses_taken): Rewrite local variables (identified just by use-after-scope as addressable) into SSA. gcc/testsuite/ChangeLog: 2016-12-22 Martin Liska * gcc.dg/asan/use-after-scope-3.c: Add additional flags. * gcc.dg/asan/use-after-scope-9.c: Likewise and grep for sanopt optimization for ASAN_POISON. --- gcc/asan.c | 109 +++++++++++++++++++++++++- gcc/asan.h | 2 + gcc/internal-fn.c | 7 ++ gcc/internal-fn.def | 1 + gcc/sanopt.c | 11 +++ gcc/testsuite/gcc.dg/asan/use-after-scope-3.c | 1 + gcc/testsuite/gcc.dg/asan/use-after-scope-9.c | 2 + gcc/tree-inline.c | 2 +- gcc/tree-inline.h | 1 + gcc/tree-ssa.c | 69 +++++++++++++--- 10 files changed, 193 insertions(+), 12 deletions(-) diff --git a/gcc/asan.c b/gcc/asan.c index 53acff0a2fb..187934ad11b 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -32,8 +32,8 @@ along with GCC; see the file COPYING3. If not see #include "tree-pass.h" #include "memmodel.h" #include "tm_p.h" +#include "ssa.h" #include "stringpool.h" -#include "tree-vrp.h" #include "tree-ssanames.h" #include "optabs.h" #include "emit-rtl.h" @@ -59,6 +59,7 @@ along with GCC; see the file COPYING3. If not see #include "params.h" #include "builtins.h" #include "fnmatch.h" +#include "tree-inline.h" /* AddressSanitizer finds out-of-bounds and use-after-free bugs with <2x slowdown on average. @@ -3055,6 +3056,112 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) return true; } +/* Create ASAN shadow variable for a VAR_DECL which has been rewritten + into SSA. Already seen VAR_DECLs are stored in SHADOW_VARS_MAPPING. */ + +static tree +create_asan_shadow_var (tree var_decl, + hash_map &shadow_vars_mapping) +{ + tree *slot = shadow_vars_mapping.get (var_decl); + if (slot == NULL) + { + tree shadow_var = copy_node (var_decl); + + copy_body_data id; + memset (&id, 0, sizeof (copy_body_data)); + id.src_fn = id.dst_fn = current_function_decl; + copy_decl_for_dup_finish (&id, var_decl, shadow_var); + + DECL_ARTIFICIAL (shadow_var) = 1; + DECL_IGNORED_P (shadow_var) = 1; + DECL_SEEN_IN_BIND_EXPR_P (shadow_var) = 0; + gimple_add_tmp_var (shadow_var); + + shadow_vars_mapping.put (var_decl, shadow_var); + return shadow_var; + } + else + return *slot; +} + +bool +asan_expand_poison_ifn (gimple_stmt_iterator *iter, + bool *need_commit_edge_insert, + hash_map &shadow_vars_mapping) +{ + gimple *g = gsi_stmt (*iter); + tree poisoned_var = gimple_call_lhs (g); + if (!poisoned_var) + { + gsi_remove (iter, true); + return true; + } + + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), + shadow_vars_mapping); + + bool recover_p; + if (flag_sanitize & SANITIZE_USER_ADDRESS) + recover_p = (flag_sanitize_recover & SANITIZE_USER_ADDRESS) != 0; + else + recover_p = (flag_sanitize_recover & SANITIZE_KERNEL_ADDRESS) != 0; + tree size = DECL_SIZE_UNIT (shadow_var); + gimple *poison_call + = gimple_build_call_internal (IFN_ASAN_MARK, 3, + build_int_cst (integer_type_node, + ASAN_MARK_POISON), + build_fold_addr_expr (shadow_var), size); + + use_operand_p use_p; + imm_use_iterator imm_iter; + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) + { + gimple *use = USE_STMT (use_p); + if (is_gimple_debug (use)) + continue; + + int nargs; + tree fun = report_error_func (false, recover_p, tree_to_uhwi (size), + &nargs); + + gcall *call = gimple_build_call (fun, 1, + build_fold_addr_expr (shadow_var)); + gimple_set_location (call, gimple_location (use)); + gimple *call_to_insert = call; + + /* The USE can be a gimple PHI node. If so, insert the call on + all edges leading to the PHI node. */ + if (is_a (use)) + { + gphi *phi = dyn_cast (use); + for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) + if (gimple_phi_arg_def (phi, i) == poisoned_var) + { + edge e = gimple_phi_arg_edge (phi, i); + + if (call_to_insert == NULL) + call_to_insert = gimple_copy (call); + + gsi_insert_seq_on_edge (e, call_to_insert); + *need_commit_edge_insert = true; + call_to_insert = NULL; + } + } + else + { + gimple_stmt_iterator gsi = gsi_for_stmt (use); + gsi_insert_before (&gsi, call, GSI_NEW_STMT); + } + } + + SSA_NAME_IS_DEFAULT_DEF (poisoned_var) = true; + SSA_NAME_DEF_STMT (poisoned_var) = gimple_build_nop (); + gsi_replace (iter, poison_call, false); + + return true; +} + /* Instrument the current function. */ static unsigned int diff --git a/gcc/asan.h b/gcc/asan.h index 355a350bfeb..3800aabaf8b 100644 --- a/gcc/asan.h +++ b/gcc/asan.h @@ -30,6 +30,8 @@ extern void initialize_sanitizer_builtins (void); extern tree asan_dynamic_init_call (bool); extern bool asan_expand_check_ifn (gimple_stmt_iterator *, bool); extern bool asan_expand_mark_ifn (gimple_stmt_iterator *); +extern bool asan_expand_poison_ifn (gimple_stmt_iterator *, bool *, + hash_map &); extern gimple_stmt_iterator create_cond_insert_point (gimple_stmt_iterator *, bool, bool, bool, basic_block *, basic_block *); diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c index b1dbc988b9c..40f5fe6c69c 100644 --- a/gcc/internal-fn.c +++ b/gcc/internal-fn.c @@ -380,6 +380,13 @@ expand_ASAN_MARK (internal_fn, gcall *) gcc_unreachable (); } +/* This should get expanded in the sanopt pass. */ + +static void +expand_ASAN_POISON (internal_fn, gcall *) +{ + gcc_unreachable (); +} /* This should get expanded in the tsan pass. */ diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 9a03e17be23..81a6bbdd15c 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -167,6 +167,7 @@ DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL) DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".R...") DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..") +DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) diff --git a/gcc/sanopt.c b/gcc/sanopt.c index ae716cffcf4..a19c3a1b6dd 100644 --- a/gcc/sanopt.c +++ b/gcc/sanopt.c @@ -894,6 +894,8 @@ pass_sanopt::execute (function *fun) bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD; + hash_map shadow_vars_mapping; + bool need_commit_edge_insert = false; FOR_EACH_BB_FN (bb, fun) { gimple_stmt_iterator gsi; @@ -931,6 +933,11 @@ pass_sanopt::execute (function *fun) case IFN_ASAN_MARK: no_next = asan_expand_mark_ifn (&gsi); break; + case IFN_ASAN_POISON: + no_next = asan_expand_poison_ifn (&gsi, + &need_commit_edge_insert, + shadow_vars_mapping); + break; default: break; } @@ -962,6 +969,10 @@ pass_sanopt::execute (function *fun) gsi_next (&gsi); } } + + if (need_commit_edge_insert) + gsi_commit_edge_inserts (); + return 0; } diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c index 9aeed51a770..8b11bea9940 100644 --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c @@ -1,5 +1,6 @@ // { dg-do run } // { dg-shouldfail "asan" } +// { dg-additional-options "-O0" } int main (void) diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c index 2e30deffa18..5d069dd18ea 100644 --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c @@ -1,5 +1,6 @@ // { dg-do run } // { dg-shouldfail "asan" } +// { dg-additional-options "-O2 -fdump-tree-asan1" } int main (int argc, char **argv) @@ -15,6 +16,7 @@ main (int argc, char **argv) return *ptr; } +// { dg-final { scan-tree-dump-times "= ASAN_POISON \\(\\)" 1 "asan1" } } // { dg-output "ERROR: AddressSanitizer: stack-use-after-scope on address.*(\n|\r\n|\r)" } // { dg-output "READ of size .*" } // { dg-output ".*'a' <== Memory access at offset \[0-9\]* is inside this variable.*" } diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 0de0b89dbbf..7666320ec00 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -5446,7 +5446,7 @@ declare_inline_vars (tree block, tree vars) but now it will be in the TO_FN. PARM_TO_VAR means enable PARM_DECL to VAR_DECL translation. */ -static tree +tree copy_decl_for_dup_finish (copy_body_data *id, tree decl, tree copy) { /* Don't generate debug information for the copy if we wouldn't have diff --git a/gcc/tree-inline.h b/gcc/tree-inline.h index 9ca2a91f08f..3e55a0ae0e5 100644 --- a/gcc/tree-inline.h +++ b/gcc/tree-inline.h @@ -218,6 +218,7 @@ extern gimple_seq copy_gimple_seq_and_replace_locals (gimple_seq seq); extern bool debug_find_tree (tree, tree); extern tree copy_fn (tree, tree&, tree&); extern const char *copy_forbidden (struct function *fun); +extern tree copy_decl_for_dup_finish (copy_body_data *id, tree decl, tree copy); /* This is in tree-inline.c since the routine uses data structures from the inliner. */ diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index 62eea8bb8a4..3adbef48037 100644 --- a/gcc/tree-ssa.c +++ b/gcc/tree-ssa.c @@ -41,6 +41,7 @@ along with GCC; see the file COPYING3. If not see #include "cfgexpand.h" #include "tree-cfg.h" #include "tree-dfa.h" +#include "asan.h" /* Pointer map of variable mappings, keyed by edge. */ static hash_map > *edge_var_maps; @@ -1550,6 +1551,30 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, } } +/* Return true when STMT is ASAN mark where second argument is an address + of a local variable. */ + +static bool +is_asan_mark_p (gimple *stmt) +{ + if (!gimple_call_internal_p (stmt, IFN_ASAN_MARK)) + return false; + + tree addr = get_base_address (gimple_call_arg (stmt, 1)); + if (TREE_CODE (addr) == ADDR_EXPR + && VAR_P (TREE_OPERAND (addr, 0))) + { + tree var = TREE_OPERAND (addr, 0); + unsigned addressable = TREE_ADDRESSABLE (var); + TREE_ADDRESSABLE (var) = 0; + bool r = is_gimple_reg (var); + TREE_ADDRESSABLE (var) = addressable; + return r; + } + + return false; +} + /* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. */ void @@ -1575,17 +1600,23 @@ execute_update_addresses_taken (void) enum gimple_code code = gimple_code (stmt); tree decl; - if (code == GIMPLE_CALL - && optimize_atomic_compare_exchange_p (stmt)) + if (code == GIMPLE_CALL) { - /* For __atomic_compare_exchange_N if the second argument - is &var, don't mark var addressable; - if it becomes non-addressable, we'll rewrite it into - ATOMIC_COMPARE_EXCHANGE call. */ - tree arg = gimple_call_arg (stmt, 1); - gimple_call_set_arg (stmt, 1, null_pointer_node); - gimple_ior_addresses_taken (addresses_taken, stmt); - gimple_call_set_arg (stmt, 1, arg); + if (optimize_atomic_compare_exchange_p (stmt)) + { + /* For __atomic_compare_exchange_N if the second argument + is &var, don't mark var addressable; + if it becomes non-addressable, we'll rewrite it into + ATOMIC_COMPARE_EXCHANGE call. */ + tree arg = gimple_call_arg (stmt, 1); + gimple_call_set_arg (stmt, 1, null_pointer_node); + gimple_ior_addresses_taken (addresses_taken, stmt); + gimple_call_set_arg (stmt, 1, arg); + } + else if (is_asan_mark_p (stmt)) + ; + else + gimple_ior_addresses_taken (addresses_taken, stmt); } else /* Note all addresses taken by the stmt. */ @@ -1841,6 +1872,24 @@ execute_update_addresses_taken (void) continue; } } + else if (is_asan_mark_p (stmt)) + { + tree var = TREE_OPERAND (gimple_call_arg (stmt, 1), 0); + if (bitmap_bit_p (suitable_for_renaming, DECL_UID (var))) + { + unlink_stmt_vdef (stmt); + if (asan_mark_p (stmt, ASAN_MARK_POISON)) + { + gcall *call + = gimple_build_call_internal (IFN_ASAN_POISON, 0); + gimple_call_set_lhs (call, var); + gsi_replace (&gsi, call, GSI_SAME_STMT); + } + else + gsi_remove (&gsi, true); + continue; + } + } for (i = 0; i < gimple_call_num_args (stmt); ++i) { tree *argp = gimple_call_arg_ptr (stmt, i); -- 2.11.0