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

Message ID 1542413514-48487-2-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • [1/2] Eliminate global state from -fsave-optimization-record
Related show

Commit Message

David Malcolm Nov. 17, 2018, 12:11 a.m.
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?

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

-- 
1.8.5.3

Comments

Richard Biener Nov. 19, 2018, 10:34 a.m. | #1
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

>

Patch

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;
+    }
+}