Dump function on internal errors

Hi,

On Wed, 24 May 2017, Richard Biener wrote:
> current_pass might be NULL so you better do set_internal_error_hook when
> we start executing passes (I detest global singletons to do such stuff anyway).

I think there are other problems in this patch, dump_function_to_file won't work
after transition to RTL (it only handles GIMPLE).  It's much better to just use
existing dump routine in passes.c and use the existing diagnostic callbacks.

Here's an alternative, imho a bit more streamlined patch.  Here's how the new
notice is placed:

ice.c: In function ‘fn1.part.0’:
ice.c:24:1: error: size of loop 1 should be 1, not 2
 }
 ^
function dumped to file ice.c.227r.expand
ice.c:24:1: internal compiler error: in verify_loop_structure, at cfgloop.c:1644
0x92aeb8 verify_loop_structure()
        /home/am/pr80640/gcc/gcc/cfgloop.c:1644


Bootstrapped on x86_64, OK for trunk?

Alexander

	* passes.c (emergency_dump_function): New.
	* tree-pass.h (emergency_dump_function): Declare.
	* plugin.c (plugins_internal_error_function): Remove.
	* plugin.h (plugins_internal_error_function): Remove declaration.
        * toplev.c (internal_error_function): New static function.  Use it...
	(general_init): ...here.
---
 gcc/passes.c    | 14 ++++++++++++++
 gcc/plugin.c    | 10 ----------
 gcc/plugin.h    |  2 --
 gcc/toplev.c    | 14 +++++++++++++-
 gcc/tree-pass.h |  1 +
 5 files changed, 28 insertions(+), 13 deletions(-)

