| Message ID | or35yuhtb5.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | skip asan-poisoning of discarded vars | expand |
On Thu, Jan 21, 2021 at 06:30:06PM -0300, Alexandre Oliva wrote: > > GNAT may create temporaries to hold return values of function calls. > If such a temporary is created as part of a dynamic initializer of a > variable in a unit other than the one being compiled, the initializer > is dropped, including the temporary and its binding block. > > Don't issue asan mark calls for such variables, they are gone. > > Regstrapped on x86_64-linux-gnu. Ok to install? > > > for gcc/ChangeLog > > * gimplify.c (gimplify_decl_expr): Skip asan marking calls for > temporaries not seen in binding block, and not about to be > added as gimple variables. > > for gcc/testsuite/ChangeLog > > * gnat.dg/asan1.adb: New test. > * gnat.dg/asan1_pkg.ads: New additional source. Does that affect only Ada and not other languages? Could you e.g. try --with-build-config bootstrap-asan and log which TUs/functions it didn't posion because of this patch? I usually use hacks like: { FILE *f = fopen ("/tmp/whatever", "a"); fprintf (f, "%d %s %s\n", (int) BITS_PER_WORD, main_input_filename ? main_input_filename : "-", current_function_name ()); fclose (f); } Jakub
On Jan 21, 2021, Jakub Jelinek <jakub@redhat.com> wrote:
> Does that affect only Ada and not other languages?
I haven't observed it on other languages, but I didn't try really hard.
Doing that now, with an assert that the newly-added condition doesn't
ever hit. I'd already completed a bootstrap-asan the other day, but not
with all languages.
The kind of variable that triggers the problem is created within
gcc/ada/gcc-interface/trans.c:Call_to_gnu, specifically within
create_temporary in the same file. In the provided testcase, it injects
the temporary created for the return value from N, passed as a parameter
to C. The binding block containing that temporary ends up dropped when
the initializer of V is discarded, because it's dynamic and V is
imported from a different unit.
I figured if the added condition were to ever hit before, we would add a
poison call to a function that did not have gimple_add_tmp_var called on
it, and that would NOT have it called in the block right after the one I
proposed to modify:
if (!DECL_SEEN_IN_BIND_EXPR_P (decl)
&& DECL_ARTIFICIAL (decl) && DECL_NAME (decl) == NULL_TREE)
gimple_add_tmp_var (decl);
Without gimple_add_tmp_var, we wouldn't allocate automatic storage to
the variable in expand, and then the attempt to take its address for the
poisoning call would explode in make_decl_rtl like this testcase does,
because make_decl_rtl is not to be called for automatic variables.
Since this didn't happen, I figured the new condition would not be hit
except for the failing case. But I was wrong. The bootstrap with the
added assert has just failed, as early as stage2 libiberty. Looking
into it...
On Jan 21, 2021, Alexandre Oliva <oliva@adacore.com> wrote: > But I was wrong. The bootstrap with the added assert has just failed, > as early as stage2 libiberty. Looking into it... Uhh, I take that back. I just goofed in the assert, inverting the condition. Long day... With the correct condition, it's got past the stage2 compilation of all of the gcc deps and Ada sources, and then some. Maybe my reasoning wasn't wrong, after all ;-)
On Jan 21, 2021, Alexandre Oliva <oliva@adacore.com> wrote: > On Jan 21, 2021, Alexandre Oliva <oliva@adacore.com> wrote: >> But I was wrong. The bootstrap with the added assert has just failed, >> as early as stage2 libiberty. Looking into it... > Uhh, I take that back. I just goofed in the assert, inverting the > condition. Long day... > With the correct condition, it's got past the stage2 compilation of all > of the gcc deps and Ada sources, and then some. Maybe my reasoning > wasn't wrong, after all ;-) Yeah, confirmed, bootstrap-asan (and -ubsan) completed on x86_64-linux-gnu, all languages, with the following patchlet (cut&pasted, then retabified manually, so it may not apply mechanically): diff --git a/gcc/gimplify.c b/gcc/gimplify.c index d2ac5f9..c0dcb39 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -1797,6 +1797,9 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p) && dbg_cnt (asan_use_after_scope) && !gimplify_omp_ctxp) { + gcc_assert (DECL_SEEN_IN_BIND_EXPR_P (decl) + || (DECL_ARTIFICIAL (decl) + && DECL_NAME (decl) == NULL_TREE)); asan_poisoned_variables->add (decl); asan_poison_variable (decl, false, seq_p); if (!DECL_ARTIFICIAL (decl) && gimplify_ctxp->live_switch_vars)
On Thu, Jan 21, 2021 at 07:42:43PM -0300, Alexandre Oliva wrote: > On Jan 21, 2021, Alexandre Oliva <oliva@adacore.com> wrote: > > > But I was wrong. The bootstrap with the added assert has just failed, > > as early as stage2 libiberty. Looking into it... > > Uhh, I take that back. I just goofed in the assert, inverting the > condition. Long day... > > With the correct condition, it's got past the stage2 compilation of all > of the gcc deps and Ada sources, and then some. Maybe my reasoning > wasn't wrong, after all ;-) The patch is ok for trunk then. Jakub
diff --git a/gcc/gimplify.c b/gcc/gimplify.c index d2ac5f913593f..95d55bb8ba4c7 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -1795,7 +1795,13 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p) && !DECL_HAS_VALUE_EXPR_P (decl) && DECL_ALIGN (decl) <= MAX_SUPPORTED_STACK_ALIGNMENT && dbg_cnt (asan_use_after_scope) - && !gimplify_omp_ctxp) + && !gimplify_omp_ctxp + /* GNAT introduces temporaries to hold return values of calls in + initializers of variables defined in other units, so the + declaration of the variable is discarded completely. We do not + want to issue poison calls for such dropped variables. */ + && (DECL_SEEN_IN_BIND_EXPR_P (decl) + || (DECL_ARTIFICIAL (decl) && DECL_NAME (decl) == NULL_TREE))) { asan_poisoned_variables->add (decl); asan_poison_variable (decl, false, seq_p); diff --git a/gcc/testsuite/gnat.dg/asan1.adb b/gcc/testsuite/gnat.dg/asan1.adb new file mode 100644 index 0000000000000..a4bc59a9a2143 --- /dev/null +++ b/gcc/testsuite/gnat.dg/asan1.adb @@ -0,0 +1,15 @@ +-- { dg-do compile } +-- { dg-additional-sources asan1_pkg.ads } +-- { dg-options "-fsanitize=address" } +-- { dg-skip-if "" no_fsanitize_address } + +with Asan1_Pkg; + +procedure Asan1 is + use Asan1_Pkg; + + X, Y : E; +begin + X := C (N); + Y := V; +end Asan1; diff --git a/gcc/testsuite/gnat.dg/asan1_pkg.ads b/gcc/testsuite/gnat.dg/asan1_pkg.ads new file mode 100644 index 0000000000000..fbbc1c5e7f5bd --- /dev/null +++ b/gcc/testsuite/gnat.dg/asan1_pkg.ads @@ -0,0 +1,9 @@ +package Asan1_Pkg is + subtype E is Integer; + type T is array (1..32) of E; + + function N return T; + function C (P : T) return E; + + V : constant E := C (N); +end Asan1_Pkg;