diff mbox series

libgccjit: Add option to hide stderr logs [PR104073]

Message ID 26928da47b7f5cbcef6c9db31221fe59a83ef4b2.camel@zoho.com
State New
Headers show
Series libgccjit: Add option to hide stderr logs [PR104073] | expand

Commit Message

Antoni Boucher Jan. 18, 2022, 2:02 a.m. UTC 
Hi.
This option will be useful for rustc_codegen_gcc to hide the error
about unsupported 128-bit integer types.

David, if you know of a better way to check if these types are
supported than creating such a type and checking if it causes an error,
I will not need this patch.

Thanks for the reviews!

Comments

David Malcolm Jan. 18, 2022, 11:22 p.m. UTC | #1 
On Mon, 2022-01-17 at 21:02 -0500, Antoni Boucher via Gcc-patches
wrote:
> Hi.
> This option will be useful for rustc_codegen_gcc to hide the error
> about unsupported 128-bit integer types.
> 
> David, if you know of a better way to check if these types are
> supported than creating such a type and checking if it causes an error,
> I will not need this patch.

Off the top of my head I don't know of such a way.

That said, this seems to be vaguely analogous to a test in a
"configure" script, attempting a compile and seeing if it succeeds.

This seems like a useful pattern for libgccjit to support, so that
client code can query the capabilities of the host, so I think the idea
of this patch is sound.

As for the details of the patch, I don't like adding new members to the
enums in libgccjit.h; I prefer adding new entrypoints, as the latter
gives a way to tell if client code uses the new entrypoint as part of
the ELF metadata, so that we can tell directly that client code is
incompatible with an older libgccjit.so from the symbol metadata in the
built binary.

So I'd prefer something like:

  extern void
  gcc_jit_context_set_bool_print_errors_to_stderr (gcc_jit_context *ctxt,
                                                   int enabled);

where gcc_jit_context_set_bool_print_errors_to_stderr defaults to true,
but client code can use:

  gcc_jit_context_set_bool_print_errors_to_stderr (ctxt, false);

Or maybe have a way to specify the FILE * for errors to be printed to,
defaulting to stderr, but settable to NULL if you want to suppress the
printing?  That might be more flexible.

Thoughts?
Dave
Antoni Boucher Jan. 23, 2022, 5:34 p.m. UTC | #2 
Thanks for the review.
Here's the updated patch.

Le mardi 18 janvier 2022 à 18:22 -0500, David Malcolm a écrit :
> On Mon, 2022-01-17 at 21:02 -0500, Antoni Boucher via Gcc-patches
> wrote:
> > Hi.
> > This option will be useful for rustc_codegen_gcc to hide the error
> > about unsupported 128-bit integer types.
> > 
> > David, if you know of a better way to check if these types are
> > supported than creating such a type and checking if it causes an
> > error,
> > I will not need this patch.
> 
> Off the top of my head I don't know of such a way.
> 
> That said, this seems to be vaguely analogous to a test in a
> "configure" script, attempting a compile and seeing if it succeeds.
> 
> This seems like a useful pattern for libgccjit to support, so that
> client code can query the capabilities of the host, so I think the
> idea
> of this patch is sound.
> 
> As for the details of the patch, I don't like adding new members to
> the
> enums in libgccjit.h; I prefer adding new entrypoints, as the latter
> gives a way to tell if client code uses the new entrypoint as part of
> the ELF metadata, so that we can tell directly that client code is
> incompatible with an older libgccjit.so from the symbol metadata in
> the
> built binary.
> 
> So I'd prefer something like:
> 
>   extern void
>   gcc_jit_context_set_bool_print_errors_to_stderr (gcc_jit_context
> *ctxt,
>                                                    int enabled);
> 
> where gcc_jit_context_set_bool_print_errors_to_stderr defaults to
> true,
> but client code can use:
> 
>   gcc_jit_context_set_bool_print_errors_to_stderr (ctxt, false);
> 
> Or maybe have a way to specify the FILE * for errors to be printed
> to,
> defaulting to stderr, but settable to NULL if you want to suppress
> the
> printing?  That might be more flexible.
> 
> Thoughts?
> Dave
>
David Malcolm Jan. 24, 2022, 10:55 p.m. UTC | #3 
On Sun, 2022-01-23 at 12:34 -0500, Antoni Boucher wrote:
> Thanks for the review.
> Here's the updated patch.

Tnanks.  The updated patch looks good to me, but we need to get release
manager approval for adding stuff in stage 4; I'll email them when I've
looked at the other pending patches.

