From patchwork Tue Jan 17 16:16:44 2017 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: 716282 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3v2wHk1Vxkz9sCM for ; Wed, 18 Jan 2017 03:17:51 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="PJ4pEXtq"; dkim-atps=neutral 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=a1St5IN+XH2G4rnXd BMmXPZUSTxexqkYThavxvuLX/SPgCJAf0oiG661ZLMOsEo69qJMl9uQFa9Fs5QQi DQ8qOX1SCb8H5OS+QTJvuYfKpdcDW2fkEMPbr2r7/pHdzCXjow394tJF09B184Ah hqMlbVtjr8D9H9n0fQR0Lynuus= 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=3iNJd64VgZLjEBZ2RTDOS8b Q2yA=; b=PJ4pEXtqkP+YfTQrk7sec/jkVXgLlUuH3eNOF7CGT6aUNbZGp6Exyqm rlleF7C/Rgv73TBDWe3mDChL53jzVFjJyhQ7VdjgJcXt/X/u9eGaMyMjlKZwCmuF nuqhfDHfviyMxOUzHKc3AdRj7qSat4dQIntKOVNaw67+qBo8b9/E= Received: (qmail 61886 invoked by alias); 17 Jan 2017 16:16:59 -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 61784 invoked by uid 89); 17 Jan 2017 16:16:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, SPF_PASS autolearn=ham version=3.3.2 spammy=*a, Something, H*M:8c77, GSI 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; Tue, 17 Jan 2017 16:16:47 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id D02BCAD95; Tue, 17 Jan 2017 16:16:44 +0000 (UTC) Subject: Re: [PATCH] Speed-up use-after-scope (re-writing to SSA) (version 2) To: Jakub Jelinek References: <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> <4ec48432-9df6-154a-1b13-065b9772cbbf@suse.cz> <20161222172140.GF21933@tucnak> <29d32a7c-a95d-ddb1-d64e-ae8f659d3a4b@suse.cz> <20170116142025.GO1867@tucnak> Cc: Richard Biener , GCC Patches From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <7e7f795d-a7a7-584e-8c77-61ea01207c40@suse.cz> Date: Tue, 17 Jan 2017 17:16:44 +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: <20170116142025.GO1867@tucnak> X-IsSubscribed: yes On 01/16/2017 03:20 PM, Jakub Jelinek wrote: > On Mon, Jan 09, 2017 at 03:58:04PM +0100, Martin Liška wrote: >>>> Well, having following sample: >>>> >>>> int >>>> main (int argc, char **argv) >>>> { >>>> int *ptr = 0; >>>> >>>> { >>>> int a; >>>> ptr = &a; >>>> *ptr = 12345; >>>> } >>>> >>>> *ptr = 12345; >>>> return *ptr; >>>> } >>>> > >> I'm still not sure how to do that. Problem is that transformation from: >> >> ASAN_MARK (UNPOISON, &a, 4); >> a = 5; >> ASAN_MARK (POISON, &a, 4); >> >> to >> >> a_8 = 5; >> a_9 = ASAN_POISON (); >> >> happens in tree-ssa.c, after SSA is created, in situation where we prove the 'a' >> does not need to live in memory. Thus said, question is how to identify that we >> need to transform into SSA in a different way: >> >> a_10 = ASAN_POISON (); >> ASAN_POISON (a_10); > > I meant something like this (completely untested, and without the testcase > added to the testsuite). > The incremental patch as is relies on the ASAN_POISON_USE call having the > argument the result of ASAN_POISON, it would ICE if that is not the case > (especially if -fsanitize-recover=address). Dunno if some optimization > might decide to create a PHI in between, say merge two unrelated vars for > if (something) > { > x_1 = ASAN_POISON (); > ... > ASAN_POISON_USE (x_1); > } > else > { > y_2 = ASAN_POISON (); > ... > ASAN_POISON_USE (y_2); > } > to turn that into: > if (something) > x_1 = ASAN_POISON (); > else > y_2 = ASAN_POISON (); > _3 = PHI ; > ... > ASAN_POISON_USE (_3); > > If it did, we would ICE because ASAN_POISON_USE would survive this way until > expansion. A quick fix for the ICE (if it can ever happen) would be easy, > in sanopt remove ASAN_POISON_USE calls which have argument that is not lhs > of ASAN_POISON (all other ASAN_POISON_USE calls will be handled by my > incremental patch). Of course that would also mean in that case we'd report > a read rather than write. But if it can't happen or is very unlikely to > happen, then it is a non-issue. Thank you Jakub for working on that. The patch is fine, I added DCE support and a test-case. Please see attached patch. asan.exp regression tests look fine and I've been building linux kernel with KASAN enabled. I'll also do asan-boostrap. I would like to commit the patch soon, should I squash both patches together, or would it be preferred to separate basic optimization and support for stores? Thanks, Martin > Something missing from the patch is some change in DCE to remove ASAN_POISON > calls without lhs earlier. I think we can't make ASAN_POISON ECF_CONST, we > don't want it to be merged for different variables. > > --- gcc/internal-fn.def.jj 2017-01-16 13:19:49.000000000 +0100 > +++ gcc/internal-fn.def 2017-01-16 14:25:37.427962196 +0100 > @@ -167,6 +167,7 @@ DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, EC > 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 (ASAN_POISON_USE, 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) > --- gcc/asan.c.jj 2017-01-16 13:19:49.000000000 +0100 > +++ gcc/asan.c 2017-01-16 14:52:34.022044223 +0100 > @@ -3094,6 +3094,8 @@ create_asan_shadow_var (tree var_decl, > return *slot; > } > > +/* Expand ASAN_POISON ifn. */ > + > bool > asan_expand_poison_ifn (gimple_stmt_iterator *iter, > bool *need_commit_edge_insert, > @@ -3107,8 +3109,8 @@ asan_expand_poison_ifn (gimple_stmt_iter > return true; > } > > - tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), > - shadow_vars_mapping); > + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), > + shadow_vars_mapping); > > bool recover_p; > if (flag_sanitize & SANITIZE_USER_ADDRESS) > @@ -3122,16 +3124,16 @@ asan_expand_poison_ifn (gimple_stmt_iter > ASAN_MARK_POISON), > build_fold_addr_expr (shadow_var), size); > > - use_operand_p use_p; > + gimple *use; > imm_use_iterator imm_iter; > - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) > + FOR_EACH_IMM_USE_STMT (use, 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), > + bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE); > + tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size), > &nargs); > > gcall *call = gimple_build_call (fun, 1, > @@ -3160,7 +3162,10 @@ asan_expand_poison_ifn (gimple_stmt_iter > else > { > gimple_stmt_iterator gsi = gsi_for_stmt (use); > - gsi_insert_before (&gsi, call, GSI_NEW_STMT); > + if (store_p) > + gsi_replace (&gsi, call, true); > + else > + gsi_insert_before (&gsi, call, GSI_NEW_STMT); > } > } > > --- gcc/tree-into-ssa.c.jj 2017-01-01 12:45:35.000000000 +0100 > +++ gcc/tree-into-ssa.c 2017-01-16 14:32:14.853808726 +0100 > @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. > #include "tree-ssa.h" > #include "domwalk.h" > #include "statistics.h" > +#include "asan.h" > > #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y)) > > @@ -1807,6 +1808,26 @@ maybe_replace_use_in_debug_stmt (use_ope > } > > > +/* If DEF has x_5 = ASAN_POISON () as its current def, add > + ASAN_POISON_USE (x_5) stmt before GSI to denote the stmt writes into > + a poisoned (out of scope) variable. */ > + > +static void > +maybe_add_asan_poison_write (tree def, gimple_stmt_iterator *gsi) > +{ > + tree cdef = get_current_def (def); > + if (cdef != NULL > + && TREE_CODE (cdef) == SSA_NAME > + && gimple_call_internal_p (SSA_NAME_DEF_STMT (cdef), IFN_ASAN_POISON)) > + { > + gcall *call > + = gimple_build_call_internal (IFN_ASAN_POISON_USE, 1, cdef); > + gimple_set_location (call, gimple_location (gsi_stmt (*gsi))); > + gsi_insert_before (gsi, call, GSI_SAME_STMT); > + } > +} > + > + > /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES > or OLD_SSA_NAMES, or if it is a symbol marked for renaming, > register it as the current definition for the names replaced by > @@ -1837,7 +1858,11 @@ maybe_register_def (def_operand_p def_p, > def = get_or_create_ssa_default_def (cfun, sym); > } > else > - def = make_ssa_name (def, stmt); > + { > + if (asan_sanitize_use_after_scope ()) > + maybe_add_asan_poison_write (def, &gsi); > + def = make_ssa_name (def, stmt); > + } > SET_DEF (def_p, def); > > tree tracked_var = target_for_debug_bind (sym); > --- gcc/internal-fn.c.jj 2017-01-16 13:19:49.000000000 +0100 > +++ gcc/internal-fn.c 2017-01-16 14:26:10.828529039 +0100 > @@ -388,6 +388,14 @@ expand_ASAN_POISON (internal_fn, gcall * > gcc_unreachable (); > } > > +/* This should get expanded in the sanopt pass. */ > + > +static void > +expand_ASAN_POISON_USE (internal_fn, gcall *) > +{ > + gcc_unreachable (); > +} > + > /* This should get expanded in the tsan pass. */ > > static void > > > Jakub > From c30802f6a29390a83208bfdb1090a6378ed42691 Mon Sep 17 00:00:00 2001 From: marxin Date: Tue, 17 Jan 2017 16:49:29 +0100 Subject: [PATCH] use-after-scope: handle writes to a poisoned variable gcc/testsuite/ChangeLog: 2017-01-17 Martin Liska * gcc.dg/asan/use-after-scope-10.c: New test. gcc/ChangeLog: 2017-01-16 Jakub Jelinek * asan.c (asan_expand_poison_ifn): Support stores and use appropriate ASAN report function. * internal-fn.c (expand_ASAN_POISON_USE): New function. * internal-fn.def (ASAN_POISON_USE): Declare. * tree-into-ssa.c (maybe_add_asan_poison_write): New function. (maybe_register_def): Create ASAN_POISON_USE when sanitizing. * tree-ssa-dce.c (eliminate_unnecessary_stmts): Remove ASAN_POISON calls w/o LHS. --- gcc/asan.c | 19 +++++++++++------- gcc/internal-fn.c | 8 ++++++++ gcc/internal-fn.def | 1 + gcc/testsuite/gcc.dg/asan/use-after-scope-10.c | 22 +++++++++++++++++++++ gcc/tree-into-ssa.c | 27 +++++++++++++++++++++++++- gcc/tree-ssa-dce.c | 4 ++++ 6 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/asan/use-after-scope-10.c diff --git a/gcc/asan.c b/gcc/asan.c index fe117a6951a..486ebfdb6af 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -3094,6 +3094,8 @@ create_asan_shadow_var (tree var_decl, return *slot; } +/* Expand ASAN_POISON ifn. */ + bool asan_expand_poison_ifn (gimple_stmt_iterator *iter, bool *need_commit_edge_insert, @@ -3107,8 +3109,8 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, return true; } - tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), - shadow_vars_mapping); + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), + shadow_vars_mapping); bool recover_p; if (flag_sanitize & SANITIZE_USER_ADDRESS) @@ -3122,16 +3124,16 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, ASAN_MARK_POISON), build_fold_addr_expr (shadow_var), size); - use_operand_p use_p; + gimple *use; imm_use_iterator imm_iter; - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) + FOR_EACH_IMM_USE_STMT (use, 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), + bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE); + tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size), &nargs); gcall *call = gimple_build_call (fun, 1, @@ -3160,7 +3162,10 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, else { gimple_stmt_iterator gsi = gsi_for_stmt (use); - gsi_insert_before (&gsi, call, GSI_NEW_STMT); + if (store_p) + gsi_replace (&gsi, call, true); + else + gsi_insert_before (&gsi, call, GSI_NEW_STMT); } } diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c index 71be382ab8b..a4a2995f58b 100644 --- a/gcc/internal-fn.c +++ b/gcc/internal-fn.c @@ -388,6 +388,14 @@ expand_ASAN_POISON (internal_fn, gcall *) gcc_unreachable (); } +/* This should get expanded in the sanopt pass. */ + +static void +expand_ASAN_POISON_USE (internal_fn, gcall *) +{ + gcc_unreachable (); +} + /* This should get expanded in the tsan pass. */ static void diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 7b28b6722ff..fd25a952299 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -168,6 +168,7 @@ 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 (ASAN_POISON_USE, 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/testsuite/gcc.dg/asan/use-after-scope-10.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c new file mode 100644 index 00000000000..24de8cec1ff --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c @@ -0,0 +1,22 @@ +// { dg-do run } +// { dg-shouldfail "asan" } +// { dg-additional-options "-O2 -fdump-tree-asan1" } + +int +main (int argc, char **argv) +{ + int *ptr = 0; + + { + int a; + ptr = &a; + *ptr = 12345; + } + + *ptr = 12345; + return *ptr; +} + +// { dg-output "ERROR: AddressSanitizer: stack-use-after-scope on address.*(\n|\r\n|\r)" } +// { dg-output "WRITE of size .*" } +// { dg-output ".*'a' <== Memory access at offset \[0-9\]* is inside this variable.*" } diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c index c7df237d57f..22261c15dc2 100644 --- a/gcc/tree-into-ssa.c +++ b/gcc/tree-into-ssa.c @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-ssa.h" #include "domwalk.h" #include "statistics.h" +#include "asan.h" #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y)) @@ -1807,6 +1808,26 @@ maybe_replace_use_in_debug_stmt (use_operand_p use_p) } +/* If DEF has x_5 = ASAN_POISON () as its current def, add + ASAN_POISON_USE (x_5) stmt before GSI to denote the stmt writes into + a poisoned (out of scope) variable. */ + +static void +maybe_add_asan_poison_write (tree def, gimple_stmt_iterator *gsi) +{ + tree cdef = get_current_def (def); + if (cdef != NULL + && TREE_CODE (cdef) == SSA_NAME + && gimple_call_internal_p (SSA_NAME_DEF_STMT (cdef), IFN_ASAN_POISON)) + { + gcall *call + = gimple_build_call_internal (IFN_ASAN_POISON_USE, 1, cdef); + gimple_set_location (call, gimple_location (gsi_stmt (*gsi))); + gsi_insert_before (gsi, call, GSI_SAME_STMT); + } +} + + /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES or OLD_SSA_NAMES, or if it is a symbol marked for renaming, register it as the current definition for the names replaced by @@ -1837,7 +1858,11 @@ maybe_register_def (def_operand_p def_p, gimple *stmt, def = get_or_create_ssa_default_def (cfun, sym); } else - def = make_ssa_name (def, stmt); + { + if (asan_sanitize_use_after_scope ()) + maybe_add_asan_poison_write (def, &gsi); + def = make_ssa_name (def, stmt); + } SET_DEF (def_p, def); tree tracked_var = target_for_debug_bind (sym); diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c index 4e51e699d49..7fd2478adec 100644 --- a/gcc/tree-ssa-dce.c +++ b/gcc/tree-ssa-dce.c @@ -1384,6 +1384,10 @@ eliminate_unnecessary_stmts (void) case IFN_MUL_OVERFLOW: maybe_optimize_arith_overflow (&gsi, MULT_EXPR); break; + case IFN_ASAN_POISON: + if (!gimple_has_lhs (stmt)) + remove_dead_stmt (&gsi, bb); + break; default: break; } -- 2.11.0