On Mon, 2020-05-25 at 16:56 +0200, Richard Biener wrote:
> This adds an alternate debug_dump_context similar to the one for
> selftests but for interactive debugging routines. This allows
> to share code between user-visible dumping via the dump_* API
> and those debugging routines. The primary driver was SLP node
> dumping which wasn't accessible from inside a gdb session up to
> now.
>
> Bootstrap & regtest running on x86_64-unknown-linux-gnu.
>
> David, does this look OK?
Overall, seems sane to me; a couple of items inline below.
> Thanks,
> Richard.
>
> 2020-05-25 Richard Biener <rguent...@suse.de>
>
> * dump-context.h (debug_dump_context): New class.
> (dump_context): Make it friend.
> * dumpfile.c (debug_dump_context::debug_dump_context):
> Implement.
> (debug_dump_context::~debug_dump_context): Likewise.
> * tree-vect-slp.c: Include dump-context.h.
> (vect_print_slp_tree): Dump a single SLP node.
> (debug): New overload for slp_tree.
> (vect_print_slp_graph): Rename from vect_print_slp_tree and
> use that.
> (vect_analyze_slp_instance): Adjust.
> ---
> gcc/dump-context.h | 19 ++++++++++++++++++
> gcc/dumpfile.c | 26 +++++++++++++++++++++++++
> gcc/tree-vect-slp.c | 47 +++++++++++++++++++++++++++++++++--------
> ----
> 3 files changed, 80 insertions(+), 12 deletions(-)
>
> diff --git a/gcc/dump-context.h b/gcc/dump-context.h
> index 347477f331e..6d51eaf31ad 100644
> --- a/gcc/dump-context.h
> +++ b/gcc/dump-context.h
> @@ -29,6 +29,7 @@ along with GCC; see the file COPYING3. If not see
>
> class optrecord_json_writer;
> namespace selftest { class temp_dump_context; }
> +class debug_dump_context;
>
> /* A class for handling the various dump_* calls.
>
> @@ -42,6 +43,7 @@ namespace selftest { class temp_dump_context; }
> class dump_context
> {
> friend class selftest::temp_dump_context;
> + friend class debug_dump_context;
>
> public:
> static dump_context &get () { return *s_current; }
> @@ -195,6 +197,23 @@ private:
> auto_vec<stashed_item> m_stashed_items;
> };
>
> +/* An RAII-style class for use in debug dumpers for temporarily
> using a
> + different dump_context. */
> +
> +class debug_dump_context
(Bikeshed Alert) The name might be confusing in that this class isn't
a dump_context itself. Some of our existing RAII classes have an
"auto_" prefix; would that be an idea?
Maybe "auto_dump_everything"???
But I don't have a strong objection to the name as-is.
[...snip...]
> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> index 54718784fd4..0c0c076d890 100644
> --- a/gcc/dumpfile.c
> +++ b/gcc/dumpfile.c
> @@ -2078,6 +2078,32 @@ enable_rtl_dump_file (void)
> return num_enabled > 0;
> }
>
> +/* debug_dump_context's ctor. Temporarily override the dump_context
> + (to forcibly enable output to stderr). */
> +
> +debug_dump_context::debug_dump_context ()
> +: m_context (),
> + m_saved (&dump_context::get ()),
> + m_saved_flags (dump_flags),
> + m_saved_file (dump_file)
> +{
> + set_dump_file (stderr);
> + dump_context::s_current = &m_context;
> + pflags = dump_flags = MSG_ALL_KINDS | MSG_ALL_PRIORITIES;
> + dump_context::get ().refresh_dumps_are_enabled ();
> +}
> +
> +/* debug_dump_context's dtor. Restore the saved dump_context. */
> +
> +debug_dump_context::~debug_dump_context ()
> +{
> + set_dump_file (m_saved_file);
> + dump_context::s_current = m_saved;
> + pflags = dump_flags = m_saved_flags;
> + dump_context::get ().refresh_dumps_are_enabled ();
> +}
I notice that the code saves dump_flags, and later restores both
dump_flags and pflags to the same value. I'm a little hazy on this,
but is there any guarantee they had the same value? Should the value
of pflags be saved separately from dump_flags?
[...snip...]
Hope this is constructive
Dave
