Hello David,
On Fri, Jul 11 2025, David Malcolm wrote:
> In r16-1631-g2334d30cd8feac I added support for capturing state
> information from -fanalyzer in XML form, and adding a way to visualize
> these states in HTML output. The data was optionally captured in SARIF
> output (with "xml-state=yes"), stashing the XML in string form in
> a property bag.
>
> This worked, but there was no way to round-trip the stored data back
> from SARIF without adding an XML parser to GCC, which I don't want to
> do.
>
> SARIF supports capturing directed graphs, so this patch:
>
> (a) adds a new namespace diagnostics::digraphs, with classes digraph,
> node, and edge, representing directed graphs in a form similar to
> what SARIF can serialize
>
> (b) adds support to GCC's diagnostic subsystem for reporting graphs,
> either "globally" or as part of a diagnostic. An example in a testsuite
> plugin emits an error that has a couple of dummy graphs associated with
> it, and captures the optimization passes as a digraph "globally".
> Graphs are ignored by text sinks, but are captured by sarif sinks,
> and the "experimental-html" sink gains SVG-based rendering of any graphs
> using dot. This HTML output is rather crude; an example can be seen
> here:
>
> https://dmalcolm.fedorapeople.org/gcc/2025-07-10/diagnostic-test-graphs-html.c.html
>
> (c) adds support to libgdiagnostics for the above
>
> (d) adds support to sarif-replay for the above (round-tripping any
> graph information)
>
> (e) replaces the XML representation of state with a representation
> based on the above directed graphs, using property bags to stash
> additional information (e.g. "this is an on-stack buffer")
>
> (f) implements round-tripping of this information in sarif-replay
>
[...]
> diff --git a/gcc/diagnostic-state-to-dot.cc b/gcc/diagnostic-state-to-dot.cc
> index ddae83b85cd2..8195c1148fe0 100644
> --- a/gcc/diagnostic-state-to-dot.cc
> +++ b/gcc/diagnostic-state-to-dot.cc
> @@ -1,4 +1,4 @@
> -/* Creating GraphViz .dot files from XML state documents.
> +/* Creating GraphViz .dot files from diagnostic state graphs.
> Copyright (C) 2025 Free Software Foundation, Inc.
> Contributed by David Malcolm <dmalc...@redhat.com>.
>
[...]
> @@ -237,13 +244,13 @@ private:
> enum class style { h1, h2 };
>
> void
> - add_title_tr (const dot::id &id_of_node,
> + add_title_tr (const dot::id &id_of_dot_node,
> xml::printer &xp,
> int num_columns,
> - const xml::element &input_element,
> + state_node_ref state_node,
> std::string heading,
> enum style styl,
> - enum dynalloc_state dynalloc_state)
> + enum node_dynalloc_state dynalloc_state)
> {
> xp.push_tag ("tr", true);
> xp.push_tag ("td", false);
> @@ -282,183 +289,194 @@ private:
> xp.add_text (std::move (heading));
> xp.pop_tag ("font");
>
> - maybe_add_dst_port (id_of_node, xp, input_element);
> + maybe_add_dst_port (id_of_dot_node, xp, state_node);
>
> xp.pop_tag ("td");
> xp.pop_tag ("tr");
> }
>
> - /* Recursively add <TR> to XP for INPUT_NODE and its descendents. */
> + /* Recursively add <TR> to XP for STATE_NODE and its descendents. */
> void
> - on_xml_node (const dot::id &id_of_node,
> - xml::printer &xp,
> - xml::node &input_node,
> - int max_depth,
> - int depth,
> - int num_columns)
> + on_node_in_table (const dot::id &id_of_dot_node,
> + xml::printer &xp,
> + state_node_ref state_node,
> + int max_depth,
> + int depth,
> + int num_columns)
> {
> bool recurse = true;
> + auto input_node_kind = state_node.get_node_kind ();
>
> - xml::element *input_element = input_node.dyn_cast_element ();
> - if (!input_element)
> - return;
> -
> - if (input_element->m_kind == "concrete-bindings")
> - return;
> - if (input_element->m_kind == "padding")
> - return;
> -
> - if (input_element->m_kind == "stack")
> - {
> - add_title_tr (id_of_node, xp, num_columns, *input_element, "Stack",
> - style::h1, dynalloc_state::unknown);
> - }
> - else if (input_element->m_kind == "stack-frame")
> - {
> - if (const char *function = input_element->get_attr ("function"))
> - add_title_tr (id_of_node, xp, num_columns, *input_element,
> - std::string ("Frame: ") + function,
> - style::h2, dynalloc_state::unknown);
> - }
> - else if (input_element->m_kind == "heap-buffer")
> + switch (input_node_kind)
> {
> - const char *extents = input_element->get_attr ("dynamic-extents");
> - enum dynalloc_state dynalloc_st = get_dynalloc_state (*input_element);
> - if (auto region_id = input_element->get_attr ("region_id"))
> - m_region_id_to_dynalloc_state[region_id] = dynalloc_st;
> - const char *type = input_element->get_attr ("type");
> - pretty_printer pp;
> - switch (dynalloc_st)
> - {
> - default:
> - gcc_unreachable ();
> -
> - case dynalloc_state::unknown:
> - case dynalloc_state::nonnull:
> - if (type)
> - {
> + case node_kind::padding:
> + case node_kind::other:
> + return;
> +
> + case node_kind::stack:
> + add_title_tr (id_of_dot_node, xp, num_columns, state_node, "Stack",
> + style::h1,
> + node_dynalloc_state::unknown);
> + break;
> + case node_kind::stack_frame:
> + if (auto logical_loc = state_node.get_logical_loc ())
> + if (const char *function
> + = m_logical_loc_mgr.get_short_name (logical_loc))
> + add_title_tr (id_of_dot_node, xp, num_columns, state_node,
> + std::string ("Frame: ") + function,
> + style::h2,
> + node_dynalloc_state::unknown);
> + break;
> + case node_kind::dynalloc_buffer:
> + {
> + enum node_dynalloc_state dynalloc_st
> + = state_node.get_dynalloc_state ();
> + const char *extents = state_node.get_dynamic_extents ();
> + const char *type = state_node.get_type ();
> + pretty_printer pp;
> + switch (dynalloc_st)
> + {
> + default:
> + gcc_unreachable ();
> +
> + case node_dynalloc_state::unknown:
> + case node_dynalloc_state::nonnull:
> + if (type)
> + {
> if (extents)
> pp_printf (&pp, "%s (%s byte allocation)",
> type, extents);
> else
> pp_printf (&pp, "%s", type);
> - }
> - else
> + }
> + else
> + {
> + if (extents)
> + pp_printf (&pp, "%s byte allocation",
> + extents);
> + }
> + break;
> +
> + case node_dynalloc_state::unchecked:
> + if (type)
> + {
> + if (extents)
> + pp_printf (&pp, "%s (unchecked %s byte allocation)",
> + type, extents);
> + }
> + else
> + {
> + if (extents)
> + pp_printf (&pp, "Unchecked %s byte allocation",
> + extents);
> + }
> + break;
> +
> + case node_dynalloc_state::freed:
> + // TODO: show deallocator
> + // TODO: show deallocation event
> + pp_printf (&pp, "Freed buffer");
> + break;
> + }
> + maybe_add_dst_port (id_of_dot_node, xp, state_node);
> + add_title_tr (id_of_dot_node, xp, num_columns, state_node,
> + pp_formatted_text (&pp),
> + style::h2,
> + dynalloc_st);
> + }
> + break;
> +
> + default:
> + {
> + xp.push_tag ("tr", true);
> +
> + maybe_add_dst_port (id_of_dot_node, xp, state_node);
> +
> + if (depth > 0)
> + {
> + /* Indent, by create a <td> spanning "depth" columns. */
> + xp.push_tag ("td", false);
> + xp.set_attr ("colspan", std::to_string (depth));
> + xp.add_text (" "); // graphviz doesn't like <td/>
> + xp.pop_tag ("td");
> + }
> +
> + switch (input_node_kind)
> + {
> + default:
> + break;
> + case node_kind::variable:
> {
> - if (extents)
> - pp_printf (&pp, "%s byte allocation",
> - extents);
> + const char *name = state_node.get_name ();
> + gcc_assert (name);
> + xp.push_tag ("td", false);
> + maybe_add_dst_port (id_of_dot_node, xp, state_node);
> + push_src_text (xp);
> + xp.add_text (name);
> + pop_src_text (xp);
> + xp.pop_tag ("td");
> }
> - break;
> -
> - case dynalloc_state::unchecked:
> - if (type)
> + break;
> + case node_kind::element:
> {
> - if (extents)
> - pp_printf (&pp, "%s (unchecked %s byte allocation)",
> - type, extents);
> + const char *index = state_node.get_index ();
> + gcc_assert (index);
> + xp.push_tag ("td", false);
> + maybe_add_dst_port (id_of_dot_node, xp, state_node);
> + push_src_text (xp);
> + xp.add_text ("[");
> + xp.add_text (index);
> + xp.add_text ("]");
> + pop_src_text (xp);
> + xp.pop_tag ("td");
> }
> - else
> + break;
> + case node_kind::field:
> {
> - if (extents)
> - pp_printf (&pp, "Unchecked %s byte allocation",
> - extents);
> + const char *name = state_node.get_name ();
> + gcc_assert (name);
> + xp.push_tag ("td", false);
> + maybe_add_dst_port (id_of_dot_node, xp, state_node);
> + push_src_text (xp);
> + xp.add_text (".");
> + xp.add_text (name);
> + pop_src_text (xp);
> + xp.pop_tag ("td");
> }
> - break;
> -
> - case dynalloc_state::freed:
> - // TODO: show deallocator
> - // TODO: show deallocation event
> - pp_printf (&pp, "Freed buffer");
> - break;
> - }
> - add_title_tr (id_of_node, xp, num_columns, *input_element,
> - pp_formatted_text (&pp),
> - style::h2,
> - dynalloc_st);
> - }
> - else
> - {
> - xp.push_tag ("tr", true);
> - if (depth > 0)
> - {
> - /* Indent, by create a <td> spanning "depth" columns. */
> - xp.push_tag ("td", false);
> - xp.set_attr ("colspan", std::to_string (depth));
> - xp.add_text (" "); // graphviz doesn't like <td/>
> - xp.pop_tag ("td");
> - }
> - if (m_show_tags)
> - {
> - // Debug: show XML tag
> - xp.push_tag ("td", false);
> - xp.add_text ("<");
> - xp.add_text (input_element->m_kind);
> - xp.add_text (">");
> - xp.pop_tag ("td");
> - }
This seems to have removed the only use, of...
[...]
>
> private:
> - int m_next_id;
> - std::vector<pending_edge> m_pending_edges;
> - std::map<std::string, dot::node_id> m_region_id_to_dst_node_id;
> - std::map<std::string, enum dynalloc_state> m_region_id_to_dynalloc_state;
> + const logical_location_manager &m_logical_loc_mgr;
> +
> + /* All nodes involved in edges (and thus will need a port). */
> + std::set<diagnostics::digraphs::node *> m_src_nodes;
> + std::set<diagnostics::digraphs::node *> m_dst_nodes;
> +
> + std::map<diagnostics::digraphs::node *, dot::node_id>
> m_src_node_to_port_id;
> + std::map<diagnostics::digraphs::node *, dot::node_id>
> m_dst_node_to_port_id;
> +
> bool m_show_tags;
> };
this private member, at least clang says so (but it looks only
initialized and never read). Should it have been removed as a part of
your patch?
More generally speaking, is catching such unused private member fields
useful ? (There is another, m_wanted_type, in c-family/c-format.cc.) I
can just filter them out if we feel that the warnings simply get in the
way of building/changing things in stages.
Thanks,
Martin
