public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Dump function on internal errors
@ 2017-05-22 14:26 Andi Kleen
  2017-05-24 13:45 ` Richard Biener
  0 siblings, 1 reply; 11+ messages in thread
From: Andi Kleen @ 2017-05-22 14:26 UTC (permalink / raw)
  To: gcc-patches; +Cc: Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

When a verification check fails it is useful to dump the current
function to the dump file, so it's easier to figure out what
actually went wrong.

v2: Updated version now using a hook in internal_error, and
also prints the pass name and the dump file name.

gcc/:

2017-05-21  Andi Kleen  <ak@linux.intel.com>

        * diagnostic.c (internal_error): Call hook.
        (set_internal_error_hook): New function.
        * diagnostic.h: Add set_internal_error_hook.
        * passes.c: (pass_ice_hook): Add class to set
	internal_error_hook to dump functions.
---
 gcc/diagnostic.c | 18 ++++++++++++++++++
 gcc/diagnostic.h |  1 +
 gcc/passes.c     | 21 +++++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 158519606f8..8d9caade60b 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -33,6 +33,9 @@ along with GCC; see the file COPYING3.  If not see
 #include "diagnostic-color.h"
 #include "edit-context.h"
 #include "selftest.h"
+#include "tree.h"
+#include "tree-pass.h"
+#include "tree-cfg.h"
 
 #ifdef HAVE_TERMIOS_H
 # include <termios.h>
@@ -1396,6 +1399,10 @@ fatal_error (location_t loc, const char *gmsgid, ...)
   gcc_unreachable ();
 }
 
+/* Hook to call on internal errors.  */
+
+static void (*internal_error_hook)();
+
 /* An internal consistency check has failed.  We make no attempt to
    continue.  Note that unless there is debugging value to be had from
    a more specific message, or some other good reason, you should use
@@ -1403,6 +1410,9 @@ fatal_error (location_t loc, const char *gmsgid, ...)
 void
 internal_error (const char *gmsgid, ...)
 {
+  if (internal_error_hook)
+    internal_error_hook ();
+
   va_list ap;
   va_start (ap, gmsgid);
   rich_location richloc (line_table, input_location);
@@ -1412,6 +1422,14 @@ internal_error (const char *gmsgid, ...)
   gcc_unreachable ();
 }
 
+/* Call HOOK on internal errors.  */
+
+void
+set_internal_error_hook (void (*hook)())
+{
+  internal_error_hook = hook;
+}
+
 /* Like internal_error, but no backtrace will be printed.  Used when
    the internal error does not happen at the current location, but happened
    somewhere else.  */
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index dbd1703e0ef..3d26a84ab44 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -369,5 +369,6 @@ extern char *file_name_as_prefix (diagnostic_context *, const char *);
 
 extern char *build_message_string (const char *, ...) ATTRIBUTE_PRINTF_1;
 
+extern void set_internal_error_hook (void (*)());
 
 #endif /* ! GCC_DIAGNOSTIC_H */
diff --git a/gcc/passes.c b/gcc/passes.c
index e7c5d194010..1bd92c8140a 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -3040,4 +3040,25 @@ function_called_by_processed_nodes_p (void)
   return e != NULL;
 }
 
+class pass_ice_hook {
+
+  /* Print function to dump file on internal errors.  */
+
+  static void ice_hook() {
+    if (dump_file)
+      {
+	fprintf (stderr, "Dumping function to dump file %s for pass %s\n",
+		 dump_file_name, current_pass->name);
+	dump_function_to_file (current_function_decl, dump_file, dump_flags);
+      }
+  }
+
+ public:
+  pass_ice_hook() {
+    set_internal_error_hook (ice_hook);
+  }
+};
+
+static pass_ice_hook pass_ice_hook;
+
 #include "gt-passes.h"
-- 
2.12.2

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-22 14:26 [PATCH] Dump function on internal errors Andi Kleen
@ 2017-05-24 13:45 ` Richard Biener
  2017-05-29 16:22   ` Alexander Monakov
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2017-05-24 13:45 UTC (permalink / raw)
  To: Andi Kleen; +Cc: GCC Patches, Andi Kleen

On Mon, May 22, 2017 at 4:19 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> When a verification check fails it is useful to dump the current
> function to the dump file, so it's easier to figure out what
> actually went wrong.
>
> v2: Updated version now using a hook in internal_error, and
> also prints the pass name and the dump file name.
>
> gcc/:
>
> 2017-05-21  Andi Kleen  <ak@linux.intel.com>
>
>         * diagnostic.c (internal_error): Call hook.
>         (set_internal_error_hook): New function.
>         * diagnostic.h: Add set_internal_error_hook.
>         * passes.c: (pass_ice_hook): Add class to set
>         internal_error_hook to dump functions.
> ---
>  gcc/diagnostic.c | 18 ++++++++++++++++++
>  gcc/diagnostic.h |  1 +
>  gcc/passes.c     | 21 +++++++++++++++++++++
>  3 files changed, 40 insertions(+)
>
> diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
> index 158519606f8..8d9caade60b 100644
> --- a/gcc/diagnostic.c
> +++ b/gcc/diagnostic.c
> @@ -33,6 +33,9 @@ along with GCC; see the file COPYING3.  If not see
>  #include "diagnostic-color.h"
>  #include "edit-context.h"
>  #include "selftest.h"
> +#include "tree.h"
> +#include "tree-pass.h"
> +#include "tree-cfg.h"
>
>  #ifdef HAVE_TERMIOS_H
>  # include <termios.h>
> @@ -1396,6 +1399,10 @@ fatal_error (location_t loc, const char *gmsgid, ...)
>    gcc_unreachable ();
>  }
>
> +/* Hook to call on internal errors.  */
> +
> +static void (*internal_error_hook)();
> +
>  /* An internal consistency check has failed.  We make no attempt to
>     continue.  Note that unless there is debugging value to be had from
>     a more specific message, or some other good reason, you should use
> @@ -1403,6 +1410,9 @@ fatal_error (location_t loc, const char *gmsgid, ...)
>  void
>  internal_error (const char *gmsgid, ...)
>  {
> +  if (internal_error_hook)
> +    internal_error_hook ();
> +
>    va_list ap;
>    va_start (ap, gmsgid);
>    rich_location richloc (line_table, input_location);
> @@ -1412,6 +1422,14 @@ internal_error (const char *gmsgid, ...)
>    gcc_unreachable ();
>  }
>
> +/* Call HOOK on internal errors.  */
> +
> +void
> +set_internal_error_hook (void (*hook)())
> +{
> +  internal_error_hook = hook;
> +}
> +
>  /* Like internal_error, but no backtrace will be printed.  Used when
>     the internal error does not happen at the current location, but happened
>     somewhere else.  */
> diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
> index dbd1703e0ef..3d26a84ab44 100644
> --- a/gcc/diagnostic.h
> +++ b/gcc/diagnostic.h
> @@ -369,5 +369,6 @@ extern char *file_name_as_prefix (diagnostic_context *, const char *);
>
>  extern char *build_message_string (const char *, ...) ATTRIBUTE_PRINTF_1;
>
> +extern void set_internal_error_hook (void (*)());
>
>  #endif /* ! GCC_DIAGNOSTIC_H */
> diff --git a/gcc/passes.c b/gcc/passes.c
> index e7c5d194010..1bd92c8140a 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -3040,4 +3040,25 @@ function_called_by_processed_nodes_p (void)
>    return e != NULL;
>  }
>
> +class pass_ice_hook {
> +
> +  /* Print function to dump file on internal errors.  */
> +
> +  static void ice_hook() {
> +    if (dump_file)
> +      {
> +       fprintf (stderr, "Dumping function to dump file %s for pass %s\n",
> +                dump_file_name, current_pass->name);
> +       dump_function_to_file (current_function_decl, dump_file, dump_flags);
> +      }
> +  }
> +
> + public:
> +  pass_ice_hook() {
> +    set_internal_error_hook (ice_hook);
> +  }
> +};
> +
> +static pass_ice_hook pass_ice_hook;

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).

Otherwise looks reasonable to me.

Richard.

> +
>  #include "gt-passes.h"
> --
> 2.12.2
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-24 13:45 ` Richard Biener
@ 2017-05-29 16:22   ` Alexander Monakov
  2017-05-29 16:24     ` Jakub Jelinek
                       ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Alexander Monakov @ 2017-05-29 16:22 UTC (permalink / raw)
  To: Richard Biener; +Cc: Andi Kleen, GCC Patches, Andi Kleen

