On Wed, 2024-10-30 at 15:53 +0000, Qing Zhao wrote:
>
>
> > On Oct 30, 2024, at 10:48, David Malcolm <dmalc...@redhat.com>
> > wrote:
> >
> > On Wed, 2024-10-30 at 14:34 +0000, Sam James wrote:
> > > Qing Zhao <qing.z...@oracle.com> writes:
> > >
> > > > Control this with a new option -fdiagnostics-details.
> > > >
[...]
>
> I have a question on the changes to the “warning_at”: (there are a
> lot of such changes for -Warray-bounds and -Wstringop-**)
>
> - warned = warning_at (location, OPT_Warray_bounds_,
> + {
> + rich_location *richloc
> + = build_rich_location_with_diagnostic_path (location,
> stmt);
> + warned = warning_at (richloc, OPT_Warray_bounds_,
>
> The above is the current change.
>
> My concern with this change is:
> even when -fdiagnostics_details is NOT on, the rich_location is
> created.
A rich_location instance is always constructed when emitting
diagnostics; warning_at with a location_t simply makes a rich_location
on the stack.
>
> How much is the additional overhead when using “rich_location *”
> other than “location_t” as the 1st argument of warning_at?
The warning_at overload taking a rich_location * takes a borrowed
pointer to a rich_location; it doesn't take ownership. Hence, as
written, the patch has a memory leak: every call to
build_rich_location_with_diagnostic_path is using "new" to make a new
rich_location instance on the heap, and they aren't being deleted.
>
> Should I control the creation of “rich_location" with the flag
> “flag_diagnostics_details” (Similar as I control the creation of
> “move_history” data structure with the flag
> “flag_diagnostics_details”?
>
> If so, how should I do it? Do you have a suggestion on a clean and
> simply coding here (Sorry for the stupid question on this)
You can probably do all of this on the stack; make a new rich_location
subclass, with something like:
class rich_location_with_details : public gcc_rich_location
{
public:
rich_location_with_details (location_t location, gimple *stmt);
private:
class deferred_move_history_path {
public:
deferred_move_history_path (location_t location, gimple *stmt)
: m_location (location), m_stmt (stmt)
{
}
std::unique_ptr<diagnostic_path>
make_path () const final override;
/* TODO: you'll need to implement this; it will be called on
demand if a diagnostic is acutally emitted for this
rich_location. */
location_t m_location;
gimple *m_stmt;
} m_deferred_move_history_path;
};
rich_location_with_details::
rich_location_with_details (location_t location, gimple *stmt)
: gcc_rich_location (location),
m_deferred_move_history_path (location, stmt)
{
set_path (&m_deferred_move_history_path);
}
using class deferred_diagnostic_path from the attached patch (caveat: I
haven't tried bootstrapping it yet).
With that support subclass, you should be able to do something like
this to make them on the stack:
rich_location_with_details richloc (location, stmt);
warned = warning_at (&richloc, OPT_Warray_bounds_,
"array subscript %E is outside array"
" bounds of %qT", low_sub_org, artype);
and no work will be done for path creation unless and until a
diagnostic is actually emitted for richloc - the richloc ctor will just
initialize the vtable and some location_t/gimple * fields, which ought
to be very cheap for the "warning is disabled" case .
I'll try bootstrapping the attached patch.
Hope this makes sense.
Dave
From a45e3718315ed1c2e2242f76c6af3aa5d646636a Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalc...@redhat.com>
Date: Wed, 30 Oct 2024 14:03:52 -0400
Subject: [PATCH] diagnostics: add class deferred_diagnostic_path
This patch adds a new class deferred_diagnostic_path for
use when creating rich_location instances, to allow deferring
expensive computations until the path is actually used (when
a diagnostic using the rich_location is emitted).
gcc/ChangeLog:
* Makefile.in (OBJS): Add deferred-diagnostic-path.o.
* deferred-diagnostic-path.cc: New file.
* deferred-diagnostic-path.h: New file.
* selftest-diagnostic.cc: Include "diagnostic-format.h".
(test_diagnostic_context::test_diagnostic_context): Turn off
flushing for the output format's printer.
* selftest-run-tests.cc (selftest::run_tests): Call
selftest::deferred_diagnostic_path_cc_tests.
* selftest.h (selftest::deferred_diagnostic_path_cc_tests): New decl.
Signed-off-by: David Malcolm <dmalc...@redhat.com>
---
gcc/Makefile.in | 1 +
gcc/deferred-diagnostic-path.cc | 206 ++++++++++++++++++++++++++++++++
gcc/deferred-diagnostic-path.h | 56 +++++++++
gcc/selftest-diagnostic.cc | 2 +
gcc/selftest-run-tests.cc | 1 +
gcc/selftest.h | 1 +
6 files changed, 267 insertions(+)
create mode 100644 gcc/deferred-diagnostic-path.cc
create mode 100644 gcc/deferred-diagnostic-path.h
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 798d4302fa78..2d93d6451e20 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1428,6 +1428,7 @@ OBJS = \
dce.o \
ddg.o \
debug.o \
+ deferred-diagnostic-path.o \
df-core.o \
df-problems.o \
df-scan.o \
diff --git a/gcc/deferred-diagnostic-path.cc b/gcc/deferred-diagnostic-path.cc
new file mode 100644
index 000000000000..93aaacf01d80
--- /dev/null
+++ b/gcc/deferred-diagnostic-path.cc
@@ -0,0 +1,206 @@
+/* Helper class for deferring path creation until a diagnostic is emitted.
+ Copyright (C) 2019-2024 Free Software Foundation, Inc.
+ Contributed by David Malcolm <dmalc...@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"
+#define INCLUDE_MEMORY
+#define INCLUDE_VECTOR
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "version.h"
+#include "intl.h"
+#include "diagnostic.h"
+#include "deferred-diagnostic-path.h"
+#include "make-unique.h"
+#include "selftest.h"
+#include "selftest-diagnostic.h"
+#include "simple-diagnostic-path.h"
+#include "gcc-rich-location.h"
+
+/* class deferred_diagnostic_path : public diagnostic_path. */
+
+/* Implementation of diagnostic_path vfuncs in terms of a lazily-generated
+ path. */
+
+unsigned
+deferred_diagnostic_path::num_events () const
+{
+ lazily_generate_path ();
+ return m_generated_path->num_events ();
+}
+
+const diagnostic_event &
+deferred_diagnostic_path::get_event (int idx) const
+{
+ lazily_generate_path ();
+ return m_generated_path->get_event (idx);
+}
+
+unsigned
+deferred_diagnostic_path::num_threads () const
+{
+ lazily_generate_path ();
+ return m_generated_path->num_threads ();
+}
+
+const diagnostic_thread &
+deferred_diagnostic_path::get_thread (diagnostic_thread_id_t idx) const
+{
+ lazily_generate_path ();
+ return m_generated_path->get_thread (idx);
+}
+
+bool
+deferred_diagnostic_path::same_function_p (int event_idx_a,
+ int event_idx_b) const
+{
+ lazily_generate_path ();
+ return m_generated_path->same_function_p (event_idx_a, event_idx_b);
+}
+
+void
+deferred_diagnostic_path::lazily_generate_path () const
+{
+ if (!m_generated_path)
+ m_generated_path = make_path ();
+ gcc_assert (m_generated_path != nullptr);
+}
+
+#if CHECKING_P
+
+namespace selftest {
+
+class test_deferred_path : public deferred_diagnostic_path
+{
+public:
+ test_deferred_path (pretty_printer &pp)
+ : m_pp (pp.clone ())
+ {
+ }
+ std::unique_ptr<diagnostic_path> make_path () const final override
+ {
+ tree fntype_void_void
+ = build_function_type_array (void_type_node, 0, NULL);
+ tree fndecl_foo = build_fn_decl ("foo", fntype_void_void);
+ auto path = ::make_unique<simple_diagnostic_path> (m_pp.get ());
+ path->add_event (UNKNOWN_LOCATION, fndecl_foo, 0, "first %qs", "free");
+ path->add_event (UNKNOWN_LOCATION, fndecl_foo, 0, "double %qs", "free");
+ return path;
+ }
+private:
+ std::unique_ptr<pretty_printer> m_pp;
+};
+
+static void
+test_intraprocedural_path (pretty_printer *event_pp)
+{
+ test_deferred_path path (*event_pp);
+ ASSERT_FALSE (path.generated_p ());
+ ASSERT_EQ (path.num_events (), 2);
+ ASSERT_TRUE (path.generated_p ());
+ ASSERT_EQ (path.num_threads (), 1);
+ ASSERT_FALSE (path.interprocedural_p ());
+ ASSERT_STREQ (path.get_event (0).get_desc ().get (), "first `free'");
+ ASSERT_STREQ (path.get_event (1).get_desc ().get (), "double `free'");
+}
+
+/* Implementation of diagnostic_option_manager for which all
+ options are disabled, for use in selftests.
+ Note that this is *not* called for diagnostic_option_id (0), which
+ means "always warn" */
+
+class all_warnings_disabled : public diagnostic_option_manager
+{
+public:
+ int option_enabled_p (diagnostic_option_id) const final override
+ {
+ /* Treat all options as disabled. */
+ return 0;
+ }
+ char *make_option_name (diagnostic_option_id,
+ diagnostic_t,
+ diagnostic_t) const final override
+ {
+ return nullptr;
+ }
+ char *make_option_url (diagnostic_option_id) const final override
+ {
+ return nullptr;
+ }
+};
+
+static void
+test_emission (pretty_printer *event_pp)
+{
+ test_deferred_path path (*event_pp);
+ gcc_rich_location rich_loc (UNKNOWN_LOCATION);
+ rich_loc.set_path (&path);
+ ASSERT_FALSE (path.generated_p ());
+
+ /* Verify that we don't bother generating a path if the warning
+ is skipped. */
+ {
+ diagnostic_option_id option_id (42); // has to be non-zero
+ test_diagnostic_context dc;
+ dc.set_option_manager (::make_unique<all_warnings_disabled> (), 0);
+ bool emitted
+ = dc.emit_diagnostic_with_group (DK_WARNING, rich_loc, nullptr,
+ option_id,
+ "this is a warning");
+ ASSERT_FALSE (emitted);
+ ASSERT_FALSE (path.generated_p ());
+ }
+
+ /* Verify that we *do* generate a path for an error. */
+ {
+ test_diagnostic_context dc;
+ bool emitted
+ = dc.emit_diagnostic_with_group (DK_ERROR, rich_loc, nullptr, 0,
+ "this is a test");
+ ASSERT_TRUE (emitted);
+ ASSERT_TRUE (path.generated_p ());
+ }
+}
+
+/* Run all of the selftests within this file. */
+
+void
+deferred_diagnostic_path_cc_tests ()
+{
+ /* In a few places we use the global dc's printer to determine
+ colorization so ensure this off during the tests. */
+ pretty_printer *global_pp = global_dc->get_reference_printer ();
+ const bool saved_show_color = pp_show_color (global_pp);
+ pp_show_color (global_pp) = false;
+
+ auto_fix_quotes fix_quotes;
+ std::unique_ptr<pretty_printer> event_pp
+ = std::unique_ptr<pretty_printer> (global_pp->clone ());
+
+ test_intraprocedural_path (event_pp.get ());
+ test_emission (event_pp.get ());
+
+ pp_show_color (global_pp) = saved_show_color;
+}
+
+} // namespace selftest
+
+#endif /* #if CHECKING_P */
diff --git a/gcc/deferred-diagnostic-path.h b/gcc/deferred-diagnostic-path.h
new file mode 100644
index 000000000000..b069781377c2
--- /dev/null
+++ b/gcc/deferred-diagnostic-path.h
@@ -0,0 +1,56 @@
+/* Helper class for deferring path creation until a diagnostic is emitted.
+ Copyright (C) 2024 Free Software Foundation, Inc.
+ Contributed by David Malcolm <dmalc...@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/>. */
+
+#ifndef GCC_DEFERRED_DIAGNOSTIC_PATH_H
+#define GCC_DEFERRED_DIAGNOSTIC_PATH_H
+
+#include "diagnostic-path.h"
+
+/* An implementation of diagnostic_path which has a trivial ctor
+ and creates another diagnostic_path the first time the path
+ is queried, deferring to this lazily-created path for all queries.
+ Use this to avoid expensive path creation logic when creating
+ rich_location instances, so that the path-creation can be deferred
+ until the path is actually used by a diagnostic. */
+
+class deferred_diagnostic_path : public diagnostic_path
+{
+ public:
+ virtual ~deferred_diagnostic_path () {}
+
+ unsigned num_events () const final override;
+ const diagnostic_event & get_event (int idx) const final override;
+ unsigned num_threads () const final override;
+ const diagnostic_thread &
+ get_thread (diagnostic_thread_id_t) const final override;
+ bool
+ same_function_p (int event_idx_a,
+ int event_idx_b) const final override;
+
+ bool generated_p () const { return m_generated_path != nullptr; }
+
+ private:
+ void lazily_generate_path () const;
+ virtual std::unique_ptr<diagnostic_path> make_path () const = 0;
+
+ mutable std::unique_ptr<diagnostic_path> m_generated_path;
+};
+
+#endif /* ! GCC_DEFERRED_DIAGNOSTIC_PATH_H */
diff --git a/gcc/selftest-diagnostic.cc b/gcc/selftest-diagnostic.cc
index a9118b55f18f..0c23085b9ec4 100644
--- a/gcc/selftest-diagnostic.cc
+++ b/gcc/selftest-diagnostic.cc
@@ -22,6 +22,7 @@ along with GCC; see the file COPYING3. If not see
#include "system.h"
#include "coretypes.h"
#include "diagnostic.h"
+#include "diagnostic-format.h"
#include "selftest.h"
#include "selftest-diagnostic.h"
@@ -44,6 +45,7 @@ test_diagnostic_context::test_diagnostic_context ()
diagnostic_start_span (this) = start_span_cb;
m_source_printing.min_margin_width = 6;
m_source_printing.max_width = 80;
+ pp_buffer (get_output_format (0).get_printer ())->m_flush_p = false;
}
test_diagnostic_context::~test_diagnostic_context ()
diff --git a/gcc/selftest-run-tests.cc b/gcc/selftest-run-tests.cc
index 6085e334b917..3694ad298f2d 100644
--- a/gcc/selftest-run-tests.cc
+++ b/gcc/selftest-run-tests.cc
@@ -105,6 +105,7 @@ selftest::run_tests ()
tree_cfg_cc_tests ();
diagnostic_path_cc_tests ();
simple_diagnostic_path_cc_tests ();
+ deferred_diagnostic_path_cc_tests ();
attribs_cc_tests ();
opts_diagnostic_cc_tests ();
diff --git a/gcc/selftest.h b/gcc/selftest.h
index 5109128f7f30..85b8a6197d55 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -220,6 +220,7 @@ extern void attribs_cc_tests ();
extern void bitmap_cc_tests ();
extern void cgraph_cc_tests ();
extern void convert_cc_tests ();
+extern void deferred_diagnostic_path_cc_tests ();
extern void diagnostic_color_cc_tests ();
extern void diagnostic_format_json_cc_tests ();
extern void diagnostic_format_sarif_cc_tests ();
--
2.26.3
