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
>