From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61661 invoked by alias); 29 Jun 2017 12:35:37 -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 61650 invoked by uid 89); 29 Jun 2017 12:35:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=23PM, 23pm 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; Thu, 29 Jun 2017 12:35:35 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D47F6128281; Thu, 29 Jun 2017 12:35:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D47F6128281 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jakub@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D47F6128281 Received: from tucnak.zalov.cz (ovpn-116-143.ams2.redhat.com [10.36.116.143]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 797D189CAA; Thu, 29 Jun 2017 12:35:33 +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 v5TCZUd1009765; Thu, 29 Jun 2017 14:35:30 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id v5TCZR0M009764; Thu, 29 Jun 2017 14:35:27 +0200 Date: Thu, 29 Jun 2017 12:35:00 -0000 From: Jakub Jelinek To: Maxim Ostapenko Cc: GCC Patches , Richard Biener , Yuri Gribov Subject: Re: [PATCH v2][ASAN] Implement =?utf-8?Q?d?= =?utf-8?Q?ynamic_allocas=2FVLAs_sanitization=2E=E2=80=8B?= Message-ID: <20170629123527.GC2123@tucnak> Reply-To: Jakub Jelinek References: <595102D3.3070603@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <595102D3.3070603@samsung.com> User-Agent: Mutt/1.7.1 (2016-10-04) X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg02263.txt.bz2 Hi! Sorry for the review delay. On Mon, Jun 26, 2017 at 03:49:23PM +0300, Maxim Ostapenko wrote: > (handle_builtin_stackrestore): Likewise. The function is called with _ between stack and restore. > * match.pd: Add new pattern. Unless the patch relies on this, I think it should be posted separately and reviewed by Richard. > @@ -245,6 +246,7 @@ along with GCC; see the file COPYING3. If not see > static unsigned HOST_WIDE_INT asan_shadow_offset_value; > static bool asan_shadow_offset_computed; > static vec sanitized_sections; > +static tree last_alloca_addr = NULL_TREE; You are shadowing this variable in multiple places. Either rename it to something different, or rename the results of get_last_alloca_addr. And the " = NULL_TREE" part is not needed. > > /* Set of variable declarations that are going to be guarded by > use-after-scope sanitizer. */ > @@ -529,11 +531,183 @@ get_mem_ref_of_assignment (const gassign *assignment, > return true; > } > > +/* Return address of last allocated dynamic alloca. */ > + > +static tree > +get_last_alloca_addr () > +{ > + if (last_alloca_addr) > + return last_alloca_addr; > + > + gimple_seq seq = NULL; > + gassign *g; > + > + last_alloca_addr = create_tmp_reg (ptr_type_node, "last_alloca_addr"); > + g = gimple_build_assign (last_alloca_addr, NOP_EXPR, > + build_int_cst (ptr_type_node, 0)); Instead of build_int_cst (ptr_type_node, 0) you should use null_pointer_node. And the NOP_EXPR there is just wrong, either it should be gimple_build_assign (last_alloca_addr, null_pointer_node); or gimple_build_assign (last_alloca_addr, INTEGER_CST, null_pointer_node); > + gimple_seq_add_stmt_without_update (&seq, g); Why the seq stuff at all? You have a single stmt you want to insert on edge. > + > + edge e = single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun)); > + gsi_insert_seq_on_edge_immediate (e, seq); So just use here gsi_insert_on_edge_immediate (e, g); instead. > + return last_alloca_addr; > +} > + > +/* Insert __asan_allocas_unpoison (top, bottom) call after > + __builtin_stack_restore (new_sp) call. > + The pseudocode of this routine should look like this: > + __builtin_stack_restore (new_sp); > + top = last_alloca_addr; > + bot = virtual_dynamic_stack_rtx; > + __asan_allocas_unpoison (top, bottom); > + last_alloca_addr = new_sp; The comment doesn't seem to agree with what you actually implement. There is no virtual_dynamic_stack_rtx during the asan pass, it is there only during expansion until the virtual regs are instantiated in the next pass. Furthermore, you have bot variable, but then use bottom. > + tree last_alloca_addr = get_last_alloca_addr (); Here is the shadowing I talked about. > + tree restored_stack = gimple_call_arg (call, 0); > + tree fn = builtin_decl_implicit (BUILT_IN_ASAN_ALLOCAS_UNPOISON); > + gimple *g = gimple_build_call (fn, 2, last_alloca_addr, restored_stack); Here you clearly use the first argument of __builtin_stack_restore, which is that new_sp. > + gimple_seq_add_stmt_without_update (&seq, g); Why the messing up with sequences? Just insert the stmt immediately in, and the others as well. > + g = gimple_build_assign (last_alloca_addr, NOP_EXPR, restored_stack); This is again wrong, here you really don't know what restored_stack is, it could be SSA_NAME, but also something different, so you should use gimple_build_assign (last_alloca_addr, restored_stack); and let it figure out the rhs code. > + /* Extract lower bits from old_size. */ > + wide_int size_nonzero_bits = get_nonzero_bits (old_size); > + wide_int rz_mask > + = wi::uhwi (redzone_mask, wi::get_precision (size_nonzero_bits)); > + wide_int old_size_lower_bits = wi::bit_and (size_nonzero_bits, rz_mask); > + > + /* If alloca size is aligned to ASAN_RED_ZONE_SIZE, we don't need partial > + redzone. Otherwise, compute its size here. */ > + if (wi::ne_p (old_size_lower_bits, 0)) > + { > + /* misalign = size & (ASAN_RED_ZONE_SIZE - 1) > + partial_size = ASAN_RED_ZONE_SIZE - misalign. */ > + g = gimple_build_assign (make_ssa_name (size_type_node, NULL), > + BIT_AND_EXPR, old_size, alloca_rz_mask); > + gimple_seq_add_stmt_without_update (&seq, g); > + tree misalign = gimple_assign_lhs (g); > + g = gimple_build_assign (make_ssa_name (size_type_node, NULL), MINUS_EXPR, > + redzone_size, misalign); > + gimple_seq_add_stmt_without_update (&seq, g); Again, why add the stmts into a seq first instead of just adding it immediately into the IL? > @@ -4962,6 +4962,20 @@ expand_builtin_alloca (tree exp) > return result; > } > Missing function comment here. > +static rtx > +expand_asan_emit_allocas_unpoison (tree exp) > +{ > + tree arg0 = CALL_EXPR_ARG (exp, 0); > + rtx top = expand_expr (arg0, NULL_RTX, GET_MODE (virtual_stack_dynamic_rtx), > + EXPAND_NORMAL); > + rtx ret = init_one_libfunc ("__asan_allocas_unpoison"); > + ret = emit_library_call_value (ret, NULL_RTX, LCT_NORMAL, ptr_mode, 2, top, > + TYPE_MODE (pointer_sized_int_node), > + virtual_stack_dynamic_rtx, > + TYPE_MODE (pointer_sized_int_node)); I see you are here pretty much ignoring the old second argument and instead using a different one. But then the above mentioned comment should explain that, how you transform it during the asan pass and how you later change it during expansion. > + case BUILT_IN_ASAN_ALLOCAS_UNPOISON: > + target = expand_asan_emit_allocas_unpoison (exp); > + if (target) > + return target; Do you need this test, when it always returns non-NULL? Just return expand_asan_emit_allocas_unpoison (exp); Jakub