On Fri, Nov 3, 2017 at 6:00 PM, David Malcolm <dmalc...@redhat.com> wrote:
> PR jit/82826 reports a crash when running jit.dg/test-benchmark.c,
> introduced by r254140
> (aka "Extend ipa-pure-const pass to propagate malloc attribute.")
>
> I see the crash on the 28th of 400 in-process iterations of the
> compiler; on turning on GCC_JIT_BOOL_OPTION_SELFCHECK_GC, it shows up
> on the 2nd iteration.
>
> The root cause is that in one in-process invocation of the compiler we
> have mismatching calls to ipa_fn_summary_alloc/ipa_free_fn_summary.
>
> The sequence of calls is:
>
> 1st in-process iteration:
> (1): ipa_fn_summary_alloc
> (called indirectly by pass_local_fn_summary::execute)
>
> (2): ipa_free_fn_summary
> (called by pass_ipa_free_fn_summary::execute)
>
> (3): ipa_fn_summary_alloc
>
> ...where (3) is called thusly:
>
> (gdb) bt
> #0 ipa_fn_summary_alloc () at ../../src/gcc/ipa-fnsummary.c:533
> #1 0x00007ffff6616788 in ipa_fn_summary_generate () at
> ../../src/gcc/ipa-fnsummary.c:3184
> #2 0x00007ffff679ebe4 in execute_ipa_summary_passes (ipa_pass=0x656be0) at
> ../../src/gcc/passes.c:2200
> #3 0x00007ffff6359c2c in ipa_passes () at ../../src/gcc/cgraphunit.c:2448
> #4 0x00007ffff635a095 in symbol_table::compile (this=0x7fffef8ed100) at
> ../../src/gcc/cgraphunit.c:2558
> #5 0x00007ffff635a4e2 in symbol_table::finalize_compilation_unit
> (this=0x7fffef8ed100) at ../../src/gcc/cgraphunit.c:2716
> #6 0x00007ffff68ab25d in compile_file () at ../../src/gcc/toplev.c:481
> #7 0x00007ffff68ae3e1 in do_compile () at ../../src/gcc/toplev.c:2063
> #8 0x00007ffff68ae758 in toplev::main (this=0x7fffffffdf8e, argc=12,
> argv=0x61f6c8) at ../../src/gcc/toplev.c:2198
> (etc)
>
> and so we have an "alloc" that's not matched by a free.
>
> Hence the global "ipa_fn_summaries" is left non-NULL, a
> GTY-marked object, with a pointer to the GC-heap-allocated
> "symtab". There's no GTY marking for the symtab reference
> in the function_summary <T *>; it has GTY((user)), and the
> hand-written functions don't visit the symtab.
>
> On the next in-process iteration, the would-be alloc encounters this
> conditional:
>
> 2396 if (!ipa_fn_summaries)
> 2397 ipa_fn_summary_alloc ();
>
> and hence doesn't re-allocate.
>
> The global "ipa_fn_summaries" thus contains a pointer to last iteration's
> "symtab", leading to unpredictable bugs due to there being *two* symbol
> tables (one for the previous iteration, one for the current iteration).
> At some point the previous symtab will be collected from "under"
> ipa_fn_summaries, leading to a use-after-free crash e.g. here in
> function_summary<ipa_fn_summary*>::release:
> 67 m_symtab->remove_cgraph_insertion_hook (m_symtab_insertion_hook);
>
> due to m_symtab being GC-allocated but with nothing keeping it alive,
> and thus being (hopefully) poisoned by GC, or perhaps with the memory
> being reused for something else.
>
> This isn't an issue for the regular compiler, but when libgccjit
> reruns in-process, the mismatched alloc/cleanups can lead to use-after-free
> after the GC has run.
>
> This patch fixes the issue in the least invasive way I could, by explicitly
> ensuring that the cleanup happens in toplev::finalize (the jit's hook for
> doing such cleanups); merely fixing the GTY issue wouldn't fix the issue of
> accidentally having two symtabs.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> OK for trunk?
Ok.
Richard.
> gcc/ChangeLog:
> PR jit/82826
> * ipa-fnsummary.c (ipa_fnsummary_c_finalize): New function.
> * ipa-fnsummary.h (ipa_fnsummary_c_finalize): New decl.
> * toplev.c: Include "ipa-fnsummary.h".
> (toplev::finalize): Call ipa_fnsummary_c_finalize.
> ---
> gcc/ipa-fnsummary.c | 9 +++++++++
> gcc/ipa-fnsummary.h | 2 ++
> gcc/toplev.c | 2 ++
> 3 files changed, 13 insertions(+)
>
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index f71338e..20eec2a 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -3618,3 +3618,12 @@ make_pass_ipa_fn_summary (gcc::context *ctxt)
> {
> return new pass_ipa_fn_summary (ctxt);
> }
> +
> +/* Reset all state within ipa-fnsummary.c so that we can rerun the compiler
> + within the same process. For use by toplev::finalize. */
> +
> +void
> +ipa_fnsummary_c_finalize (void)
> +{
> + ipa_free_fn_summary ();
> +}
> diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h
> index a794bd0..b345bbc 100644
> --- a/gcc/ipa-fnsummary.h
> +++ b/gcc/ipa-fnsummary.h
> @@ -266,4 +266,6 @@ void estimate_node_size_and_time (struct cgraph_node
> *node,
> vec<inline_param_summary>
> inline_param_summary);
>
> +void ipa_fnsummary_c_finalize (void);
> +
> #endif /* GCC_IPA_FNSUMMARY_H */
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index f68de3b..590ab58 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -83,6 +83,7 @@ along with GCC; see the file COPYING3. If not see
> #include "edit-context.h"
> #include "tree-pass.h"
> #include "dumpfile.h"
> +#include "ipa-fnsummary.h"
>
> #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
> #include "dbxout.h"
> @@ -2238,6 +2239,7 @@ toplev::finalize (void)
>
> /* Needs to be called before cgraph_c_finalize since it uses symtab. */
> ipa_reference_c_finalize ();
> + ipa_fnsummary_c_finalize ();
>
> cgraph_c_finalize ();
> cgraphunit_c_finalize ();
> --
> 1.8.5.3
>