Dave

> 
> Le mardi 18 janvier 2022 à 18:22 -0500, David Malcolm a écrit :
> > On Mon, 2022-01-17 at 21:02 -0500, Antoni Boucher via Gcc-patches
> > wrote:
> > > Hi.
> > > This option will be useful for rustc_codegen_gcc to hide the
> > > error
> > > about unsupported 128-bit integer types.
> > > 
> > > David, if you know of a better way to check if these types are
> > > supported than creating such a type and checking if it causes an
> > > error,
> > > I will not need this patch.
> > 
> > Off the top of my head I don't know of such a way.
> > 
> > That said, this seems to be vaguely analogous to a test in a
> > "configure" script, attempting a compile and seeing if it succeeds.
> > 
> > This seems like a useful pattern for libgccjit to support, so that
> > client code can query the capabilities of the host, so I think the
> > idea
> > of this patch is sound.
> > 
> > As for the details of the patch, I don't like adding new members to
> > the
> > enums in libgccjit.h; I prefer adding new entrypoints, as the
> > latter
> > gives a way to tell if client code uses the new entrypoint as part
> > of
> > the ELF metadata, so that we can tell directly that client code is
> > incompatible with an older libgccjit.so from the symbol metadata in
> > the
> > built binary.
> > 
> > So I'd prefer something like:
> > 
> >   extern void
> >   gcc_jit_context_set_bool_print_errors_to_stderr (gcc_jit_context
> > *ctxt,
> >                                                    int enabled);
> > 
> > where gcc_jit_context_set_bool_print_errors_to_stderr defaults to
> > true,
> > but client code can use:
> > 
> >   gcc_jit_context_set_bool_print_errors_to_stderr (ctxt, false);
> > 
> > Or maybe have a way to specify the FILE * for errors to be printed
> > to,
> > defaulting to stderr, but settable to NULL if you want to suppress
> > the
> > printing?  That might be more flexible.
> > 
> > Thoughts?
> > Dave
> > 
>
David Malcolm April 12, 2022, 9:48 p.m. UTC | #4 
On Mon, 2022-01-24 at 17:55 -0500, David Malcolm wrote:
> On Sun, 2022-01-23 at 12:34 -0500, Antoni Boucher wrote:
> > Thanks for the review.
> > Here's the updated patch.
> 
> Tnanks.  The updated patch looks good to me, but we need to get release
> manager approval for adding stuff in stage 4; I'll email them when I've
> looked at the other pending patches.

I updated the patch slightly:
* fixed a copy&paste typo in the comment for
LIBGCCJIT_HAVE_gcc_jit_context_set_bool_print_errors_to_stderr
* regenerated .texinfo from .rst

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

I've pushed the patch to trunk for GCC 12 as r12-8119-g79e1a6fb9babb3.

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=79e1a6fb9babb34dfcb99964c37d3c4f8bb619ca

Thanks again for the patch
Dave

> 
> Dave
> 
> > 
> > Le mardi 18 janvier 2022 à 18:22 -0500, David Malcolm a écrit :
> > > On Mon, 2022-01-17 at 21:02 -0500, Antoni Boucher via Gcc-patches
> > > wrote:
> > > > Hi.
> > > > This option will be useful for rustc_codegen_gcc to hide the
> > > > error
> > > > about unsupported 128-bit integer types.
> > > > 
> > > > David, if you know of a better way to check if these types are
> > > > supported than creating such a type and checking if it causes an
> > > > error,
> > > > I will not need this patch.
> > > 
> > > Off the top of my head I don't know of such a way.
> > > 
> > > That said, this seems to be vaguely analogous to a test in a
> > > "configure" script, attempting a compile and seeing if it succeeds.
> > > 
> > > This seems like a useful pattern for libgccjit to support, so that
> > > client code can query the capabilities of the host, so I think the
> > > idea
> > > of this patch is sound.
> > > 
> > > As for the details of the patch, I don't like adding new members to
> > > the
> > > enums in libgccjit.h; I prefer adding new entrypoints, as the
> > > latter
> > > gives a way to tell if client code uses the new entrypoint as part
> > > of
> > > the ELF metadata, so that we can tell directly that client code is
> > > incompatible with an older libgccjit.so from the symbol metadata in
> > > the
> > > built binary.
> > > 
> > > So I'd prefer something like:
> > > 
> > >   extern void
> > >   gcc_jit_context_set_bool_print_errors_to_stderr (gcc_jit_context
> > > *ctxt,
> > >                                                    int enabled);
> > > 
> > > where gcc_jit_context_set_bool_print_errors_to_stderr defaults to
> > > true,
> > > but client code can use:
> > > 
> > >   gcc_jit_context_set_bool_print_errors_to_stderr (ctxt, false);
> > > 
> > > Or maybe have a way to specify the FILE * for errors to be printed
> > > to,
> > > defaulting to stderr, but settable to NULL if you want to suppress
> > > the
> > > printing?  That might be more flexible.
> > > 
> > > Thoughts?
> > > Dave
> > > 
> > 
> 
>
diff mbox series

