On Thu, Jan 21, 2021 at 10:36 PM Alexandre Oliva <ol...@adacore.com> wrote:
>
>
> Ada makes extensive use of nested functions, which turn all automatic
> variables of the enclosing function that are used in nested ones into
> members of an artificial FRAME record type.
>
> The address of a local variable is usually passed to asan marking
> functions without using a temporary. Taking the address of a member
> of FRAME within a nested function, however, is not regarded as a
> gimple val: while introducing FRAME variables, current_function_decl
> is always the outermost function, even while processing a nested
> function, so decl_address_invariant_p returns false for such
> ADDR_EXPRs. So, as automatic variables are moved into FRAME, any asan
> call that marks such a variable has its ADDR_EXPR replaced with a
> SSA_NAME set to the ADDR_EXPR of the FRAME member.
>
> asan_expand_mark_ifn was not prepared to deal with ADDR_EXPRs split
> out into SSA_NAMEs. This patch deals with such cases.
>
> [It does NOT deal with PHI nodes and whatnot. I'm not even sure it
> should. Maybe we want the ADDR_EXPR to be a gimple val instead, but
> this more conservative fix felt more appropriate for this stage.]
Yeah, I guess such addresses could be decl_address_invariant_p by changing
the
|| DECL_CONTEXT (op) == current_function_decl
|| decl_function_context (op) == current_function_decl)
to sth like
|| auto_var_p (op)
but I guess it won't help much since the access in the nested function
will be through the static chain pointer and thus &chain->member
which definitely isn't a gimple val. When it still looks like &<FRAME>.member
it could be but that arises more re-gimplification needs during the
nested function lowering then.
>
> Regstrapped on x86_64-linux-gnu. Ok to install?
few comments below
>
> for gcc/ChangeLog
>
> * asan.c (asan_expand_mark_ifn): Follow SSA_NAME defs for
> an ADDR_EXPR base.
>
> for gcc/testsuite/ChangeLog
>
> * gcc.dg/asan/nested-1.c: New.
> ---
> gcc/asan.c | 21 +++++++++++++++++++++
> gcc/testsuite/gcc.dg/asan/nested-1.c | 24 ++++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
> create mode 100644 gcc/testsuite/gcc.dg/asan/nested-1.c
>
> diff --git a/gcc/asan.c b/gcc/asan.c
> index 89ecd99b18294..2d2fb97098b2f 100644
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -3629,6 +3629,27 @@ asan_expand_mark_ifn (gimple_stmt_iterator *iter)
> bool is_poison = ((asan_mark_flags)flag) == ASAN_MARK_POISON;
>
> tree base = gimple_call_arg (g, 1);
> + while (TREE_CODE (base) == SSA_NAME)
> + {
> + gimple *def = SSA_NAME_DEF_STMT (base);
> + if (!def)
> + break;
> +
> + if (!is_gimple_assign (def))
> + break;
> +
> + if (!SINGLE_SSA_TREE_OPERAND (def, SSA_OP_DEF))
> + break;
redundant
> + if (gimple_num_ops (def) != 2)
> + break;
likewise
> + if (gimple_expr_code (def) == ADDR_EXPR
> + || gimple_expr_code (def) == SSA_NAME)
Use gimple_assign_rhs_code (def), not gimple_expr_code.
> + base = gimple_assign_rhs1 (def);
> + else
> + break;
> + }
note with the above the assert below will become dangerous
if you think of PRE/CSE or other things we could do to the
split out address.
> gcc_checking_assert (TREE_CODE (base) == ADDR_EXPR);
> tree decl = TREE_OPERAND (base, 0);
>
> diff --git a/gcc/testsuite/gcc.dg/asan/nested-1.c
> b/gcc/testsuite/gcc.dg/asan/nested-1.c
> new file mode 100644
> index 0000000000000..87e842098077c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/asan/nested-1.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fsanitize=address" } */
> +
> +int f(int i) {
> + auto int h() {
> + int r;
> + int *p;
> +
> + {
> + int x[3];
> +
> + auto int g() {
> + return x[i];
> + }
> +
> + p = &r;
> + *p = g();
> + }
> +
> + return *p;
> + }
> +
> + return h();
> +}
>
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> Vim, Vi, Voltei pro Emacs -- GNUlius Caesar
