Re: [PATCH] libgccjit: Add support for setting the comment ident

[Antoni Boucher <bouanto@zoho.com>]

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

Thanks for the review.

Here's the updated patch. See answers to question below.

On Fri, 2024-01-05 at 14:39 -0500, David Malcolm wrote:
> On Fri, 2024-01-05 at 12:09 -0500, Antoni Boucher wrote:
> > Hi.
> > This patch adds support for setting the comment ident (analogous to
> > #ident "comment" in C).
> > Thanks for the review.
> 
> Thanks for the patch.
> 
> This may sound like a silly question, but what does #ident do and
> what
> is it used for?

This adds text to the .comment section.

> 
> FWIW I found this in our documentation:
>   https://gcc.gnu.org/onlinedocs/cpp/Other-Directives.html
> 
> [...snip...]
> 
> > +Output options
> > +**************
> > +
> > +.. function:: void gcc_jit_context_set_output_ident
> > (gcc_jit_context *ctxt,\
> > +                                                     const char*
> > output_ident)
> > +
> > +   Set the identifier to write in the .comment section of the
> > output file to
> > +   ``output_ident``. Analogous to:
> 
> ...but only on some targets, according to the link above.  Maybe add
> that link here?
> 
> > +
> > +   .. code-block:: c
> > +
> > +      #ident "My comment"
> > +
> > +   in C.
> > +
> > +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can
> > test for
> > +   its presence using
> 
> Can the param "output_ident" be NULL?  It isn't checked for NULL in
> the
> patch's implementation of gcc_jit_context_set_output_ident, and
> recording::output_ident's constructor does check for it being NULL...
> 
> > +
> > +   .. code-block:: c
> > +
> > +      #ifdef LIBGCCJIT_HAVE_gcc_jit_context_set_output_ident
> 
> > diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
> > index dddd537f3b1..243a9fdf972 100644
> > --- a/gcc/jit/jit-playback.cc
> > +++ b/gcc/jit/jit-playback.cc
> > @@ -319,6 +319,13 @@ get_type (enum gcc_jit_types type_)
> >    return new type (type_node);
> >  }
> >  
> > +void
> > +playback::context::
> > +set_output_ident (const char* ident)
> > +{
> > +  targetm.asm_out.output_ident (ident);
> > +}
> > +
> 
> ...but looking at varasm.cc's default_asm_output_ident_directive it
> looks like the param must be non-NULL.
> 
> So this should either be conditionalized here to:
> 
>   if (ident)
>     targetm.asm_out.output_ident (ident);
> 
> or else we should ensure that "ident" is non-NULL at the API boundary
> and document this.

Ok, updated the patch to do this at the API boundary.

> 
> My guess is that it doesn't make sense to have a NULL ident, so we
> should go with the latter approach.
> 
> Can you have more than one #ident directive?  Presumably each one
> just
> adds another line to the generated asm, right?

Yes.

