On Wed, 2025-07-16 at 15:58 +0200, Martin Jambor wrote: > 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?
Yes; as it happens Filip Kastl already reported this to me off-list, and I have a fix queued up. > > More generally speaking, is catching such unused private member > fields > useful ? The above was a genuine mistake on my part, so I'd say "yes". > (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. If you're using -Werror with these, that might be overkill, but otherwise the warnings seem useful to me (and maybe GCC should implement this?) Thanks Dave
