On Tue, Jan 26, 2021 at 10:52 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jan 22, 2021, Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> 
> wrote:
>
> > 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.
>
> I think we only get poison/unpoison calls in the scope of the automatic
> variables, so it won't be the case that these calls will get such
> indirect frame references: they will only get references to the own
> function's frame, and those have invariant addresses, and thus can be
> regarded as such, and as gimple vals.
>
> So I gave this alternate change a spin, and both regstrap and asan+ubsan
> bootstrap completed successfully.
>
> Given the considerations of risk about the assert you pointed out, I'm
> now inclined to regard this change as safer and superior.  Do you all
> concur?  Ok to install if so?
>
> (I see the code in tree.c was untabified, and one of my changes
> introduced a tab that misaligned stuff.  Tabify, untabify, or leave it
> inconsistent as in the tested patch below?
>
>
> regard the address of auto vars and consts as invariant
>
> From: Alexandre Oliva <ol...@adacore.com>
>
> 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.  asan_expand_mark_ifn will reject
> an ADDR_EXPRs if it's split out from the call into an SSA_NAMEs.
>
> Taking the address of a member of FRAME within a nested function was
> not regarded as a gimple val: while introducing FRAME variables,
> current_function_decl pointed to the outermost function, even while
> processing a nested function, so decl_address_invariant_p, checking
> that the context of the variable is current_function_decl, returned
> false for such ADDR_EXPRs.
>
> This patch changes decl_address_invariant_p to disregard
> current_function_decl, and regard all automatic variables as having
> invariant addresses, regardless of nesting.  This may initially
> include references to variables in other nesting levels, but once they
> become references to enclosing frames, the indirection makes them
> non-gimple_vals.  As long as poisoning and unpoisoning calls doesn't
> kick in for variables in other frames, this shouldn't be a problem.

So while I think it's safe let's look at if we can improve tree-nested.c,
like I see (probably not the correct place):

static tree
convert_nonlocal_reference_op (tree *tp, int *walk_subtrees, void *data)
{
...
            /* If we changed anything, we might no longer be directly
               referencing a decl.  */
            save_context = current_function_decl;
            current_function_decl = info->context;
            recompute_tree_invariant_for_addr_expr (t);
            current_function_decl = save_context;

            /* If the callback converted the address argument in a context
               where we only accept variables (and min_invariant, presumably),
               then compute the address into a temporary.  */
            if (save_val_only)
              *tp = gsi_gimplify_val ((struct nesting_info *) wi->info,
                                      t, &wi->gsi);

seeing how we adjust current_function_decl around the
recompute_tree_invariant_for_addr_expr call but not the
gsi_gimplify_val one (we already pass it a nesting_info,
not sure if wi->info is the same as the 'info' used above though),
so eventually we can fix it in one place?

I see the testcase ICEs only at -O0 and with -O we CCP the
address so we definitely treat it as invariant just not in the
skewed context tree-nested operates in.

Richard.

>
> for  gcc/ChangeLog
>
>         * tree.c (decl_address_invariant_p): Accept auto variables and
>         constants.
>
> for  gcc/testsuite/ChangeLog
>
>         * gcc.dg/asan/nested-1.c: New.
> ---
>  gcc/testsuite/gcc.dg/asan/nested-1.c |   24 ++++++++++++++++++++++++
>  gcc/tree.c                           |    5 ++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/asan/nested-1.c
>
> 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();
> +}
> diff --git a/gcc/tree.c b/gcc/tree.c
> index 287e5001dc3b3..3de3085f42c8a 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -3590,14 +3590,13 @@ decl_address_invariant_p (const_tree op)
>      case VAR_DECL:
>        if ((TREE_STATIC (op) || DECL_EXTERNAL (op))
>            || DECL_THREAD_LOCAL_P (op)
> -          || DECL_CONTEXT (op) == current_function_decl
> -          || decl_function_context (op) == current_function_decl)
> +         || auto_var_p (op))
>          return true;
>        break;
>
>      case CONST_DECL:
>        if ((TREE_STATIC (op) || DECL_EXTERNAL (op))
> -          || decl_function_context (op) == current_function_decl)
> +          || auto_var_p (op))
>          return true;
>        break;
>
>
>
> --
> Alexandre Oliva, happy hacker  https://FSFLA.org/blogs/lxo/
>    Free Software Activist         GNU Toolchain Engineer
>         Vim, Vi, Voltei pro Emacs -- GNUlius Caesar

Reply via email to