[-- Attachment #1: Type: text/plain, Size: 5350 bytes --]

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(-)

diff --git a/gcc/passes.c b/gcc/passes.c
index 98e05e4..e8e0322 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -60,6 +60,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-live.h"  /* For remove_unused_locals.  */
 #include "tree-cfgcleanup.h"
 #include "insn-addr.h" /* for INSN_ADDRESSES_ALLOC.  */
+#include "diagnostic-core.h" /* for fnotice */
 
 using namespace gcc;
 
@@ -1779,6 +1780,19 @@ execute_function_dump (function *fn, void *data)
     }
 }
 
+/* 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);
+}
+
 static struct profile_record *profile_record;
 
 /* Do profile consistency book-keeping for the pass with static number INDEX.
diff --git a/gcc/plugin.c b/gcc/plugin.c
index c6d3cdd..9892748 100644
--- a/gcc/plugin.c
+++ b/gcc/plugin.c
@@ -858,16 +858,6 @@ warn_if_plugins (void)
 
 }
 
-/* Likewise, as a callback from the diagnostics code.  */
-
-void
-plugins_internal_error_function (diagnostic_context *context ATTRIBUTE_UNUSED,
-				 const char *msgid ATTRIBUTE_UNUSED,
-				 va_list *ap ATTRIBUTE_UNUSED)
-{
-  warn_if_plugins ();
-}
-
 /* The default version check. Compares every field in VERSION. */
 
 bool
