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