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
