From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 110320 invoked by alias); 20 Jun 2017 09:23:43 -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 110114 invoked by uid 89); 20 Jun 2017 09:23:40 -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= X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 20 Jun 2017 09:23:38 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 66E84AAB6; Tue, 20 Jun 2017 09:23:36 +0000 (UTC) Subject: Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040). To: Jakub Jelinek Cc: GCC Patches References: <20170619141340.GP2123@tucnak> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: Date: Tue, 20 Jun 2017 09:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170619141340.GP2123@tucnak> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg01413.txt.bz2 On 06/19/2017 04:13 PM, Jakub Jelinek wrote: > On Mon, Jun 19, 2017 at 03:50:42PM +0200, Martin Liška wrote: >> @@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void) >> } >> } >> > > Missing function comment. > >> +static tree >> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data) >> +{ >> + struct walk_stmt_info *wi = (struct walk_stmt_info *) data; >> + std::pair *replacement = (std::pair *)wi->info; > > Missing space after ) > >> + >> + if (*op == replacement->first) >> + { >> + *op = replacement->second; >> + *walk_subtrees = 0; >> + } >> + >> + return NULL; >> +} > >> +static void >> +sanitize_rewrite_addressable_params (function *fun) >> +{ >> + basic_block entry_bb = NULL; >> + >> + for (tree arg = DECL_ARGUMENTS (current_function_decl); >> + arg; arg = DECL_CHAIN (arg)) >> + { >> + if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg))) >> + { >> + /* The parameter is no longer addressable. */ >> + tree type = TREE_TYPE (arg); >> + TREE_ADDRESSABLE (arg) = 0; >> + >> + /* Create a new automatic variable. */ >> + tree var = build_decl (DECL_SOURCE_LOCATION (arg), >> + VAR_DECL, DECL_NAME (arg), type); >> + TREE_ADDRESSABLE (var) = 1; >> + DECL_ARTIFICIAL (var) = 1; >> + DECL_SEEN_IN_BIND_EXPR_P (var) = 0; > > I think it is highly inefficient to walk the whole IL for every addressable > argument. Can't you first find out what PARM_DECLs you need to change, > stick the corresponding VAR_DECL somewhere (dunno, e.g. a vector with pairs > perhaps sorted by DECL_UID, or stick it into DECL_VALUE_EXPR or whatever), > then if you create at least one, walk whole IL and replace all the > PARM_DECLs you want to replace, then finally clear the TREE_ADDRESSABLE > flag for all of them and emit the initialization sequence? Yes, this is doable. I've done that. > Then something needs to be done for debugging too. If it is without VTA, > then probably just having DECL_VALUE_EXPR is good enough, otherwise > (VTA) you probably don't want that (or can reset it at that point), but > instead emit after the initialization stmt a debug stmt that the variable > value now lives in a different var. Though ideally we want the debugger > to be able to also change the value of the var, that might be harder. > With DECL_VALUE_EXPR on the other side the debug info will be incorrect in > the prologue until it is assigned to the slot. Here I'm not sure about how to distinguish whether to build or not to build the debug statement. According to flag_var_tracking? You mean something like: g = gimple_build_debug_bind (arg, var, g); ? > >> + >> + gimple_add_tmp_var (var); >> + >> + if (dump_file) >> + fprintf (dump_file, >> + "Rewritting parameter whos address is taken: %s\n", >> + IDENTIFIER_POINTER (DECL_NAME (arg))); > > s/tting/ting/, s/whos/whose/ >> + >> + gimple_seq stmts = NULL; >> + >> + /* Assign value of parameter to newly created variable. */ >> + if ((TREE_CODE (type) == COMPLEX_TYPE >> + || TREE_CODE (type) == VECTOR_TYPE)) >> + { >> + /* We need to create a SSA name that will be used for the >> + assignment. */ >> + tree tmp = make_ssa_name (type); >> + gimple *g = gimple_build_assign (tmp, arg); >> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg)); >> + gimple_seq_add_stmt (&stmts, g); >> + g = gimple_build_assign (var, tmp); >> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg)); >> + gimple_seq_add_stmt (&stmts, g); >> + } >> + else >> + { >> + gimple *g = gimple_build_assign (var, arg); >> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg)); >> + gimple_seq_add_stmt (&stmts, g); >> + } > > I don't understand the distinction. If you turn the original parm > for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code > (but I think it would be better to use the default SSA_NAME of the PARM_DECL > if it is a gimple reg type, rather than use the PARM_DECL itself > and wait for update_ssa). Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me as one needs to have a temporary SSA name, otherwise: /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: error: invalid rhs for gimple memory store foo (v4si arg) ^~~ arg arg # .MEM_4 = VDEF <.MEM_1(D)> arg = arg; during GIMPLE pass: sanopt If I see correctly the function in my test-case does not have default def SSA name for the parameter. Thus I guess I need to create a SSA name? Thanks, Martin > > Jakub >