Message ID | 0c37a2a72b2e3a5f85844d27f8d59c21d10ca7f6.camel@zoho.com |
---|---|
State | New |
Headers | show |
Series | libgccjit: Add support for setting the comment ident | expand |
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 diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst index ebede440ee4..c4de996506b 100644 --- a/gcc/jit/docs/topics/compatibility.rst +++ b/gcc/jit/docs/topics/compatibility.rst @@ -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` diff --git a/gcc/jit/docs/topics/contexts.rst b/gcc/jit/docs/topics/contexts.rst index b22eb2aa983..c51cf5a82ea 100644 --- a/gcc/jit/docs/topics/contexts.rst +++ b/gcc/jit/docs/topics/contexts.rst @@ -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 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); +} + /* 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 b0166f8f6ce..c6ed15517e9 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -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, diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc index 9b5b8005ebe..d86616f45ef 100644 --- a/gcc/jit/jit-recording.cc +++ b/gcc/jit/jit-recording.cc @@ -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. diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h index 4a8082991fb..879fdda065d 100644 --- a/gcc/jit/jit-recording.h +++ b/gcc/jit/jit-recording.h @@ -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: diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc index 0451b4df7f9..7eb150fce70 100644 --- a/gcc/jit/libgccjit.cc +++ b/gcc/jit/libgccjit.cc @@ -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. diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h index 749f6c24177..29a057faf3c 100644 --- a/gcc/jit/libgccjit.h +++ b/gcc/jit/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 */ diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map index 8b90a0e2ff3..a40ec61856a 100644 --- a/gcc/jit/libgccjit.map +++ b/gcc/jit/libgccjit.map @@ -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; diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h index e762563f9bd..fd2d80f70f6 100644 --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h @@ -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 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