From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119390 invoked by alias); 30 Jun 2017 16:37:19 -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 118792 invoked by uid 89); 30 Jun 2017 16:37:18 -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,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=Furthermore, H*RU:sk:2017063, Hx-spam-relays-external:sk:2017063, 23PM X-HELO: mailout2.w1.samsung.com Received: from mailout2.w1.samsung.com (HELO mailout2.w1.samsung.com) (210.118.77.12) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 30 Jun 2017 16:37:15 +0000 Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OSD002AECTZRY70@mailout2.w1.samsung.com> for gcc-patches@gcc.gnu.org; Fri, 30 Jun 2017 17:37:11 +0100 (BST) Received: from eusmges2.samsung.com (unknown [203.254.199.241]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20170630163710eucas1p1e5dae34bc77175f70789931409d0e01d~M85y_Yys63185831858eucas1p1e; Fri, 30 Jun 2017 16:37:10 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2.samsung.com (EUCPMTA) with SMTP id 19.1E.04459.63E76595; Fri, 30 Jun 2017 17:37:10 +0100 (BST) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20170630163709eucas1p15a9aebfd433d291363448d377065fbfe~M85yWap2k3165831658eucas1p1U; Fri, 30 Jun 2017 16:37:09 +0000 (GMT) Received: from eusync3.samsung.com ( [203.254.199.213]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 4E.E0.20206.53E76595; Fri, 30 Jun 2017 17:37:09 +0100 (BST) Received: from [106.109.129.18] by eusync3.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0OSD009XLCTXTO00@eusync3.samsung.com>; Fri, 30 Jun 2017 17:37:09 +0100 (BST) Subject: =?UTF-8?Q?Re:_[PATCH_v2][ASAN]_Implement_dynamic_allocas/VLAs_sanit?= =?UTF-8?B?aXphdGlvbi7igIs=?= To: Jakub Jelinek Cc: GCC Patches , Richard Biener , Yuri Gribov From: Maxim Ostapenko Message-id: <59567E34.1010001@samsung.com> Date: Fri, 30 Jun 2017 16:37:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 In-reply-to: <20170629123527.GC2123@tucnak> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170630163709eucas1p15a9aebfd433d291363448d377065fbfe X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?TWFrc2ltIE9zdGFwZW5rbxtTUlItU1cgVG9vbHMgTGFiGw==?= =?UTF-8?B?7IK87ISx7KCE7J6QG0VuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?TWF4aW0gT3N0YXBlbmtvG1NSUi1TVyBUb29scyBMYWIbU2Ft?= =?UTF-8?B?c3VuZ8KgRWxlY3Ryb25pY3MbRW5naW5lZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG0NJU0hRG0MxMEdEMDFHRDAxMDE1Nw==?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170626124925eucas1p18c56742a07db5bb2dabbedd0e894aa0e X-RootMTR: 20170626124925eucas1p18c56742a07db5bb2dabbedd0e894aa0e References: <595102D3.3070603@samsung.com> <20170629123527.GC2123@tucnak> X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg02413.txt.bz2 Hi, On 29/06/17 15:35, Jakub Jelinek wrote: > 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. OK, good point, will remove from this patch. > >> @@ -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. Err, thanks, will fix it. > >> >> /* 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. Right, will fix it. >> + >> + 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. Right, 'bottom' should be replaced by 'bot'. Regarding to virtual_dynamic_stactk_rtx - as you correctly noted below, second parameter of __asan_allocas_unpoison will be completely rewritten in expand_builtin_alloca function by virtual_dynamic_stactk_rtx. Here I've just passed new_sp as a dummy argument. This looks hacky, but the problem is that for several architectures (e.g. PPC) we cannot use new_sp as a 'bot' parameter because new_sp != last_alloca_addr on these targets. Originally, I tried to do something like this: top = last_alloca_addr; bot = new_sp + STACK_DYNAMIC_OFFSET; __asan_allocas_unpoison(top, bot); last_alloca_addr = bot; but I was confused by the fact that STACK_DYNAMIC_OFFSET becomes available only after expansion to RTL. Rewriting 'bot' with virtual_dynamic_stactk_rtx in RTL looks like the most feasible way how to overcome this issue for me. > >> + 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. Thanks, will fix. > >> + /* 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. Yeah, I'll add a comment about motivation. > >> + 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 > > > -Maxim