[PATCH] toplev: avoid recursive emergency_dump_function

[PATCH] toplev: avoid recursive emergency_dump_function

[Alexander Monakov]

Hi,

Segher pointed out on IRC that ICE reporting with dumps enabled got worse:
if emergency_dump_function itself leads to an ICE (e.g. by segfaulting),
nested ICE reporting will invoke emergency_dump_function in exactly the
same context, but not only would we uselessly announce current pass again,
this time around the second SIGSEGV will just terminate cc1 because the
signal handler is unregistered (so we won't print the backtrace).  Sorry
for not really considering the implications when submitting that patch.

Solve this by substituting the callback for global_dc->internal_error;
this avoids invoking emergency_dump_function, and also gives a
convenient point to flush the dump file.  OK to apply?

	* topvel.c (dumpfile.h): New include.
	(internal_error_reentered): New static function.  Use it...
	(internal_error_function): ...here to handle reentered internal_error.

diff --git a/gcc/toplev.c b/gcc/toplev.c
index e6c69a4..67254fb 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -80,6 +80,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "hsa-common.h"
 #include "edit-context.h"
 #include "tree-pass.h"
+#include "dumpfile.h"

 #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
 #include "dbxout.h"
@@ -1064,11 +1065,22 @@ open_auxiliary_file (const char *ext)
   return file;
 }

+/* Alternative diagnostics callback for reentered ICE reporting.  */
+
+static void
+internal_error_reentered (diagnostic_context *, const char *, va_list *)
+{
+  /* Flush the dump file if emergency_dump_function itself caused an ICE.  */
+  if (dump_file)
+    fflush (dump_file);
+}
+
 /* Auxiliary callback for the diagnostics code.  */

 static void
 internal_error_function (diagnostic_context *, const char *, va_list *)
 {
+  global_dc->internal_error = internal_error_reentered;
   warn_if_plugins ();
   emergency_dump_function ();
 }

Re: [PATCH] toplev: avoid recursive emergency_dump_function

[Segher Boessenkool]

Hi!

On Thu, Jul 20, 2017 at 05:40:28PM +0300, Alexander Monakov wrote:
> Segher pointed out on IRC that ICE reporting with dumps enabled got worse:
> if emergency_dump_function itself leads to an ICE (e.g. by segfaulting),
> nested ICE reporting will invoke emergency_dump_function in exactly the
> same context, but not only would we uselessly announce current pass again,
> this time around the second SIGSEGV will just terminate cc1 because the
> signal handler is unregistered (so we won't print the backtrace).  Sorry
> for not really considering the implications when submitting that patch.
> 
> Solve this by substituting the callback for global_dc->internal_error;
> this avoids invoking emergency_dump_function, and also gives a
> convenient point to flush the dump file.  OK to apply?

Extra thanks for that last point, it's been driving me nuts :-)

> 	* topvel.c (dumpfile.h): New include.

s/topvel/toplev/


Segher

Re: [PATCH] toplev: avoid recursive emergency_dump_function

[Alexander Monakov]

On Sat, 22 Jul 2017, Segher Boessenkool wrote:
> On Thu, Jul 20, 2017 at 05:40:28PM +0300, Alexander Monakov wrote:
> > Segher pointed out on IRC that ICE reporting with dumps enabled got worse:
> > if emergency_dump_function itself leads to an ICE (e.g. by segfaulting),
> > nested ICE reporting will invoke emergency_dump_function in exactly the
> > same context, but not only would we uselessly announce current pass again,
> > this time around the second SIGSEGV will just terminate cc1 because the
> > signal handler is unregistered (so we won't print the backtrace).  Sorry
> > for not really considering the implications when submitting that patch.
> > 
> > Solve this by substituting the callback for global_dc->internal_error;
> > this avoids invoking emergency_dump_function, and also gives a
> > convenient point to flush the dump file.  OK to apply?
> 
> Extra thanks for that last point, it's been driving me nuts :-)
> 
> > 	* topvel.c (dumpfile.h): New include.
> 
> s/topvel/toplev/

I assume this is not an approval to apply - can somebody give an explicit OK?

Thanks!
Alexander

Re: [PATCH] toplev: avoid recursive emergency_dump_function

[Alexander Monakov]

Hello,

On Thu, 20 Jul 2017, Alexander Monakov wrote:
> Segher pointed out on IRC that ICE reporting with dumps enabled got worse:
> if emergency_dump_function itself leads to an ICE (e.g. by segfaulting),
> nested ICE reporting will invoke emergency_dump_function in exactly the
> same context, but not only would we uselessly announce current pass again,
> this time around the second SIGSEGV will just terminate cc1 because the
> signal handler is unregistered (so we won't print the backtrace).  Sorry
> for not really considering the implications when submitting that patch.
> 
> Solve this by substituting the callback for global_dc->internal_error;
> this avoids invoking emergency_dump_function, and also gives a
> convenient point to flush the dump file.  OK to apply?
> 
> 	* topvel.c (dumpfile.h): New include.
> 	(internal_error_reentered): New static function.  Use it...
> 	(internal_error_function): ...here to handle reentered internal_error.

David, could you review this please?  Segher indicated in the other
subthread that he's happy with this solution.

Thanks.
Alexander

> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index e6c69a4..67254fb 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -80,6 +80,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "hsa-common.h"
>  #include "edit-context.h"
>  #include "tree-pass.h"
> +#include "dumpfile.h"
> 
>  #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
>  #include "dbxout.h"
> @@ -1064,11 +1065,22 @@ open_auxiliary_file (const char *ext)
>    return file;
>  }
> 
> +/* Alternative diagnostics callback for reentered ICE reporting.  */
> +
> +static void
> +internal_error_reentered (diagnostic_context *, const char *, va_list *)
> +{
> +  /* Flush the dump file if emergency_dump_function itself caused an ICE.  */
> +  if (dump_file)
> +    fflush (dump_file);
> +}
> +
>  /* Auxiliary callback for the diagnostics code.  */
> 
>  static void
>  internal_error_function (diagnostic_context *, const char *, va_list *)
>  {
> +  global_dc->internal_error = internal_error_reentered;
>    warn_if_plugins ();
>    emergency_dump_function ();
>  }

Re: [PATCH] toplev: avoid recursive emergency_dump_function

[Jeff Law]

On 07/20/2017 08:40 AM, Alexander Monakov wrote:
> Hi,
> 
> Segher pointed out on IRC that ICE reporting with dumps enabled got worse:
> if emergency_dump_function itself leads to an ICE (e.g. by segfaulting),
> nested ICE reporting will invoke emergency_dump_function in exactly the
> same context, but not only would we uselessly announce current pass again,
> this time around the second SIGSEGV will just terminate cc1 because the
> signal handler is unregistered (so we won't print the backtrace).  Sorry
> for not really considering the implications when submitting that patch.
> 
> Solve this by substituting the callback for global_dc->internal_error;
> this avoids invoking emergency_dump_function, and also gives a
> convenient point to flush the dump file.  OK to apply?
> 
> 	* topvel.c (dumpfile.h): New include.
> 	(internal_error_reentered): New static function.  Use it...
> 	(internal_error_function): ...here to handle reentered internal_error.
OK.
jeff