On Wed, Jan 27, 2021 at 1:29 PM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jan 26, 2021, Richard Biener <richard.guent...@gmail.com> wrote:
>
> > 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):
>
> *nod*, it's just not the *only* place.
>
> > 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?
>
> There are pieces of nested function lowering for which we set cfun and
> current_function_decl while walking each function, and there are other
> pieces that just don't bother, and we only set up current_function_decl
> temporarily for ADDR_EXPR handling.
>
> This patch adjusts both of the ADDR_EXPR handlers that override
> current_function_decl, so that the temporary overriding remains in
> effect during the re-gimplification. That is enough to avoid the
> problem. But I'm not very happy with this temporary overriding, it
> seems fishy. I'd rather we set things up for the entire duration of the
> walking of each function.
>
> But that's only relevant because we rely on current_function_decl for
> address handling. It's not clear to me that we should, as the other
> patch demonstrated. With it, we could probably even do away with these
> overriders.
True, but I guess at least documentation of the predicates need to be
more precise. I've also considered making the current_function_decl
accesses function parameters instead. We do have
decl_address_ip_invariant_p on which decl_address_invariant_p
could build on by simply doing
decl_address_ip_invariant_p (op) || auto_var_p (op)
(if there were not the strange STRING_CST handling in the _ip_ variant)
> But, for this stage, this is probably as conservative a change as we
> could possibly hope for. I've regstrapped it on x86_64-linux-gnu, and
> also bootstrapped it with asan and ubsan. Ok to install?
Yes, OK.
Thanks,
Richard.
>
> restore current_function_decl after re-gimplifying nested ADDR_EXPRs
>
> 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.
>
> decl_address_invariant_p, called when determining whether an
> expression is a legitimate gimple value, compares the context of
> automatic variables with current_function_decl. Some of the
> tree-nested function processing doesn't set current_function_decl, but
> ADDR_EXPR-processing bits temporarily override it. However, they
> restore it before re-gimplifying, which causes even ADDR_EXPRs
> referencing automatic variables in the FRAME struct of a nested
> function to not be regarded as address-invariant.
>
> This patch moves the restores of current_function_decl in the
> ADDR_EXPR-handling bits after the re-gimplification, so that the
> correct current_function_decl is used when testing for address
> invariance.
>
>
> for gcc/ChangeLog
>
> * tree-nested.c (convert_nonlocal_reference_op): Move
> current_function_decl restore after re-gimplification.
> (convert_local_reference_op): Likewise.
>
> for gcc/testsuite/ChangeLog
>
> * gcc.dg/asan/nested-1.c: New.
> ---
> gcc/testsuite/gcc.dg/asan/nested-1.c | 24 ++++++++++++++++++++++++
> gcc/tree-nested.c | 4 ++--
> 2 files changed, 26 insertions(+), 2 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-nested.c b/gcc/tree-nested.c
> index 1b52669b622aa..addd6eef9aba6 100644
> --- a/gcc/tree-nested.c
> +++ b/gcc/tree-nested.c
> @@ -1214,7 +1214,6 @@ convert_nonlocal_reference_op (tree *tp, int
> *walk_subtrees, void *data)
> 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),
> @@ -1222,6 +1221,7 @@ convert_nonlocal_reference_op (tree *tp, int
> *walk_subtrees, void *data)
> if (save_val_only)
> *tp = gsi_gimplify_val ((struct nesting_info *) wi->info,
> t, &wi->gsi);
> + current_function_decl = save_context;
> }
> }
> break;
> @@ -1969,13 +1969,13 @@ convert_local_reference_op (tree *tp, int
> *walk_subtrees, void *data)
> save_context = current_function_decl;
> current_function_decl = info->context;
> recompute_tree_invariant_for_addr_expr (t);
> - current_function_decl = save_context;
>
> /* If we are in a context where we only accept values, then
> compute the address into a temporary. */
> if (save_val_only)
> *tp = gsi_gimplify_val ((struct nesting_info *) wi->info,
> t, &wi->gsi);
> + current_function_decl = save_context;
> }
> break;
>
>
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> Vim, Vi, Voltei pro Emacs -- GNUlius Caesar
