Re: [PATCH 2/2] Fix -fsave-optimization-record ICE (PR tree-optimization/87025)

[Richard Biener <richard dot guenther at gmail dot com>]

On Sat, Nov 17, 2018 at 1:12 AM David Malcolm <dmalcolm@redhat.com> wrote:
>
> PR tree-optimization/87025 reports an ICE within
> -fsave-optimization-record's optrecord_json_writer.
>
> The issue is that dump_context::begin_scope creates an optinfo
> of kind OPTINFO_KIND_SCOPE, but fails to call
> dump_context::end_any_optinfo, so the optinfo for the scope remains
> pending.
>
> The JSON writer would normally push a JSON array for the "scope" optinfo
> when the latter is emitted.  However, if a dump_* call happens that
> doesn't flush the "scope" optinfo e.g. dump_printf (as opposed to
> dump_printf_loc), that dump_ call is added to the pending optinfo, and
> optinfo::handle_dump_file_kind changes the pending optinfo's m_kind
> (e.g. to OPTINFO_KIND_NOTE).  Hence when the pending optinfo is
> eventually emitted, it isn't OPTINFO_KIND_SCOPE anymore, and hence
> the JSON writer doesn't create and push a JSON array for it, leading
> to dump_context's view of scopes getting out-of-sync with that of
> the JSON writer's.
>
> Later, dump_context::end_scope unconditionally tries to pop the JSON scope
> array, but no JSON scope array was added, leading to an assertion
> failure (or crash).
>
> The fix is to call dump_context::end_any_optinfo immediately after
> creating the scope optinfo, so that it is emitted immediately, ensuring
> that the JSON writer stays in-sync with the dump_context.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu
> (in conjuction with the previous patch)
>
> OK for trunk?

OK.

> gcc/ChangeLog:
>         PR tree-optimization/87025
>         * dumpfile.c (dump_context::begin_scope): Call end_any_optinfo
>         immediately after creating the scope optinfo.
>         (selftest::test_pr87025): New function.
>         (selftest::dumpfile_c_tests): Call it.
>         * optinfo-emit-json.cc (optrecord_json_writer::pop_scope): Assert
>         that we're not popping the top-level records array.
>         * optinfo.cc (optinfo::handle_dump_file_kind): Assert that we're
>         not changing the kind of a "scope" optinfo.
>
> gcc/testsuite/ChangeLog:
>         PR tree-optimization/87025
>         * gcc.dg/pr87025.c: New test.
> ---
>  gcc/dumpfile.c                 | 16 ++++++++++++++++
>  gcc/optinfo-emit-json.cc       |  3 +++
>  gcc/optinfo.cc                 |  3 +++
>  gcc/testsuite/gcc.dg/pr87025.c | 22 ++++++++++++++++++++++
>  4 files changed, 44 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/pr87025.c
>
> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> index 014acf1..e125650 100644
> --- a/gcc/dumpfile.c
> +++ b/gcc/dumpfile.c
> @@ -1131,6 +1131,7 @@ dump_context::begin_scope (const char *name, const dump_location_t &loc)
>        optinfo &info = begin_next_optinfo (loc);
>        info.m_kind = OPTINFO_KIND_SCOPE;
>        info.add_item (item);
> +      end_any_optinfo ();
>      }
>    else
>      delete item;
> @@ -2575,6 +2576,20 @@ test_capture_of_dump_calls (const line_table_case &case_)
>    }
>  }
>
> +static void
> +test_pr87025 ()
> +{
> +  dump_user_location_t loc
> +    = dump_user_location_t::from_location_t (UNKNOWN_LOCATION);
> +
> +  temp_dump_context tmp (true, true,
> +                        MSG_ALL_KINDS | MSG_PRIORITY_USER_FACING);
> +  {
> +    AUTO_DUMP_SCOPE ("outer scope", loc);
> +    dump_printf (MSG_NOTE, "msg1\n");
> +  }
> +}
> +
>  /* Run all of the selftests within this file.  */
>
>  void
> @@ -2582,6 +2597,7 @@ dumpfile_c_tests ()
>  {
>    test_impl_location ();
>    for_each_line_table_case (test_capture_of_dump_calls);
> +  test_pr87025 ();
>  }
>
>  } // namespace selftest
> diff --git a/gcc/optinfo-emit-json.cc b/gcc/optinfo-emit-json.cc
> index 4fa6708..841a13b 100644
> --- a/gcc/optinfo-emit-json.cc
> +++ b/gcc/optinfo-emit-json.cc
> @@ -169,6 +169,9 @@ void
>  optrecord_json_writer::pop_scope ()
>  {
>    m_scopes.pop ();
> +
> +  /* We should never pop the top-level records array.  */
> +  gcc_assert (m_scopes.length () > 0);
>  }
>
>  /* Create a JSON object representing LOC.  */
> diff --git a/gcc/optinfo.cc b/gcc/optinfo.cc
> index f8e08de..f76da45 100644
> --- a/gcc/optinfo.cc
> +++ b/gcc/optinfo.cc
> @@ -133,6 +133,9 @@ optinfo::emit_for_opt_problem () const
>  void
>  optinfo::handle_dump_file_kind (dump_flags_t dump_kind)
>  {
> +  /* Any optinfo for a "scope" should have been emitted separately.  */
> +  gcc_assert (m_kind != OPTINFO_KIND_SCOPE);
> +
>    if (dump_kind & MSG_OPTIMIZED_LOCATIONS)
>      m_kind = OPTINFO_KIND_SUCCESS;
>    else if (dump_kind & MSG_MISSED_OPTIMIZATION)
> diff --git a/gcc/testsuite/gcc.dg/pr87025.c b/gcc/testsuite/gcc.dg/pr87025.c
> new file mode 100644
> index 0000000..059313c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr87025.c
> @@ -0,0 +1,22 @@
> +/* Ensure we don't ICE when tracking optimization record scopes within
> +   the vectorizer.  */
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fsave-optimization-record -ftree-vectorize -fno-tree-scev-cprop -fno-tree-sink" } */
> +
> +void
> +fk (unsigned int sf)
> +{
> +  for (;;)
> +    {
> +      if (sf != 0)
> +        {
> +          while (sf != 0)
> +            ++sf;
> +
> +          while (sf < 8)
> +            ++sf;
> +        }
> +
> +      ++sf;
> +    }
> +}
> --
> 1.8.5.3
>