diff --git a/gcc/plugin.h b/gcc/plugin.h
index 68a673b..b96445d 100644
--- a/gcc/plugin.h
+++ b/gcc/plugin.h
@@ -167,8 +167,6 @@ extern bool plugins_active_p (void);
 extern void dump_active_plugins (FILE *);
 extern void debug_active_plugins (void);
 extern void warn_if_plugins (void);
-extern void plugins_internal_error_function (diagnostic_context *,
-					     const char *, va_list *);
 extern void print_plugins_versions (FILE *file, const char *indent);
 extern void print_plugins_help (FILE *file, const char *indent);
 extern void finalize_plugins (void);
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 425315c..23b884a 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -79,6 +79,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "omp-offload.h"
 #include "hsa-common.h"
 #include "edit-context.h"
+#include "tree-pass.h"
 
 #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
 #include "dbxout.h"
@@ -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 ();
+}
+
 /* Initialization of the front end environment, before command line
    options are parsed.  Signal handlers, internationalization etc.
    ARGV0 is main's argv[0].  */
@@ -1101,7 +1113,7 @@ general_init (const char *argv0, bool init_signals)
     = global_options_init.x_flag_diagnostics_show_option;
   global_dc->show_column
     = global_options_init.x_flag_show_column;
-  global_dc->internal_error = plugins_internal_error_function;
+  global_dc->internal_error = internal_error_function;
   global_dc->option_enabled = option_enabled;
   global_dc->option_state = &global_options;
   global_dc->option_name = option_name;
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index cfa4b01..0f7d936 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -634,6 +634,7 @@ extern void execute_all_ipa_transforms (void);
 extern void execute_all_ipa_stmt_fixups (struct cgraph_node *, gimple **);
 extern bool pass_init_dump_file (opt_pass *);
 extern void pass_fini_dump_file (opt_pass *);