On Mon, May 29, 2017 at 07:15:33PM +0300, Alexander Monakov wrote:
> @@ -1063,6 +1064,17 @@ open_auxiliary_file (const char *ext)
>    return file;
>  }
>  
> +/* Auxiliary callback for the diagnostics code.  */
> +
> +static void
> +internal_error_function (diagnostic_context *context ATTRIBUTE_UNUSED,
> +			 const char *msgid ATTRIBUTE_UNUSED,
> +			 va_list *ap ATTRIBUTE_UNUSED)
> +{
> +  warn_if_plugins ();
> +  emergency_dump_function ();

What if there is another ICE during the dumping?  Won't we then
end in endless recursion?  Perhaps global_dc->internal_error should
be cleared here first?
Also, as none of the arguments are used and we are in C++,
perhaps it should be
static void
internal_error_function (diagnostic_context *, const char *, va_list *)
{
?

	Jakub

On Mon, 29 May 2017, Jakub Jelinek wrote:
> What if there is another ICE during the dumping?  Won't we then
> end in endless recursion?  Perhaps global_dc->internal_error should
> be cleared here first?

Hm, no, as far as I can see existing diagnostic machinery is supposed to fully
handle that.  It detects recursion; see e.g. diagnostic.c: error_recursion.

> Also, as none of the arguments are used and we are in C++,
> perhaps it should be
> static void
> internal_error_function (diagnostic_context *, const char *, va_list *)
> {
> ?

Ah, it seems GCC tends to use either the long-winded form I've copy-pasted in my
patch, or the slightly shorter variant with 'type_name ARG_UNUSED (arg_name)'.
The shorthand form you're pointing out seems to be used only once, in
vmsdbgout.c, as far as I can tell.  I'll be happy to change my patch as desired
by the reviewer, of course :)

Alexander

On Mon, May 29, 2017 at 07:15:33PM +0300, Alexander Monakov wrote:
> Hi,
> 
> On Wed, 24 May 2017, Richard Biener wrote:
> > current_pass might be NULL so you better do set_internal_error_hook when
> > we start executing passes (I detest global singletons to do such stuff anyway).
> 
> I think there are other problems in this patch, dump_function_to_file won't work
> after transition to RTL (it only handles GIMPLE).  It's much better to just use
> existing dump routine in passes.c and use the existing diagnostic callbacks.

Your patch looks good to me.

Thanks,
-Andi

On Mon, 29 May 2017, Alexander Monakov wrote:
> On Mon, 29 May 2017, Jakub Jelinek wrote:
> > Also, as none of the arguments are used and we are in C++,
> > perhaps it should be
> > static void
> > internal_error_function (diagnostic_context *, const char *, va_list *)
> > {
> > ?
> 
> Ah, it seems GCC tends to use either the long-winded form I've copy-pasted in my
> patch, or the slightly shorter variant with 'type_name ARG_UNUSED (arg_name)'.
> The shorthand form you're pointing out seems to be used only once, in
> vmsdbgout.c, as far as I can tell.

Scratch this, I totally botched my grep invocation.  There's plenty of instances
where the shorthand form is used, and I'll be happy to use it here as well.

Alexander

On Mon, May 29, 2017 at 07:46:22PM +0300, Alexander Monakov wrote:
> On Mon, 29 May 2017, Alexander Monakov wrote:
> > On Mon, 29 May 2017, Jakub Jelinek wrote:
> > > Also, as none of the arguments are used and we are in C++,
> > > perhaps it should be
> > > static void
> > > internal_error_function (diagnostic_context *, const char *, va_list *)
> > > {
> > > ?
> > 
> > Ah, it seems GCC tends to use either the long-winded form I've copy-pasted in my
> > patch, or the slightly shorter variant with 'type_name ARG_UNUSED (arg_name)'.
> > The shorthand form you're pointing out seems to be used only once, in
> > vmsdbgout.c, as far as I can tell.
> 
> Scratch this, I totally botched my grep invocation.  There's plenty of instances
> where the shorthand form is used, and I'll be happy to use it here as well.

Generally, ARG_UNUSED or ATTRIBUTE_UNUSED needs to be used if the argument
is used conditionally (e.g. in macro that on some target uses its argument
and on another target ignores it).  Otherwise, sometimes somebody wants to
make clear what the names of the ignored arguments are, in that case one
can use those 2 forms too.  If the arguments are not used and it isn't
important how they are called, C++ omitted argument names are fine.

	Jakub

On Mon, 29 May 2017, Alexander Monakov wrote:
> +/* This helper function is invoked from diagnostic routines prior to aborting
> +   due to internal compiler error.  If a dump file is set up, dump the
> +   current function.  */
> +
> +void
> +emergency_dump_function ()
> +{
> +  if (!dump_file || !current_pass || !cfun)
> +    return;
> +  fnotice (stderr, "function dumped to file %s\n", dump_file_name);
> +  execute_function_dump (cfun, current_pass);
> +}

I've noticed that the notice is not terribly useful.  Perhaps it's better to
mention the failing pass when not producing the dump (untested):

void
emergency_dump_function ()
{
  if (!current_pass || !cfun)
    return;
  if (dump_file)
    {
      fnotice (stderr, "dump file: %s\n", dump_file_name);
      execute_function_dump (cfun, current_pass);
    }
  else if (current_pass->name[0] != '*')
    {
      enum opt_pass_type pt = current_pass->type;
      fnotice (stderr, "during %s pass: %s\n", 
	       pt == GIMPLE_PASS ? "GIMPLE" : pt == RTL_PASS ? "RTL" : "IPA",
	       current_pass->name);
    }
}

Alexander

On Mon, May 29, 2017 at 8:10 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
> On Mon, 29 May 2017, Alexander Monakov wrote:
>> +/* This helper function is invoked from diagnostic routines prior to aborting
>> +   due to internal compiler error.  If a dump file is set up, dump the
>> +   current function.  */
>> +
>> +void
>> +emergency_dump_function ()
>> +{
>> +  if (!dump_file || !current_pass || !cfun)
>> +    return;
>> +  fnotice (stderr, "function dumped to file %s\n", dump_file_name);
>> +  execute_function_dump (cfun, current_pass);
>> +}
>
> I've noticed that the notice is not terribly useful.  Perhaps it's better to
> mention the failing pass when not producing the dump (untested):
>
> void
> emergency_dump_function ()
> {
>   if (!current_pass || !cfun)
>     return;
>   if (dump_file)
>     {
>       fnotice (stderr, "dump file: %s\n", dump_file_name);
>       execute_function_dump (cfun, current_pass);
>     }
>   else if (current_pass->name[0] != '*')
>     {
>       enum opt_pass_type pt = current_pass->type;
>       fnotice (stderr, "during %s pass: %s\n",
>                pt == GIMPLE_PASS ? "GIMPLE" : pt == RTL_PASS ? "RTL" : "IPA",
>                current_pass->name);
>     }
> }

If you want to improve here I'd do

   if (current_pass)
     fnotice (stderr, "during %s pass: %s\n", ...
   if (dump_file && cfun)
     {
        fnotice (..);
        execute_function_dump ...
     }

and I'd print the pass name even if it starts with '*' (that just
means it won't get a dumpfile).

Generally the patch looks good to me.

Thanks,
Richard.

> Alexander