> 
> [...snip...]
> 
> > @@ -2185,6 +2192,52 @@ recording::string::write_reproducer
> > (reproducer &)
> >    /* Empty.  */
> >  }
> >  
> > +/* The implementation of class gcc::jit::recording::output_ident. 
> > */
> > +
> > +/* Constructor for gcc::jit::recording::output_ident, allocating a
> > +   copy of the given text using new char[].  */
> > +
> > +recording::output_ident::output_ident (context *ctxt, const char
> > *ident)
> > +: memento (ctxt)
> > +{
> > +  m_ident = ident ? xstrdup (ident) : NULL;
> > +}
> > +
> > +/* Destructor for gcc::jit::recording::output_ident.  */
> > +
> > +recording::output_ident::~output_ident ()
> > +{
> > +  delete[] m_ident;
> 
> m_ident is allocated above using xstrdup, so it must be cleaned up
> with
> "free"; I don't think it's safe to use "delete[]" here.
> 
> [...snip...]
> 
> > +/* Implementation of recording::memento::write_reproducer for
> > output_ident.  */
> > +
> > +void
> > +recording::output_ident::write_reproducer (reproducer &r)
> > +{
> > +  r.write ("  gcc_jit_context_set_output_ident (%s, \"%s\");",
> > +	   r.get_identifier (get_context ()),
> > +	   m_ident);
> 
> It isn't safe on all implementations to use %s with m_ident being
> NULL.

Now, m_ident is non-NULL.

> 
> [...snip...]
> 
> Thanks again for the patch; hope this is constructive
> Dave
> 


[-- Attachment #2: 0001-libgccjit-Add-support-for-setting-the-comment-ident.patch --]
[-- Type: text/x-patch, Size: 10252 bytes --]

From 1e98faa4cc489641289cb8d6634e24efaa3d36a2 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Fri, 27 Oct 2023 17:36:03 -0400
Subject: [PATCH] libgccjit: Add support for setting the comment ident

gcc/jit/ChangeLog:

	* docs/topics/compatibility.rst (LIBGCCJIT_ABI_28): New ABI tag.
	* docs/topics/contexts.rst: Document gcc_jit_context_set_output_ident.
	* jit-playback.cc (set_output_ident): New method.
	* jit-playback.h (set_output_ident): New method.
	* jit-recording.cc (recording::context::set_output_ident,
	recording::output_ident::output_ident,
	recording::output_ident::~output_ident,
	recording::output_ident::replay_into,
	recording::output_ident::make_debug_string,
	recording::output_ident::write_reproducer): New methods.
	* jit-recording.h (class output_ident): New class.
	* libgccjit.cc (gcc_jit_context_set_output_ident): New function.
	* libgccjit.h (gcc_jit_context_set_output_ident): New function.
	* libgccjit.map: New function.

gcc/testsuite/ChangeLog:

	* jit.dg/all-non-failing-tests.h: New test.
	* jit.dg/test-output-ident.c: New test.
---
 gcc/jit/docs/topics/compatibility.rst        |  7 +++
 gcc/jit/docs/topics/contexts.rst             | 29 +++++++++++
 gcc/jit/jit-playback.cc                      |  7 +++
 gcc/jit/jit-playback.h                       |  3 ++
 gcc/jit/jit-recording.cc                     | 53 ++++++++++++++++++++
 gcc/jit/jit-recording.h                      | 22 ++++++++
 gcc/jit/libgccjit.cc                         | 16 ++++++
 gcc/jit/libgccjit.h                          |  6 +++
 gcc/jit/libgccjit.map                        |  5 ++
 gcc/testsuite/jit.dg/all-non-failing-tests.h |  3 ++
 gcc/testsuite/jit.dg/test-output-ident.c     | 23 +++++++++
 11 files changed, 174 insertions(+)
 create mode 100644 gcc/testsuite/jit.dg/test-output-ident.c

diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index 9cfb054f653..d27ec313fa4 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -397,3 +397,10 @@ on functions and variables:
 --------------------
 ``LIBGCCJIT_ABI_27`` covers the addition of
 :func:`gcc_jit_context_new_sizeof`
+
+.. _LIBGCCJIT_ABI_28:
+
+``LIBGCCJIT_ABI_28``
+--------------------
+``LIBGCCJIT_ABI_28`` covers the addition of
+:func:`gcc_jit_context_set_output_ident`
diff --git a/gcc/jit/docs/topics/contexts.rst b/gcc/jit/docs/topics/contexts.rst
index 10a0e50f9f6..839c77eded0 100644
--- a/gcc/jit/docs/topics/contexts.rst
+++ b/gcc/jit/docs/topics/contexts.rst
@@ -599,3 +599,32 @@ Additional command-line options
    .. code-block:: c
 
       #ifdef LIBGCCJIT_HAVE_gcc_jit_context_add_driver_option
+
+Output options
+**************
+
+.. function:: void gcc_jit_context_set_output_ident (gcc_jit_context *ctxt,\
+                                                     const char* output_ident)
+
+   Set the identifier to write in the .comment section of the output file to
+   ``output_ident``.
+
+   The parameter ``output_ident`` must be non-NULL.
+
+   This only works on some target, as you can see here:
+   https://gcc.gnu.org/onlinedocs/cpp/Other-Directives.html
+
+   Analogous to:
+
+   .. code-block:: c
+
+      #ident "My comment"
+
+   in C.
+
+   This entrypoint was added in :ref:`LIBGCCJIT_ABI_28`; you can test for
+   its presence using
+
+   .. code-block:: c
+
+      #ifdef LIBGCCJIT_HAVE_gcc_jit_context_set_output_ident
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index 6baa838af10..e857c373ade 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -320,6 +320,13 @@ get_type (enum gcc_jit_types type_)
   return new type (type_node);
 }
 
+void
+playback::context::
+set_output_ident (const char* ident)
+{
+  targetm.asm_out.output_ident (ident);
+}
+
 /* Construct a playback::type instance (wrapping a tree) for the given
    array type.  */
 
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index aa6a086613c..837836d210e 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -77,6 +77,9 @@ public:
   type *
   get_type (enum gcc_jit_types type);
 
+  void
+  set_output_ident (const char* ident);
+
   type *
   new_array_type (location *loc,
 		  type *element_type,
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index 68a2e860c1f..90c25ea6d32 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -1362,6 +1362,13 @@ recording::context::set_str_option (enum gcc_jit_str_option opt,
   log_str_option (opt);
 }
 
+void
+recording::context::set_output_ident (const char *ident)
+{
+  recording::output_ident *memento = new output_ident (this, ident);
+  record (memento);
+}
+
 /* Set the given integer option for this context, or add an error if
    it's not recognized.
 
@@ -2201,6 +2208,52 @@ recording::string::write_reproducer (reproducer &)
   /* Empty.  */
 }
 
+/* The implementation of class gcc::jit::recording::output_ident.  */
+
+/* Constructor for gcc::jit::recording::output_ident, allocating a
+   copy of the given text using new char[].  */
+
+recording::output_ident::output_ident (context *ctxt, const char *ident)
+: memento (ctxt)
+{
+  m_ident = ident ? xstrdup (ident) : NULL;
+}
+
+/* Destructor for gcc::jit::recording::output_ident.  */
+
+recording::output_ident::~output_ident ()
+{
+  free (m_ident);
+}
+
+/* Implementation of pure virtual hook recording::memento::replay_into
+   for recording::output_ident.  */
+
+void
+recording::output_ident::replay_into (replayer *r)
+{
+  r->set_output_ident (m_ident);
+}
+
+/* Implementation of recording::memento::make_debug_string for output_ident.  */
+
+recording::string *
+recording::output_ident::make_debug_string ()
+{
+  return m_ctxt->new_string (m_ident);
+}
+
+/* Implementation of recording::memento::write_reproducer for output_ident.  */
+
+void
+recording::output_ident::write_reproducer (reproducer &r)
+{
+  r.write ("  gcc_jit_context_set_output_ident (%s, \"%s\");",
+	   r.get_identifier (get_context ()),
+	   m_ident);
+}
+
+
 /* The implementation of class gcc::jit::recording::location.  */
 
 /* Implementation of recording::memento::replay_into for locations.
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index d8d16f4fe29..cf44bd0912b 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -231,6 +231,9 @@ public:
   set_str_option (enum gcc_jit_str_option opt,
 		  const char *value);
 
+  void
+  set_output_ident (const char *output_ident);
+
   void
   set_int_option (enum gcc_jit_int_option opt,
 		  int value);
@@ -470,6 +473,25 @@ private:
   bool m_escaped;
 };
 
+class output_ident : public memento
+{
+public:
+  output_ident (context *ctxt, const char *text);
+  ~output_ident ();
+
+  void replay_into (replayer *) final override;
+
+  output_ident (const output_ident&) = delete;
+  output_ident& operator= (const output_ident&) = delete;
+
+private:
+  string * make_debug_string () final override;
+  void write_reproducer (reproducer &r) final override;
+
+private:
+  const char *m_ident;
+};
+
 class location : public memento
 {
 public:
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index f40a9781405..9ebdec64676 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -3697,6 +3697,22 @@ gcc_jit_context_compile_to_file (gcc_jit_context *ctxt,
   ctxt->compile_to_file (output_kind, output_path);
 }
 
+/* Public entrypoint.  See description in libgccjit.h.
+
+   After error-checking, the real work is done by the
+   gcc::jit::recording::context::set_str_option method in
+   jit-recording.cc.  */
+
+void
+gcc_jit_context_set_output_ident (gcc_jit_context *ctxt,
+				  const char* output_ident)
+{
+  RETURN_IF_FAIL (ctxt, NULL, NULL, "NULL context");
+  RETURN_VAL_IF_FAIL (output_ident, 0, NULL, NULL, "NULL output_ident");
+  JIT_LOG_FUNC (ctxt->get_logger ());
+
+  ctxt->set_output_ident (output_ident);
+}
 
 /* Public entrypoint.  See description in libgccjit.h.
 
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 74e847b2dec..47898eb3fc0 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -2066,6 +2066,12 @@ gcc_jit_lvalue_add_string_attribute (gcc_jit_lvalue *variable,
 				     enum gcc_jit_variable_attribute attribute,
 				     const char* value);
 
+extern void
+gcc_jit_context_set_output_ident (gcc_jit_context *ctxt,
+				  const char* output_ident);
+
+#define LIBGCCJIT_HAVE_gcc_jit_context_set_output_ident
+
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 99aa5970be1..4c37485536c 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -289,3 +289,8 @@ LIBGCCJIT_ABI_27 {
   global:
     gcc_jit_context_new_sizeof;
 } LIBGCCJIT_ABI_26;
+
+LIBGCCJIT_ABI_28 {
+  global:
+    gcc_jit_context_set_output_ident;
+} LIBGCCJIT_ABI_27;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 14a0a321550..36ac826452e 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -266,6 +266,9 @@
 #undef create_code
 #undef verify_code
 
+/* test-output-ident.c: This can't be in the testcases array as it
+   is target-specific.  */
+
 /* test-quadratic.c */
 #define create_code create_code_quadratic
 #define verify_code verify_code_quadratic
diff --git a/gcc/testsuite/jit.dg/test-output-ident.c b/gcc/testsuite/jit.dg/test-output-ident.c
new file mode 100644
index 00000000000..db60101aca3
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-output-ident.c
@@ -0,0 +1,23 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME  "output-of-test-output-ident.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Let's try to inject the equivalent of:
+     #ident "My comment"
+  */
+  gcc_jit_context_set_output_ident (ctxt, "My comment");
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output ".ident	\"My comment\"" } } */
-- 
2.43.0