Re: [PATCH] Extract a common logger from jit and analyzer frameworks

[David Malcolm <dmalcolm@redhat.com>]

On Fri, 2021-03-05 at 10:58 +0000, Philip Herron wrote:
> On 04/03/2021 16:45, David Malcolm wrote:
> > On Thu, 2021-03-04 at 16:17 +0000, Philip Herron wrote:
> > > In development of the Rust FE logging is useful in debugging.
> > > Instead
> > > of
> > > rolling our own logger it became clear the loggers part of
> > > analyzer
> > > and jit
> > > projects could be extracted and made generic so they could be
> > > reused
> > > in
> > > other projects.

[...]

> > [...]
> > 
> > > @@ -144,19 +152,27 @@ logger::log_partial (const char *fmt, ...)
> > >  void
> > >  logger::log_va_partial (const char *fmt, va_list *ap)
> > >  {
> > > -  text_info text;
> > > -  text.format_spec = fmt;
> > > -  text.args_ptr = ap;
> > > -  text.err_no = 0;
> > > -  pp_format (m_pp, &text);
> > > -  pp_output_formatted_text (m_pp);
> > I think there's an issue here: what format codes are accepted by
> > this
> > function?
> > 
> > > +  if (!has_pretty_printer ())
> > > +    vfprintf (m_f_out, fmt, *ap);
> > ...here we're formatting using vfprintf...
> > 
> > > +  else
> > > +    {
> > > +      text_info text;
> > > +      text.format_spec = fmt;
> > > +      text.args_ptr = ap;
> > > +      text.err_no = 0;
> > > +      pp_format (m_pp, &text);
> > ...whereas here we're formatting using pp_format, which has
> > different
> > conventions.
> > 
> > The jit implementation decls have e.g.:
> >    GNU_PRINTF(2, 3);
> > whereas the analyzer decls have e.g.:
> >    ATTRIBUTE_GCC_DIAG(2, 3);
> > 
> > I'm not quite sure how to square this circle - templates?  Gah.
> > 
> > I guess the analyzer implementation drifted away from the jit
> > implementation (the analyzer code was originally copied from the
> > jit
> > code).  Sorry about that.
> > 
> > [...]
> > 
> > Hope this is constructive
> > Dave
> 
> Hi David,
> 
> Thanks for the great feedback and for reviewing my earlier attempts.
> It
> seems as though there are really two distinct loggers one with a
> pretty
> printer and one without. Maybe there is a case for creating
> BaseLogger
> which is using GNU_PRINTF and vfprintf etc. Then a
> PrettyPrinterLogger
> based on the BaseLogger but allows for the extensions and the pretty
> printer I think could solve this.

Nit: we don't use CamelCase in GCC [1]

>  Though at that point maybe it is
> getting too complex. 

Agreed, that would be too complicated.

> For gccrs I am interested in the logger in the
> analyzer since it can give such detailed output on the trees

FWIW, I'm not quite sure what you mean by the above.

>  which could
> be useful so maybe just extracting that is the right thing to do for
> GCC
> projects.
> 
> What do you think?

I don't think the jit logger exposes the formatted printing API to end-
users, so at some point it ought to be possible to port the jit logger
from vfprintf to pretty_printer.

How's this for a plan? - for the Rust FE you extract just the analyzer
logging, and at some later point I can port the jit logging over to the
shared gccrs/analyzer logging (so that the Rust FE project doesn't need
to depend on the jit porting being done).  Obviously it would be better
to have just one hierarchical logging framework rather than two, but at
least this way we don't have three, and it gives us a path to getting
down to a single one, and hopefully is sufficiently non-controversial
to not make it harder to (eventually) get the Rust FE merged into
mainline gcc.

Thoughts?

Hope this sounds sane
Dave

[1] except in template declarations, for template arguments