Patch

From 002c6803ac7069bf18eabd6729e31de8e2be6128 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Sun, 9 Jan 2022 13:46:35 -0500
Subject: [PATCH] libgccjit: Add option to hide stderr logs [PR104073]

2022-01-17  Antoni Boucher <bouanto@zoho.com

gcc/jit/
	PR target/104073
	* docs/topics/contexts.rst: Add documentation for the new option
	GCC_JIT_BOOL_OPTION_HIDE_LOG_STDERR.
	* jit-recording.c: handle the option
	GCC_JIT_BOOL_OPTION_HIDE_LOG_STDERR.
	* libgccjit.h: New option GCC_JIT_BOOL_OPTION_HIDE_LOG_STDERR.
---
 gcc/jit/docs/topics/contexts.rst |  4 ++++
 gcc/jit/jit-recording.c          | 25 ++++++++++++++++---------
 gcc/jit/libgccjit.h              |  3 +++
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/gcc/jit/docs/topics/contexts.rst b/gcc/jit/docs/topics/contexts.rst
index 68ab7ab1321..193f5096de9 100644
--- a/gcc/jit/docs/topics/contexts.rst
+++ b/gcc/jit/docs/topics/contexts.rst
@@ -453,6 +453,10 @@  Boolean options
      If true, the :type:`gcc_jit_context` will not clean up intermediate files
      written to the filesystem, and will display their location on stderr.
 
+  .. macro:: GCC_JIT_BOOL_OPTION_HIDE_LOG_STDERR
+
+     If true, libgccjit will not log the errors on stderr.
+
 .. function:: void \
               gcc_jit_context_set_bool_allow_unreachable_blocks (gcc_jit_context *ctxt, \
                                                                  int bool_value)
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index ee8934131d1..a6928005e9b 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -1551,15 +1551,21 @@  recording::context::add_error_va (location *loc, const char *fmt, va_list ap)
   if (!ctxt_progname)
     ctxt_progname = "libgccjit.so";
 
-  if (loc)
-    fprintf (stderr, "%s: %s: error: %s\n",
-	     ctxt_progname,
-	     loc->get_debug_string (),
-	     errmsg);
-  else
-    fprintf (stderr, "%s: error: %s\n",
-	     ctxt_progname,
-	     errmsg);
+  bool hide_log_stderr =
+    get_bool_option (GCC_JIT_BOOL_OPTION_HIDE_LOG_STDERR);
+
+  if (!hide_log_stderr)
+  {
+    if (loc)
+      fprintf (stderr, "%s: %s: error: %s\n",
+	       ctxt_progname,
+	       loc->get_debug_string (),
+	       errmsg);
+    else
+      fprintf (stderr, "%s: error: %s\n",
+	       ctxt_progname,
+	       errmsg);
+  }
 
   if (!m_error_count)
     {
@@ -1682,6 +1688,7 @@  static const char * const
   "GCC_JIT_BOOL_OPTION_DUMP_EVERYTHING",
   "GCC_JIT_BOOL_OPTION_SELFCHECK_GC",
   "GCC_JIT_BOOL_OPTION_KEEP_INTERMEDIATES"
+  "GCC_JIT_BOOL_OPTION_HIDE_LOG_STDERR"
 };
 
 static const char * const
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 2a5ffacb1fe..272b862c164 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -241,6 +241,9 @@  enum gcc_jit_bool_option
      their location on stderr.  */
   GCC_JIT_BOOL_OPTION_KEEP_INTERMEDIATES,
 
+  /* If true, gcc_jit_context_release will not print the errors to stderr.  */
+  GCC_JIT_BOOL_OPTION_HIDE_LOG_STDERR,
+
   GCC_JIT_NUM_BOOL_OPTIONS
 };
 
-- 
2.26.2.7.g19db9cfb68.dirty