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<char *> 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
