Handle fancy_abort before diagnostic initialization [PR98586]

If fancy_abort is called before the diagnostic subsystem is initialized,
internal_error will crash internally in a way that prevents a useful
message reaching the user.

This can happen with libgccjit in the case of gcc_assert failures
that occur outside of the libgccjit mutex that guards the rest of
gcc's state, including global_dc (when global_dc may not be
initialized yet, or might be in use by another thread).

I tried a few approaches to fixing this as noted in PR jit/98586
e.g. using a temporary diagnostic_context and initializing it for
the call to internal_error, however the more code that runs, the
more chance there is for other errors to occur.

The best fix appears to be to simply fall back to a minimal abort
implementation that only relies on i18n, as implemented by this
patch.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

Is there a better way to fix this?  If not I plan to push this
to master in a few days.

gcc/ChangeLog:
	PR jit/98586
	* diagnostic.c (diagnostic_kind_text): Break out this array
	from...
	(diagnostic_build_prefix): ...here.
	(fancy_abort): Detect when diagnostic_initialize has not yet been
	called and fall back to a minimal implementation of printing the
	ICE, rather than segfaulting in internal_error.
---
 gcc/diagnostic.c | 45 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

On Mon, Jan 11, 2021 at 10:57 PM David Malcolm via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> If fancy_abort is called before the diagnostic subsystem is initialized,
> internal_error will crash internally in a way that prevents a useful
> message reaching the user.
>
> This can happen with libgccjit in the case of gcc_assert failures
> that occur outside of the libgccjit mutex that guards the rest of
> gcc's state, including global_dc (when global_dc may not be
> initialized yet, or might be in use by another thread).
>
> I tried a few approaches to fixing this as noted in PR jit/98586
> e.g. using a temporary diagnostic_context and initializing it for
> the call to internal_error, however the more code that runs, the
> more chance there is for other errors to occur.
>
> The best fix appears to be to simply fall back to a minimal abort
> implementation that only relies on i18n, as implemented by this
> patch.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>
> Is there a better way to fix this?  If not I plan to push this
> to master in a few days.

The only other idea I can come up with is to somehow statically
initialize global_dc to a minimal implementation to catch those.

Otherwise your approach is entirely reasonable.

Richard.

> gcc/ChangeLog:
>         PR jit/98586
>         * diagnostic.c (diagnostic_kind_text): Break out this array
>         from...
>         (diagnostic_build_prefix): ...here.
>         (fancy_abort): Detect when diagnostic_initialize has not yet been
>         called and fall back to a minimal implementation of printing the
>         ICE, rather than segfaulting in internal_error.
> ---
>  gcc/diagnostic.c | 45 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
> index 4250bf96c8b..3be7748eb39 100644
> --- a/gcc/diagnostic.c
> +++ b/gcc/diagnostic.c
> @@ -431,6 +431,13 @@ diagnostic_get_location_text (diagnostic_context *context,
>                                line_col, locus_ce);
>  }
>
> +static const char *const diagnostic_kind_text[] = {
> +#define DEFINE_DIAGNOSTIC_KIND(K, T, C) (T),
> +#include "diagnostic.def"
> +#undef DEFINE_DIAGNOSTIC_KIND
> +  "must-not-happen"
> +};
> +
>  /* Return a malloc'd string describing a location and the severity of the
>     diagnostic, e.g. "foo.c:42:10: error: ".  The caller is responsible for
>     freeing the memory.  */
> @@ -438,12 +445,6 @@ char *
>  diagnostic_build_prefix (diagnostic_context *context,
>                          const diagnostic_info *diagnostic)
>  {
> -  static const char *const diagnostic_kind_text[] = {
> -#define DEFINE_DIAGNOSTIC_KIND(K, T, C) (T),
> -#include "diagnostic.def"
> -#undef DEFINE_DIAGNOSTIC_KIND
> -    "must-not-happen"
> -  };
>    gcc_assert (diagnostic->kind < DK_LAST_DIAGNOSTIC_KIND);
>
>    const char *text = _(diagnostic_kind_text[diagnostic->kind]);
> @@ -1832,6 +1833,38 @@ error_recursion (diagnostic_context *context)
>  void
>  fancy_abort (const char *file, int line, const char *function)
>  {
> +  /* If fancy_abort is called before the diagnostic subsystem is initialized,
> +     internal_error will crash internally in a way that prevents a
> +     useful message reaching the user.
> +     This can happen with libgccjit in the case of gcc_assert failures
> +     that occur outside of the libgccjit mutex that guards the rest of
> +     gcc's state, including global_dc (when global_dc may not be
> +     initialized yet, or might be in use by another thread).
> +     Handle such cases as gracefully as possible by falling back to a
> +     minimal abort handler that only relies on i18n.  */
> +  if (global_dc->printer == NULL)
> +    {
> +      /* Print the error message.  */
> +      fnotice (stderr, diagnostic_kind_text[DK_ICE]);
> +      fnotice (stderr, "in %s, at %s:%d", function, trim_filename (file), line);
> +      fputc ('\n', stderr);
> +
> +      /* Attempt to print a backtrace.  */
> +      struct backtrace_state *state
> +       = backtrace_create_state (NULL, 0, bt_err_callback, NULL);
> +      int count = 0;
> +      if (state != NULL)
> +       backtrace_full (state, 2, bt_callback, bt_err_callback,
> +                       (void *) &count);
> +
> +      /* We can't call warn_if_plugins or emergency_dump_function as these
> +        rely on GCC state that might not be initialized, or might be in
> +        use by another thread.  */
> +
> +      /* Abort the process.  */
> +      real_abort ();
> +    }
> +
>    internal_error ("in %s, at %s:%d", function, trim_filename (file), line);
>  }
>
> --
> 2.26.2
>

On Tue, 2021-01-12 at 08:44 +0100, Richard Biener wrote:
> On Mon, Jan 11, 2021 at 10:57 PM David Malcolm via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> > If fancy_abort is called before the diagnostic subsystem is
> > initialized,
> > internal_error will crash internally in a way that prevents a
> > useful
> > message reaching the user.
> > 
> > This can happen with libgccjit in the case of gcc_assert failures
> > that occur outside of the libgccjit mutex that guards the rest of
> > gcc's state, including global_dc (when global_dc may not be
> > initialized yet, or might be in use by another thread).
> > 
> > I tried a few approaches to fixing this as noted in PR jit/98586
> > e.g. using a temporary diagnostic_context and initializing it for
> > the call to internal_error, however the more code that runs, the
> > more chance there is for other errors to occur.
> > 
> > The best fix appears to be to simply fall back to a minimal abort
> > implementation that only relies on i18n, as implemented by this
> > patch.
> > 
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > 
> > Is there a better way to fix this?  If not I plan to push this
> > to master in a few days.
> 
> The only other idea I can come up with is to somehow statically
> initialize global_dc to a minimal implementation to catch those.

We sort of do that already, given that it points at a global
diagnostic_context.  I think trying to integrate this handling with the
diagnostic subsystem any further will run into issues for the case of
libgccjit linked into a multithreaded program, given that global_dc is
global state.  Thinking about those kinds of cases makes me feel that
this crash-handling for the crash handler ought to be as simple as
possible.

> Otherwise your approach is entirely reasonable.

Thanks.  I've gone ahead and pushed this to master as r11-6700-
g387f6c15d303a8f8da508e419fea10a6ef0e2590.

Dave