+extern void emergency_dump_function (void);
 
 extern void print_current_pass (FILE *);
 extern void debug_pass (void);
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-29 16:22   ` Alexander Monakov
@ 2017-05-29 16:24     ` Jakub Jelinek
  2017-05-29 16:42       ` Alexander Monakov
  2017-05-29 16:46     ` Andi Kleen
  2017-05-29 19:33     ` Alexander Monakov
  2 siblings, 1 reply; 11+ messages in thread
From: Jakub Jelinek @ 2017-05-29 16:24 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: Richard Biener, Andi Kleen, GCC Patches, Andi Kleen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-29 16:24     ` Jakub Jelinek
@ 2017-05-29 16:42       ` Alexander Monakov
  2017-05-29 16:51         ` Alexander Monakov
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Monakov @ 2017-05-29 16:42 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Richard Biener, Andi Kleen, GCC Patches, Andi Kleen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-29 16:22   ` Alexander Monakov
  2017-05-29 16:24     ` Jakub Jelinek
@ 2017-05-29 16:46     ` Andi Kleen
  2017-05-29 19:33     ` Alexander Monakov
  2 siblings, 0 replies; 11+ messages in thread
From: Andi Kleen @ 2017-05-29 16:46 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: Richard Biener, Andi Kleen, GCC Patches, Andi Kleen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-29 16:42       ` Alexander Monakov
@ 2017-05-29 16:51         ` Alexander Monakov
  2017-05-29 16:53           ` Jakub Jelinek
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Monakov @ 2017-05-29 16:51 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Richard Biener, Andi Kleen, GCC Patches, Andi Kleen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-29 16:51         ` Alexander Monakov
@ 2017-05-29 16:53           ` Jakub Jelinek
  0 siblings, 0 replies; 11+ messages in thread
From: Jakub Jelinek @ 2017-05-29 16:53 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: Richard Biener, Andi Kleen, GCC Patches, Andi Kleen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-29 16:22   ` Alexander Monakov
  2017-05-29 16:24     ` Jakub Jelinek
  2017-05-29 16:46     ` Andi Kleen
@ 2017-05-29 19:33     ` Alexander Monakov
  2017-05-30  7:44       ` Richard Biener
  2 siblings, 1 reply; 11+ messages in thread
From: Alexander Monakov @ 2017-05-29 19:33 UTC (permalink / raw)
  To: Richard Biener; +Cc: Andi Kleen, GCC Patches, Andi Kleen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-29 19:33     ` Alexander Monakov
@ 2017-05-30  7:44       ` Richard Biener
  2017-05-30 10:35         ` Alexander Monakov
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2017-05-30  7:44 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: Andi Kleen, GCC Patches, Andi Kleen

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Dump function on internal errors
  2017-05-30  7:44       ` Richard Biener
