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
