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