@ 2017-05-30 10:35         ` Alexander Monakov
  0 siblings, 0 replies; 11+ messages in thread
From: Alexander Monakov @ 2017-05-30 10:35 UTC (permalink / raw)
  To: Richard Biener; +Cc: Andi Kleen, GCC Patches, Andi Kleen

On Tue, 30 May 2017, Richard Biener wrote:
> 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.  I'm going to check in the following:

	* 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    | 19 +++++++++++++++++++
 gcc/plugin.c    | 10 ----------
 gcc/plugin.h    |  2 --
 gcc/toplev.c    | 12 +++++++++++-
 gcc/tree-pass.h |  1 +
 5 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/gcc/passes.c b/gcc/passes.c
index 98e05e4..64493ba 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -60,6 +60,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-live.h"  /* For remove_unused_locals.  */
 #include "tree-cfgcleanup.h"
 #include "insn-addr.h" /* for INSN_ADDRESSES_ALLOC.  */
+#include "diagnostic-core.h" /* for fnotice */
 
 using namespace gcc;
 
@@ -1779,6 +1780,24 @@ execute_function_dump (function *fn, void *data)
     }
 }
 
+/* This function is called when an internal compiler error is encountered.
+   Ensure that function dump is made available before compiler is aborted.  */
+
+void
+emergency_dump_function ()
+{
+  if (!current_pass)
+    return;
+  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 (!dump_file || !cfun)
+    return;
+  fnotice (stderr, "dump file: %s\n", dump_file_name);
+  execute_function_dump (cfun, current_pass);
+}
+
 static struct profile_record *profile_record;
 
 /* Do profile consistency book-keeping for the pass with static number INDEX.
diff --git a/gcc/plugin.c b/gcc/plugin.c
index c6d3cdd..9892748 100644
--- a/gcc/plugin.c
+++ b/gcc/plugin.c
@@ -858,16 +858,6 @@ warn_if_plugins (void)
 
 }
 
-/* Likewise, as a callback from the diagnostics code.  */
-
-void
-plugins_internal_error_function (diagnostic_context *context ATTRIBUTE_UNUSED,
-				 const char *msgid ATTRIBUTE_UNUSED,
-				 va_list *ap ATTRIBUTE_UNUSED)
-{
-  warn_if_plugins ();
-}
-
 /* The default version check. Compares every field in VERSION. */
 
 bool
diff --git a/gcc/plugin.h b/gcc/plugin.h
index 68a673b..b96445d 100644
--- a/gcc/plugin.h
+++ b/gcc/plugin.h
@@ -167,8 +167,6 @@ extern bool plugins_active_p (void);
 extern void dump_active_plugins (FILE *);
 extern void debug_active_plugins (void);
 extern void warn_if_plugins (void);
-extern void plugins_internal_error_function (diagnostic_context *,
-					     const char *, va_list *);
 extern void print_plugins_versions (FILE *file, const char *indent);
 extern void print_plugins_help (FILE *file, const char *indent);
 extern void finalize_plugins (void);
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 425315c..f8b5a40 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -79,6 +79,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "omp-offload.h"
 #include "hsa-common.h"
 #include "edit-context.h"
+#include "tree-pass.h"
 
 #if defined(DBX_DEBUGGING_INFO) || defined(XCOFF_DEBUGGING_INFO)
 #include "dbxout.h"
@@ -1063,6 +1064,15 @@ open_auxiliary_file (const char *ext)
   return file;
 }
 
+/* Auxiliary callback for the diagnostics code.  */
+
+static void
+internal_error_function (diagnostic_context *, const char *, va_list *)
+{
+  warn_if_plugins ();
+  emergency_dump_function ();
+}
+
 /* Initialization of the front end environment, before command line
    options are parsed.  Signal handlers, internationalization etc.
    ARGV0 is main's argv[0].  */
@@ -1101,7 +1111,7 @@ general_init (const char *argv0, bool init_signals)
     = global_options_init.x_flag_diagnostics_show_option;
   global_dc->show_column
     = global_options_init.x_flag_show_column;
-  global_dc->internal_error = plugins_internal_error_function;
+  global_dc->internal_error = internal_error_function;
   global_dc->option_enabled = option_enabled;
   global_dc->option_state = &global_options;
   global_dc->option_name = option_name;
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index cfa4b01..0f7d936 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -634,6 +634,7 @@ extern void execute_all_ipa_transforms (void);
 extern void execute_all_ipa_stmt_fixups (struct cgraph_node *, gimple **);
 extern bool pass_init_dump_file (opt_pass *);
 extern void pass_fini_dump_file (opt_pass *);
+extern void emergency_dump_function (void);
 
 extern void print_current_pass (FILE *);
 extern void debug_pass (void);
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-05-30 10:33 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-22 14:26 [PATCH] Dump function on internal errors Andi Kleen
2017-05-24 13:45 ` Richard Biener
2017-05-29 16:22   ` Alexander Monakov
2017-05-29 16:24     ` Jakub Jelinek
2017-05-29 16:42       ` Alexander Monakov
2017-05-29 16:51         ` Alexander Monakov
2017-05-29 16:53           ` Jakub Jelinek
2017-05-29 16:46     ` Andi Kleen
2017-05-29 19:33     ` Alexander Monakov
2017-05-30  7:44       ` Richard Biener
2017-05-30 10:35         ` Alexander Monakov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).