* [PATCH] Extract a common logger from jit and analyzer frameworks
@ 2021-03-04 16:17 Philip Herron
2021-03-04 16:45 ` David Malcolm
0 siblings, 1 reply; 5+ messages in thread
From: Philip Herron @ 2021-03-04 16:17 UTC (permalink / raw)
To: gcc-patches; +Cc: Philip Herron
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.
To test this patch make check-jit was invoked, for analyzer the following
flags were used -fanalyzer -fdump-analyzer -fanalyzer-verbosity=4.
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
---
gcc/Makefile.in | 3 +-
gcc/analyzer/analysis-plan.cc | 2 +-
gcc/analyzer/analyzer.h | 11 +-
gcc/analyzer/checker-path.cc | 2 +-
gcc/analyzer/complexity.cc | 2 +-
gcc/analyzer/diagnostic-manager.cc | 2 +-
gcc/analyzer/engine.cc | 4 +-
gcc/analyzer/pending-diagnostic.cc | 2 +-
gcc/analyzer/program-point.cc | 2 +-
gcc/analyzer/program-state.cc | 2 +-
gcc/analyzer/region-model-impl-calls.cc | 2 +-
gcc/analyzer/region-model-manager.cc | 2 +-
gcc/analyzer/region-model-reachability.cc | 2 +-
gcc/analyzer/region-model.cc | 2 +-
gcc/analyzer/region.cc | 2 +-
gcc/analyzer/sm-file.cc | 2 +-
gcc/analyzer/sm-malloc.cc | 2 +-
gcc/analyzer/sm-pattern-test.cc | 2 +-
gcc/analyzer/sm-sensitive.cc | 2 +-
gcc/analyzer/sm-signal.cc | 2 +-
gcc/analyzer/sm-taint.cc | 2 +-
gcc/analyzer/sm.cc | 2 +-
gcc/analyzer/state-purge.cc | 2 +-
gcc/analyzer/store.cc | 2 +-
gcc/analyzer/supergraph.cc | 2 +-
gcc/analyzer/supergraph.h | 8 +-
gcc/analyzer/svalue.cc | 2 +-
gcc/jit/Make-lang.in | 1 -
gcc/jit/jit-common.h | 3 +-
gcc/jit/jit-logging.c | 171 ------------------
gcc/jit/jit-logging.h | 162 +----------------
gcc/jit/libgccjit.c | 4 +-
.../analyzer-logging.cc => logging.c} | 75 +++++---
.../analyzer-logging.h => logging.h} | 103 ++++++-----
34 files changed, 145 insertions(+), 446 deletions(-)
delete mode 100644 gcc/jit/jit-logging.c
rename gcc/{analyzer/analyzer-logging.cc => logging.c} (76%)
rename gcc/{analyzer/analyzer-logging.h => logging.h} (69%)
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index a63c5d9cab6..408ef6e3f0b 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1244,7 +1244,6 @@ C_COMMON_OBJS = c-family/c-common.o c-family/c-cppbuiltin.o c-family/c-dump.o \
ANALYZER_OBJS = \
analyzer/analysis-plan.o \
analyzer/analyzer.o \
- analyzer/analyzer-logging.o \
analyzer/analyzer-pass.o \
analyzer/analyzer-selftests.o \
analyzer/bar-chart.o \
@@ -1710,7 +1709,7 @@ OBJS-libcommon = diagnostic.o diagnostic-color.o diagnostic-show-locus.o \
pretty-print.o intl.o \
sbitmap.o \
vec.o input.o version.o hash-table.o ggc-none.o memory-block.o \
- selftest.o selftest-diagnostic.o sort.o
+ selftest.o selftest-diagnostic.o sort.o logging.o
# Objects in libcommon-target.a, used by drivers and by the core
# compiler and containing target-dependent code.
diff --git a/gcc/analyzer/analysis-plan.cc b/gcc/analyzer/analysis-plan.cc
index 7dfc48e9c3e..40d325976ca 100644
--- a/gcc/analyzer/analysis-plan.cc
+++ b/gcc/analyzer/analysis-plan.cc
@@ -30,7 +30,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-core.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/analysis-plan.h"
#include "ordered-hash-map.h"
#include "options.h"
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;
+
namespace ana {
/* Forward decls of common types, with indentation to show inheritance. */
@@ -98,7 +108,6 @@ class rewind_info_t;
class engine;
class state_machine;
-class logger;
class visitor;
/* Forward decls of functions. */
diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-path.cc
index e6e3ec18688..ca138336fd3 100644
--- a/gcc/analyzer/checker-path.cc
+++ b/gcc/analyzer/checker-path.cc
@@ -40,7 +40,7 @@ along with GCC; see the file COPYING3. If not see
#include "shortest-paths.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "sbitmap.h"
#include "bitmap.h"
diff --git a/gcc/analyzer/complexity.cc b/gcc/analyzer/complexity.cc
index ece4272ff6e..2b374eeccd3 100644
--- a/gcc/analyzer/complexity.cc
+++ b/gcc/analyzer/complexity.cc
@@ -44,7 +44,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "options.h"
#include "cgraph.h"
#include "cfg.h"
diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc
index 7f20841768b..46a39a5d7fe 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -39,7 +39,7 @@ along with GCC; see the file COPYING3. If not see
#include "ordered-hash-map.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "analyzer/pending-diagnostic.h"
#include "analyzer/diagnostic-manager.h"
diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 5339ea39712..d887dc884fd 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -39,7 +39,7 @@ along with GCC; see the file COPYING3. If not see
#include "selftest.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/call-string.h"
#include "analyzer/program-point.h"
#include "analyzer/store.h"
@@ -4820,7 +4820,7 @@ run_checkers ()
log_user the_logger (NULL);
if (dump_fout)
the_logger.set_logger (new logger (dump_fout, 0, 0,
- *global_dc->printer));
+ global_dc->printer));
LOG_SCOPE (the_logger.get_logger ());
impl_run_checkers (the_logger.get_logger ());
diff --git a/gcc/analyzer/pending-diagnostic.cc b/gcc/analyzer/pending-diagnostic.cc
index adff25130fd..855ca7dece1 100644
--- a/gcc/analyzer/pending-diagnostic.cc
+++ b/gcc/analyzer/pending-diagnostic.cc
@@ -28,7 +28,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-event-id.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "diagnostic-event-id.h"
#include "analyzer/sm.h"
diff --git a/gcc/analyzer/program-point.cc b/gcc/analyzer/program-point.cc
index d8cfc61975e..e8c22388243 100644
--- a/gcc/analyzer/program-point.cc
+++ b/gcc/analyzer/program-point.cc
@@ -36,7 +36,7 @@ along with GCC; see the file COPYING3. If not see
#include "gimple-iterator.h"
#include "digraph.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/supergraph.h"
#include "analyzer/program-point.h"
#include "sbitmap.h"
diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc
index e427fff59d6..d5c521adcec 100644
--- a/gcc/analyzer/program-state.cc
+++ b/gcc/analyzer/program-state.cc
@@ -27,7 +27,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "sbitmap.h"
#include "bitmap.h"
diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
index f83c12b5cb7..7e0ff1bf257 100644
--- a/gcc/analyzer/region-model-impl-calls.cc
+++ b/gcc/analyzer/region-model-impl-calls.cc
@@ -44,7 +44,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "ordered-hash-map.h"
#include "options.h"
#include "cgraph.h"
diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc
index dfd2413e914..fca4c8179d7 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -44,7 +44,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "ordered-hash-map.h"
#include "options.h"
#include "cgraph.h"
diff --git a/gcc/analyzer/region-model-reachability.cc b/gcc/analyzer/region-model-reachability.cc
index 087185b4e45..79217339acc 100644
--- a/gcc/analyzer/region-model-reachability.cc
+++ b/gcc/analyzer/region-model-reachability.cc
@@ -41,7 +41,7 @@ along with GCC; see the file COPYING3. If not see
#include "selftest.h"
#include "function.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "ordered-hash-map.h"
#include "options.h"
#include "cgraph.h"
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 96ed549adf6..2b6aac24e91 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -44,7 +44,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "ordered-hash-map.h"
#include "options.h"
#include "cgraph.h"
diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc
index 6db1fc91afd..c63aede909e 100644
--- a/gcc/analyzer/region.cc
+++ b/gcc/analyzer/region.cc
@@ -46,7 +46,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "ordered-hash-map.h"
#include "options.h"
#include "cgraph.h"
diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc
index 48ef4aa2334..61352461d4a 100644
--- a/gcc/analyzer/sm-file.cc
+++ b/gcc/analyzer/sm-file.cc
@@ -32,7 +32,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-event-id.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "analyzer/pending-diagnostic.h"
#include "analyzer/function-set.h"
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index ef250c80915..d3f202da5b1 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -33,7 +33,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-event-id.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "analyzer/pending-diagnostic.h"
#include "tristate.h"
diff --git a/gcc/analyzer/sm-pattern-test.cc b/gcc/analyzer/sm-pattern-test.cc
index 43b847559f8..574df06c48a 100644
--- a/gcc/analyzer/sm-pattern-test.cc
+++ b/gcc/analyzer/sm-pattern-test.cc
@@ -34,7 +34,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-event-id.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "analyzer/pending-diagnostic.h"
diff --git a/gcc/analyzer/sm-sensitive.cc b/gcc/analyzer/sm-sensitive.cc
index 95172f08922..f8f1eca5911 100644
--- a/gcc/analyzer/sm-sensitive.cc
+++ b/gcc/analyzer/sm-sensitive.cc
@@ -34,7 +34,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-event-id.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "analyzer/pending-diagnostic.h"
diff --git a/gcc/analyzer/sm-signal.cc b/gcc/analyzer/sm-signal.cc
index d7e7e7cab04..83a2ef9e8de 100644
--- a/gcc/analyzer/sm-signal.cc
+++ b/gcc/analyzer/sm-signal.cc
@@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-event-id.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "analyzer/pending-diagnostic.h"
#include "sbitmap.h"
diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index 2b2792e5edb..c6e5330869a 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -34,7 +34,7 @@ along with GCC; see the file COPYING3. If not see
#include "json.h"
#include "analyzer/analyzer.h"
#include "diagnostic-event-id.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#include "analyzer/pending-diagnostic.h"
diff --git a/gcc/analyzer/sm.cc b/gcc/analyzer/sm.cc
index 2d227dd1be0..9b5de4345cc 100644
--- a/gcc/analyzer/sm.cc
+++ b/gcc/analyzer/sm.cc
@@ -33,7 +33,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-diagnostic.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/sm.h"
#if ENABLE_ANALYZER
diff --git a/gcc/analyzer/state-purge.cc b/gcc/analyzer/state-purge.cc
index 70a09ed581f..47c8b7ee975 100644
--- a/gcc/analyzer/state-purge.cc
+++ b/gcc/analyzer/state-purge.cc
@@ -47,7 +47,7 @@ along with GCC; see the file COPYING3. If not see
#include "cgraph.h"
#include "analyzer/supergraph.h"
#include "analyzer/program-point.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "analyzer/state-purge.h"
#if ENABLE_ANALYZER
diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
index 53b6e21aa75..c2ab1fc9bd6 100644
--- a/gcc/analyzer/store.cc
+++ b/gcc/analyzer/store.cc
@@ -44,7 +44,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "ordered-hash-map.h"
#include "options.h"
#include "cgraph.h"
diff --git a/gcc/analyzer/supergraph.cc b/gcc/analyzer/supergraph.cc
index 4b934568db6..eaa965901d7 100644
--- a/gcc/analyzer/supergraph.cc
+++ b/gcc/analyzer/supergraph.cc
@@ -51,7 +51,7 @@ along with GCC; see the file COPYING3. If not see
#include "cfg.h"
#include "digraph.h"
#include "analyzer/supergraph.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#if ENABLE_ANALYZER
diff --git a/gcc/analyzer/supergraph.h b/gcc/analyzer/supergraph.h
index fc4a753c5a4..b9dcd1df100 100644
--- a/gcc/analyzer/supergraph.h
+++ b/gcc/analyzer/supergraph.h
@@ -21,6 +21,12 @@ along with GCC; see the file COPYING3. If not see
#ifndef GCC_ANALYZER_SUPERGRAPH_H
#define GCC_ANALYZER_SUPERGRAPH_H
+namespace gcc {
+class logger;
+}
+
+using gcc::logger;
+
using namespace ana;
namespace ana {
@@ -38,8 +44,6 @@ class superedge;
class supercluster;
class dot_annotator;
-class logger;
-
/* An enum for discriminating between superedge subclasses. */
enum edge_kind
diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc
index d6305a37b9a..81e93dbde7f 100644
--- a/gcc/analyzer/svalue.cc
+++ b/gcc/analyzer/svalue.cc
@@ -44,7 +44,7 @@ along with GCC; see the file COPYING3. If not see
#include "function.h"
#include "json.h"
#include "analyzer/analyzer.h"
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#include "options.h"
#include "cgraph.h"
#include "cfg.h"
diff --git a/gcc/jit/Make-lang.in b/gcc/jit/Make-lang.in
index f9b0df850bd..1aa7f25a9bb 100644
--- a/gcc/jit/Make-lang.in
+++ b/gcc/jit/Make-lang.in
@@ -89,7 +89,6 @@ jit.serial = $(LIBGCCJIT_FILENAME)
jit_OBJS = attribs.o \
jit/dummy-frontend.o \
jit/libgccjit.o \
- jit/jit-logging.o \
jit/jit-recording.o \
jit/jit-playback.o \
jit/jit-result.o \
diff --git a/gcc/jit/jit-common.h b/gcc/jit/jit-common.h
index f88e6755b00..da94ff4b8e0 100644
--- a/gcc/jit/jit-common.h
+++ b/gcc/jit/jit-common.h
@@ -95,11 +95,12 @@ const int NUM_GCC_JIT_TYPES = GCC_JIT_TYPE_COMPLEX_LONG_DOUBLE + 1;
namespace gcc {
+class logger;
+
namespace jit {
class result;
class dump;
-class logger;
class builtins_manager; // declared within jit-builtins.h
class tempdir;
diff --git a/gcc/jit/jit-logging.c b/gcc/jit/jit-logging.c
deleted file mode 100644
index 9bcf2262749..00000000000
--- a/gcc/jit/jit-logging.c
+++ /dev/null
@@ -1,171 +0,0 @@
-/* Internals of libgccjit: logging
- Copyright (C) 2014-2021 Free Software Foundation, Inc.
- Contributed by David Malcolm <dmalcolm@redhat.com>.
-
-This file is part of GCC.
-
-GCC is free software; you can redistribute it and/or modify it
-under the terms of the GNU General Public License as published by
-the Free Software Foundation; either version 3, or (at your option)
-any later version.
-
-GCC is distributed in the hope that it will be useful, but
-WITHOUT ANY WARRANTY; without even the implied warranty of
-MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
-General Public License for more details.
-
-You should have received a copy of the GNU General Public License
-along with GCC; see the file COPYING3. If not see
-<http://www.gnu.org/licenses/>. */
-
-#include "config.h"
-#include "system.h"
-#include "coretypes.h"
-#include "toplev.h" /* for print_version */
-
-#include "jit-logging.h"
-
-namespace gcc {
-
-namespace jit {
-
-/* Implementation of class gcc::jit::logger. */
-
-/* The constructor for gcc::jit::logger, used by
- gcc_jit_context_set_logfile. */
-
-logger::logger (FILE *f_out,
- int, /* flags */
- int /* verbosity */) :
- m_refcount (0),
- m_f_out (f_out),
- m_indent_level (0),
- m_log_refcount_changes (false)
-{
- /* Begin the log by writing the GCC version. */
- print_version (f_out, "JIT:", false);
-}
-
-/* The destructor for gcc::jit::logger, invoked via
- the decref method when the refcount hits zero.
- Note that we do not close the underlying FILE * (m_f_out). */
-
-logger::~logger ()
-{
- /* This should be the last message emitted. */
- log ("%s", __PRETTY_FUNCTION__);
- gcc_assert (m_refcount == 0);
-}
-
-/* Increment the reference count of the gcc::jit::logger. */
-
-void
-logger::incref (const char *reason)
-{
- m_refcount++;
- if (m_log_refcount_changes)
- log ("%s: reason: %s refcount now %i ",
- __PRETTY_FUNCTION__, reason, m_refcount);
-}
-
-/* Decrement the reference count of the gcc::jit::logger,
- deleting it if nothing is referring to it. */
-
-void
-logger::decref (const char *reason)
-{
- gcc_assert (m_refcount > 0);
- --m_refcount;
- if (m_log_refcount_changes)
- log ("%s: reason: %s refcount now %i",
- __PRETTY_FUNCTION__, reason, m_refcount);
- if (m_refcount == 0)
- delete this;
-}
-
-/* Write a formatted message to the log, by calling the log_va method. */
-
-void
-logger::log (const char *fmt, ...)
-{
- va_list ap;
- va_start (ap, fmt);
- log_va (fmt, ap);
- va_end (ap);
-}
-
-/* Write an indented line to the log file.
-
- We explicitly flush after each line: if something crashes the process,
- we want the logfile/stream to contain the most up-to-date hint about the
- last thing that was happening, without it being hidden in an in-process
- buffer. */
-
-void
-logger::log_va (const char *fmt, va_list ap)
-{
- fprintf (m_f_out, "JIT: ");
- for (int i = 0; i < m_indent_level; i++)
- fputc (' ', m_f_out);
- vfprintf (m_f_out, fmt, ap);
- fprintf (m_f_out, "\n");
- fflush (m_f_out);
-}
-
-/* Record the entry within a particular scope, indenting subsequent
- log lines accordingly. */
-
-void
-logger::enter_scope (const char *scope_name)
-{
- log ("entering: %s", scope_name);
- m_indent_level += 1;
-}
-
-/* Record the exit from a particular scope, restoring the indent level to
- before the scope was entered. */
-
-void
-logger::exit_scope (const char *scope_name)
-{
- if (m_indent_level)
- m_indent_level -= 1;
- else
- log ("(mismatching indentation)");
- log ("exiting: %s", scope_name);
-}
-
-/* Implementation of class gcc::jit::log_user. */
-
-/* The constructor for gcc::jit::log_user. */
-
-log_user::log_user (logger *logger) : m_logger (logger)
-{
- if (m_logger)
- m_logger->incref("log_user ctor");
-}
-
-/* The destructor for gcc::jit::log_user. */
-
-log_user::~log_user ()
-{
- if (m_logger)
- m_logger->decref("log_user dtor");
-}
-
-/* Set the logger for a gcc::jit::log_user, managing the reference counts
- of the old and new logger (either of which might be NULL). */
-
-void
-log_user::set_logger (logger *logger)
-{
- if (logger)
- logger->incref ("log_user::set_logger");
- if (m_logger)
- m_logger->decref ("log_user::set_logger");
- m_logger = logger;
-}
-
-} // namespace gcc::jit
-
-} // namespace gcc
diff --git a/gcc/jit/jit-logging.h b/gcc/jit/jit-logging.h
index 33254c1aa06..1d067d4fa0f 100644
--- a/gcc/jit/jit-logging.h
+++ b/gcc/jit/jit-logging.h
@@ -22,172 +22,16 @@ along with GCC; see the file COPYING3. If not see
#define JIT_LOGGING_H
#include "jit-common.h"
-
-namespace gcc {
-
-namespace jit {
-
-/* A gcc::jit::logger encapsulates a logging stream: a way to send
- lines of pertinent information to a FILE *. */
-
-class logger
-{
- public:
- logger (FILE *f_out, int flags, int verbosity);
- ~logger ();
-
- void incref (const char *reason);
- void decref (const char *reason);
-
- void log (const char *fmt, ...)
- GNU_PRINTF(2, 3);
- void log_va (const char *fmt, va_list ap)
- GNU_PRINTF(2, 0);
-
- void enter_scope (const char *scope_name);
- void exit_scope (const char *scope_name);
-
-private:
- int m_refcount;
- FILE *m_f_out;
- int m_indent_level;
- bool m_log_refcount_changes;
-};
-
-/* The class gcc::jit::log_scope is an RAII-style class intended to make
- it easy to notify a logger about entering and exiting the body of a
- given function. */
-
-class log_scope
-{
-public:
- log_scope (logger *logger, const char *name);
- ~log_scope ();
-
- private:
- logger *m_logger;
- const char *m_name;
-};
-
-/* The constructor for gcc::jit::log_scope.
-
- The normal case is that the logger is NULL, in which case this should
- be largely a no-op.
-
- If we do have a logger, notify it that we're entering the given scope.
- We also need to hold a reference on it, to avoid a use-after-free
- when logging the cleanup of the owner of the logger. */
-
-inline
-log_scope::log_scope (logger *logger, const char *name) :
- m_logger (logger),
- m_name (name)
-{
- if (m_logger)
- {
- m_logger->incref ("log_scope ctor");
- m_logger->enter_scope (m_name);
- }
-}
-
-/* The destructor for gcc::jit::log_scope; essentially the opposite of
- the constructor. */
-
-inline
-log_scope::~log_scope ()
-{
- if (m_logger)
- {
- m_logger->exit_scope (m_name);
- m_logger->decref ("log_scope dtor");
- }
-}
-
-/* A gcc::jit::log_user is something that potentially uses a
- gcc::jit::logger (which could be NULL).
-
- It is the base class for each of:
-
- - class gcc::jit::recording::context
-
- - class gcc::jit::playback::context
-
- - class gcc::jit::tempdir
-
- - class gcc::jit::result
-
- The log_user class keeps the reference-count of a logger up-to-date. */
-
-class log_user
-{
- public:
- log_user (logger *logger);
- ~log_user ();
-
- logger * get_logger () const { return m_logger; }
- void set_logger (logger * logger);
-
- void log (const char *fmt, ...) const
- GNU_PRINTF(2, 3);
-
- void enter_scope (const char *scope_name);
- void exit_scope (const char *scope_name);
-
- private:
- logger *m_logger;
-};
-
-/* A shortcut for calling log from a context/result, handling the common
- case where the underlying logger is NULL via a no-op. */
-
-inline void
-log_user::log (const char *fmt, ...) const
-{
- if (m_logger)
- {
- va_list ap;
- va_start (ap, fmt);
- m_logger->log_va (fmt, ap);
- va_end (ap);
- }
-}
-
-/* A shortcut for recording entry into a scope from a context/result,
- handling the common case where the underlying logger is NULL via
- a no-op. */
-
-inline void
-log_user::enter_scope (const char *scope_name)
-{
- if (m_logger)
- m_logger->enter_scope (scope_name);
-}
-
-/* A shortcut for recording exit from a scope from a context/result,
- handling the common case where the underlying logger is NULL via
- a no-op. */
-
-inline void
-log_user::exit_scope (const char *scope_name)
-{
- if (m_logger)
- m_logger->exit_scope (scope_name);
-}
-
-} // namespace gcc::jit
-
-} // namespace gcc
+#include "logging.h"
/* If the given logger is non-NULL, log entry/exit of this scope to
it, identifying it using __PRETTY_FUNCTION__. */
-#define JIT_LOG_SCOPE(LOGGER) \
- gcc::jit::log_scope s (LOGGER, __PRETTY_FUNCTION__)
+#define JIT_LOG_SCOPE(LOGGER) gcc::log_scope s (LOGGER, __PRETTY_FUNCTION__)
/* If the given logger is non-NULL, log entry/exit of this scope to
it, identifying it using __func__. */
-#define JIT_LOG_FUNC(LOGGER) \
- gcc::jit::log_scope s (LOGGER, __func__)
+#define JIT_LOG_FUNC(LOGGER) gcc::log_scope s (LOGGER, __func__)
#endif /* JIT_LOGGING_H */
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index 0cc650f9810..e4cc5785d37 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -2891,9 +2891,9 @@ gcc_jit_context_set_logfile (gcc_jit_context *ctxt,
RETURN_IF_FAIL ((flags == 0), ctxt, NULL, "flags must be 0 for now");
RETURN_IF_FAIL ((verbosity == 0), ctxt, NULL, "verbosity must be 0 for now");
- gcc::jit::logger *logger;
+ gcc::logger *logger;
if (logfile)
- logger = new gcc::jit::logger (logfile, flags, verbosity);
+ logger = new gcc::logger (logfile, flags, verbosity, "JIT");
else
logger = NULL;
ctxt->set_logger (logger);
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
@@ -1,4 +1,4 @@
-/* Hierarchical log messages for the analyzer.
+/* Hierarchical log messages
Copyright (C) 2014-2021 Free Software Foundation, Inc.
Contributed by David Malcolm <dmalcolm@redhat.com>.
@@ -21,12 +21,11 @@ along with GCC; see the file COPYING3. If not see
#include "config.h"
#include "system.h"
#include "coretypes.h"
-#include "toplev.h" /* for print_version */
+#include "toplev.h" /* for print_version */
#include "pretty-print.h" /* for print_version */
#include "diagnostic.h"
#include "tree-diagnostic.h"
-
-#include "analyzer/analyzer-logging.h"
+#include "logging.h"
#if ENABLE_ANALYZER
@@ -34,21 +33,18 @@ along with GCC; see the file COPYING3. If not see
#pragma GCC diagnostic ignored "-Wformat-diag"
#endif
-namespace ana {
+namespace gcc {
/* 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);
+}
+
/* The destructor for logger, invoked via
the decref method when the refcount hits zero.
Note that we do not close the underlying FILE * (m_f_out). */
@@ -80,8 +85,8 @@ logger::incref (const char *reason)
{
m_refcount++;
if (m_log_refcount_changes)
- log ("%s: reason: %s refcount now %i ",
- __PRETTY_FUNCTION__, reason, m_refcount);
+ log ("%s: reason: %s refcount now %i ", __PRETTY_FUNCTION__, reason,
+ m_refcount);
}
/* Decrement the reference count of the logger,
@@ -93,8 +98,8 @@ logger::decref (const char *reason)
gcc_assert (m_refcount > 0);
--m_refcount;
if (m_log_refcount_changes)
- log ("%s: reason: %s refcount now %i",
- __PRETTY_FUNCTION__, reason, m_refcount);
+ log ("%s: reason: %s refcount now %i", __PRETTY_FUNCTION__, reason,
+ m_refcount);
if (m_refcount == 0)
delete this;
}
@@ -128,6 +133,9 @@ logger::log_va (const char *fmt, va_list *ap)
void
logger::start_log_line ()
{
+ if (m_mod != NULL)
+ fprintf (m_f_out, "%s: ", m_mod);
+
for (int i = 0; i < m_indent_level; i++)
fputc (' ', m_f_out);
}
@@ -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);
+ if (!has_pretty_printer ())
+ vfprintf (m_f_out, fmt, *ap);
+ else
+ {
+ 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);
+ }
}
void
logger::end_log_line ()
{
- pp_flush (m_pp);
- pp_clear_output_area (m_pp);
+ if (has_pretty_printer ())
+ {
+ pp_flush (m_pp);
+ pp_clear_output_area (m_pp);
+ }
fprintf (m_f_out, "\n");
fflush (m_f_out);
}
@@ -182,7 +198,6 @@ logger::enter_scope (const char *scope_name, const char *fmt, va_list *ap)
inc_indent ();
}
-
/* Record the exit from a particular scope, restoring the indent level to
before the scope was entered. */
@@ -203,7 +218,7 @@ logger::exit_scope (const char *scope_name)
log_user::log_user (logger *logger) : m_logger (logger)
{
if (m_logger)
- m_logger->incref("log_user ctor");
+ m_logger->incref ("log_user ctor");
}
/* The destructor for log_user. */
@@ -211,7 +226,7 @@ log_user::log_user (logger *logger) : m_logger (logger)
log_user::~log_user ()
{
if (m_logger)
- m_logger->decref("log_user dtor");
+ m_logger->decref ("log_user dtor");
}
/* Set the logger for a log_user, managing the reference counts
@@ -227,6 +242,6 @@ log_user::set_logger (logger *logger)
m_logger = logger;
}
-} // namespace ana
+} // namespace gcc
#endif /* #if ENABLE_ANALYZER */
diff --git a/gcc/analyzer/analyzer-logging.h b/gcc/logging.h
similarity index 69%
rename from gcc/analyzer/analyzer-logging.h
rename to gcc/logging.h
index 88b5f0a4a3f..b93980bc17f 100644
--- a/gcc/analyzer/analyzer-logging.h
+++ b/gcc/logging.h
@@ -1,4 +1,4 @@
-/* Hierarchical log messages for the analyzer.
+/* GCC Logging
Copyright (C) 2014-2021 Free Software Foundation, Inc.
Contributed by David Malcolm <dmalcolm@redhat.com>.
@@ -18,44 +18,51 @@ You should have received a copy of the GNU General Public License
along with GCC; see the file COPYING3. If not see
<http://www.gnu.org/licenses/>. */
-/* Adapted from jit-logging.h. */
+#ifndef GCC_LOGGING_H
+#define GCC_LOGGING_H
-#ifndef ANALYZER_LOGGING_H
-#define ANALYZER_LOGGING_H
+#include "diagnostic-core.h"
-namespace ana {
+namespace gcc {
-/* A logger encapsulates a logging stream: a way to send
+/* A gcc::logger encapsulates a logging stream: a way to send
lines of pertinent information to a FILE *. */
class logger
{
- public:
- logger (FILE *f_out, int flags, int verbosity, const pretty_printer &reference_pp);
+public:
+ logger (FILE *f_out, int flags, int verbosity, const char *module = NULL);
+
+ logger (FILE *f_out, int flags, int verbosity,
+ const pretty_printer *reference_pp, const char *module = NULL);
+
~logger ();
void incref (const char *reason);
void decref (const char *reason);
- void log (const char *fmt, ...)
- ATTRIBUTE_GCC_DIAG(2, 3);
- void log_va (const char *fmt, va_list *ap)
- ATTRIBUTE_GCC_DIAG(2, 0);
+ void log (const char *fmt, ...) ATTRIBUTE_GCC_DIAG (2, 3);
+ void log_va (const char *fmt, va_list *ap) ATTRIBUTE_GCC_DIAG (2, 0);
void start_log_line ();
- void log_partial (const char *fmt, ...)
- ATTRIBUTE_GCC_DIAG(2, 3);
- void log_va_partial (const char *fmt, va_list *ap)
- ATTRIBUTE_GCC_DIAG(2, 0);
+ void log_partial (const char *fmt, ...) ATTRIBUTE_GCC_DIAG (2, 3);
+ void log_va_partial (const char *fmt, va_list *ap) ATTRIBUTE_GCC_DIAG (2, 0);
void end_log_line ();
void enter_scope (const char *scope_name);
void enter_scope (const char *scope_name, const char *fmt, va_list *ap)
- ATTRIBUTE_GCC_DIAG(3, 0);
+ ATTRIBUTE_GCC_DIAG (3, 0);
void exit_scope (const char *scope_name);
void inc_indent () { m_indent_level++; }
void dec_indent () { m_indent_level--; }
- pretty_printer *get_printer () const { return m_pp; }
+ bool has_pretty_printer () const { return m_pp != nullptr; }
+
+ pretty_printer *get_printer () const
+ {
+ gcc_assert (m_pp != nullptr);
+ return m_pp;
+ }
+
FILE *get_file () const { return m_f_out; }
private:
@@ -66,9 +73,10 @@ private:
int m_indent_level;
bool m_log_refcount_changes;
pretty_printer *m_pp;
+ const char *m_mod;
};
-/* The class log_scope is an RAII-style class intended to make
+/* The class gcc::jit::log_scope is an RAII-style class intended to make
it easy to notify a logger about entering and exiting the body of a
given function. */
@@ -77,10 +85,10 @@ class log_scope
public:
log_scope (logger *logger, const char *name);
log_scope (logger *logger, const char *name, const char *fmt, ...)
- ATTRIBUTE_GCC_DIAG(4, 5);
+ ATTRIBUTE_GCC_DIAG (4, 5);
~log_scope ();
- private:
+private:
DISABLE_COPY_AND_ASSIGN (log_scope);
logger *m_logger;
@@ -96,10 +104,8 @@ public:
We also need to hold a reference on it, to avoid a use-after-free
when logging the cleanup of the owner of the logger. */
-inline
-log_scope::log_scope (logger *logger, const char *name) :
- m_logger (logger),
- m_name (name)
+inline log_scope::log_scope (logger *logger, const char *name)
+ : m_logger (logger), m_name (name)
{
if (m_logger)
{
@@ -108,10 +114,9 @@ log_scope::log_scope (logger *logger, const char *name) :
}
}
-inline
-log_scope::log_scope (logger *logger, const char *name, const char *fmt, ...):
- m_logger (logger),
- m_name (name)
+inline log_scope::log_scope (logger *logger, const char *name, const char *fmt,
+ ...)
+ : m_logger (logger), m_name (name)
{
if (m_logger)
{
@@ -123,12 +128,10 @@ log_scope::log_scope (logger *logger, const char *name, const char *fmt, ...):
}
}
-
/* The destructor for log_scope; essentially the opposite of
the constructor. */
-inline
-log_scope::~log_scope ()
+inline log_scope::~log_scope ()
{
if (m_logger)
{
@@ -143,15 +146,14 @@ log_scope::~log_scope ()
class log_user
{
- public:
+public:
log_user (logger *logger);
~log_user ();
- logger * get_logger () const { return m_logger; }
- void set_logger (logger * logger);
+ logger *get_logger () const { return m_logger; }
+ void set_logger (logger *logger);
- void log (const char *fmt, ...) const
- ATTRIBUTE_GCC_DIAG(2, 3);
+ void log (const char *fmt, ...) const ATTRIBUTE_GCC_DIAG (2, 3);
void start_log_line () const;
void end_log_line () const;
@@ -172,7 +174,7 @@ class log_user
return m_logger->get_file ();
}
- private:
+private:
DISABLE_COPY_AND_ASSIGN (log_user);
logger *m_logger;
@@ -240,27 +242,24 @@ log_user::exit_scope (const char *scope_name)
/* If the given logger is non-NULL, log entry/exit of this scope to
it, identifying it using __PRETTY_FUNCTION__. */
-#define LOG_SCOPE(LOGGER) \
- log_scope s (LOGGER, __PRETTY_FUNCTION__)
+#define LOG_SCOPE(LOGGER) gcc::log_scope s (LOGGER, __PRETTY_FUNCTION__)
/* If the given logger is non-NULL, log entry/exit of this scope to
it, identifying it using __func__. */
-#define LOG_FUNC(LOGGER) \
- log_scope s (LOGGER, __func__)
+#define LOG_FUNC(LOGGER) gcc::log_scope s (LOGGER, __func__)
-#define LOG_FUNC_1(LOGGER, FMT, A0) \
- log_scope s (LOGGER, __func__, FMT, A0)
+#define LOG_FUNC_1(LOGGER, FMT, A0) gcc::log_scope s (LOGGER, __func__, FMT, A0)
-#define LOG_FUNC_2(LOGGER, FMT, A0, A1) \
- log_scope s (LOGGER, __func__, FMT, A0, A1)
+#define LOG_FUNC_2(LOGGER, FMT, A0, A1) \
+ gcc::log_scope s (LOGGER, __func__, FMT, A0, A1)
-#define LOG_FUNC_3(LOGGER, FMT, A0, A1, A2) \
- log_scope s (LOGGER, __func__, FMT, A0, A1, A2)
+#define LOG_FUNC_3(LOGGER, FMT, A0, A1, A2) \
+ gcc::log_scope s (LOGGER, __func__, FMT, A0, A1, A2)
-#define LOG_FUNC_4(LOGGER, FMT, A0, A1, A2, A3) \
- log_scope s (LOGGER, __func__, FMT, A0, A1, A2, A3)
+#define LOG_FUNC_4(LOGGER, FMT, A0, A1, A2, A3) \
+ gcc::log_scope s (LOGGER, __func__, FMT, A0, A1, A2, A3)
-} // namespace ana
+} // namespace gcc
-#endif /* ANALYZER_LOGGING_H */
+#endif /* GCC_LOGGING_H */
--
2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Extract a common logger from jit and analyzer frameworks
2021-03-04 16:17 [PATCH] Extract a common logger from jit and analyzer frameworks Philip Herron
@ 2021-03-04 16:45 ` David Malcolm
2021-03-05 10:58 ` Philip Herron
0 siblings, 1 reply; 5+ messages in thread
From: David Malcolm @ 2021-03-04 16:45 UTC (permalink / raw)
To: Philip Herron, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Extract a common logger from jit and analyzer frameworks
2021-03-04 16:45 ` David Malcolm
@ 2021-03-05 10:58 ` Philip Herron
2021-03-05 14:18 ` David Malcolm
0 siblings, 1 reply; 5+ messages in thread
From: Philip Herron @ 2021-03-05 10:58 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Extract a common logger from jit and analyzer frameworks
2021-03-05 10:58 ` Philip Herron
@ 2021-03-05 14:18 ` David Malcolm
2021-03-05 15:07 ` Philip Herron
0 siblings, 1 reply; 5+ messages in thread
From: David Malcolm @ 2021-03-05 14:18 UTC (permalink / raw)
To: Philip Herron, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Extract a common logger from jit and analyzer frameworks
2021-03-05 14:18 ` David Malcolm
@ 2021-03-05 15:07 ` Philip Herron
0 siblings, 0 replies; 5+ messages in thread
From: Philip Herron @ 2021-03-05 15:07 UTC (permalink / raw)
To: David Malcolm, gcc-patches
[-- Attachment #1.1: Type: text/plain, Size: 4079 bytes --]
On 05/03/2021 14:18, David Malcolm wrote:
> 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
>
Hi David,
Extracting the analyzer logger sounds good to me. I was getting anxious
that this patch was going to affect both the analyzer and jit projects,
so its likely safer to make this an incremental change instead of one
big one, when some care might need to be taken.
Thanks for helping out so much.
--Phil
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-05 15:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 16:17 [PATCH] Extract a common logger from jit and analyzer frameworks Philip Herron
2021-03-04 16:45 ` David Malcolm
2021-03-05 10:58 ` Philip Herron
2021-03-05 14:18 ` David Malcolm
2021-03-05 15:07 ` Philip Herron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).