* [PATCH] Handle fancy_abort before diagnostic initialization [PR98586]
@ 2021-01-11 21:56 David Malcolm
2021-01-12 7:44 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: David Malcolm @ 2021-01-11 21:56 UTC (permalink / raw)
To: gcc-patches
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(-)
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Handle fancy_abort before diagnostic initialization [PR98586]
2021-01-11 21:56 [PATCH] Handle fancy_abort before diagnostic initialization [PR98586] David Malcolm
@ 2021-01-12 7:44 ` Richard Biener
2021-01-14 22:13 ` David Malcolm
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2021-01-12 7:44 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Handle fancy_abort before diagnostic initialization [PR98586]
2021-01-12 7:44 ` Richard Biener
@ 2021-01-14 22:13 ` David Malcolm
0 siblings, 0 replies; 3+ messages in thread
From: David Malcolm @ 2021-01-14 22:13 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-14 22:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 21:56 [PATCH] Handle fancy_abort before diagnostic initialization [PR98586] David Malcolm
2021-01-12 7:44 ` Richard Biener
2021-01-14 22:13 ` David Malcolm
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).