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

[Philip Herron <philip.herron@embecosm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6588 bytes --]

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.
> Thanks for putting together this patch.
>
>> To test this patch make check-jit was invoked, for analyzer the
>> following
>> flags were used -fanalyzer -fdump-analyzer -fanalyzer-verbosity=4.
> "-fanalyzer-verbosity=" only affects what events appear in diagnostic
> paths from the analyzer; it doesn't directly affect the logging (it
> does indirectly, I suppose, since there are logging messages per-event
> as they are processed)
>
>> gcc/jit/ChangeLog:
>>
>>         * jit-logging.h: has been extracted out to gcc/logging.h
>>
>> gcc/analyzer/ChangeLog:
>>
>>         * analyzer-logging.h: has been extract out to gcc/logging.h
>>
>> gcc/ChangeLog:
>>
>>         * logging.h: added new generic logger based off analyzer's
>> logger
> The ChangeLog entries are incomplete, and so the git hooks will
> probably complain when you try to push this.
>
> Various notes inline below...
>
> [...snip...]
>  
>> diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h
>> index f50ac662516..87193268b10 100644
>> --- a/gcc/analyzer/analyzer.h
>> +++ b/gcc/analyzer/analyzer.h
>> @@ -23,6 +23,16 @@ along with GCC; see the file COPYING3.  If not see
>>  
>>  class graphviz_out;
>>  
>> +namespace gcc {
>> +  class logger;
>> +  class log_scope;
>> +  class log_user;
>> +}
>> +
>> +using gcc::logger;
>> +using gcc::log_scope;
>> +using gcc::log_user;
> Are the "using" decls for log_scope and log_user needed?  I know that
> "logger" is used in a bunch of places, so the "using" decl for that is
> probably useful, but my guess is that the other two are barely used in
> the analyzer code, if at all.
>
> [...]
>
>> diff --git a/gcc/analyzer/analyzer-logging.cc b/gcc/logging.c
>> similarity index 76%
>> rename from gcc/analyzer/analyzer-logging.cc
>> rename to gcc/logging.c
>> index 297319069f8..630a16d19dd 100644
>> --- a/gcc/analyzer/analyzer-logging.cc
>> +++ b/gcc/logging.c
> [...]
>>  /* Implementation of class logger.  */
>>  
>>  /* ctor for logger.  */
>>  
>> -logger::logger (FILE *f_out,
>> -               int, /* flags */
>> -               int /* verbosity */,
>> -               const pretty_printer &reference_pp) :
>> -  m_refcount (0),
>> -  m_f_out (f_out),
>> -  m_indent_level (0),
>> -  m_log_refcount_changes (false),
>> -  m_pp (reference_pp.clone ())
>> +logger::logger (FILE *f_out, int, /* flags */
>> +               int /* verbosity */, const pretty_printer
>> *reference_pp,
>> +               const char *module)
>> +  : m_refcount (0), m_f_out (f_out), m_indent_level (0),
>> +    m_log_refcount_changes (false), m_pp (reference_pp->clone ()),
>> +    m_mod (module)
>>  {
>>    pp_show_color (m_pp) = 0;
>>    pp_buffer (m_pp)->stream = f_out;
>> @@ -61,6 +57,15 @@ logger::logger (FILE *f_out,
>>    print_version (f_out, "", false);
>>  }
>>  
>> +logger::logger (FILE *f_out, int, /* flags */
>> +               int /* verbosity */, const char *module)
>> +  : m_refcount (0), m_f_out (f_out), m_indent_level (0),
>> +    m_log_refcount_changes (false), m_pp (nullptr), m_mod (module)
>> +{
>> +  /* Begin the log by writing the GCC version.  */
>> +  print_version (f_out, "", false);
>> +}
> Both of these pass the empty string to print_version, but the to-be-
> deleted implementation in jit-logging.c passed "JIT: ".  So I think
> this one needs to read something like:
>
>      print_version (f_out, m_mod ? m_mod : "", false);
>
> or somesuch.
>
> I'm probably bikeshedding, but could module/m_mod be renamed to
> prefix/m_prefix?
>
> I think it would be good to have a leading comment for this new ctor.
> In particular, summarizing our discussion from github, something like:
>
> /* logger ctor for use by libgccjit.
>
>    The module param, if non-NULL, is printed as a prefix to all log
>    messages.  This is particularly useful for libgccjit, where the
>    log lines are often intermingled with messages from the program
>    that libgccjit is linked into.  */
>
> or somesuch.
>
>
> [...]
>
>> @@ -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. Though at that point maybe it is
getting too complex. For gccrs I am interested in the logger in the
analyzer since it can give such detailed output on the trees which could
be useful so maybe just extracting that is the right thing to do for GCC
projects.

What do you think?

Thanks

--Phil


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]