libgccjit: Add support for setting the comment ident
Checks
Commit Message
Hi.
This patch adds support for setting the comment ident (analogous to
#ident "comment" in C).
Thanks for the review.
Comments
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?
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.
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?
[...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.
[...snip...]
Thanks again for the patch; hope this is constructive
Dave
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
>
From 1af4e77540001cce8c30e86040c1da785e435810 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_26): 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 | 22 ++++++++
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 | 15 ++++++
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, 166 insertions(+)
create mode 100644 gcc/testsuite/jit.dg/test-output-ident.c
@@ -378,3 +378,10 @@ alignment of a variable:
--------------------
``LIBGCCJIT_ABI_25`` covers the addition of
:func:`gcc_jit_type_get_restrict`
+
+.. _LIBGCCJIT_ABI_26:
+
+``LIBGCCJIT_ABI_26``
+--------------------
+``LIBGCCJIT_ABI_26`` covers the addition of
+:func:`gcc_jit_context_set_output_ident`
@@ -599,3 +599,25 @@ 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``. Analogous to:
+
+ .. code-block:: c
+
+ #ident "My comment"
+
+ in C.
+
+ This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test for
+ its presence using
+
+ .. code-block:: c
+
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_context_set_output_ident
@@ -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);
+}
+
/* Construct a playback::type instance (wrapping a tree) for the given
array type. */
@@ -66,6 +66,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,
@@ -1346,6 +1346,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.
@@ -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;
+}
+
+/* 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.
@@ -224,6 +224,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);
@@ -463,6 +466,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:
@@ -3679,6 +3679,21 @@ 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");
+ JIT_LOG_FUNC (ctxt->get_logger ());
+
+ ctxt->set_output_ident (output_ident);
+}
/* Public entrypoint. See description in libgccjit.h.
@@ -1999,6 +1999,12 @@ gcc_jit_vector_type_get_element_type (gcc_jit_vector_type *vector_type);
extern gcc_jit_type *
gcc_jit_type_unqualified (gcc_jit_type *type);
+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 */
@@ -276,3 +276,8 @@ LIBGCCJIT_ABI_25 {
global:
gcc_jit_type_get_restrict;
} LIBGCCJIT_ABI_24;
+
+LIBGCCJIT_ABI_26 {
+ global:
+ gcc_jit_context_set_output_ident;
+} LIBGCCJIT_ABI_25;
@@ -254,6 +254,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
new file mode 100644
@@ -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