From 3dcce649a1e0833a4c3bb9ced4b9c0b38c3fb8a5 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Tue, 10 Jun 2025 20:06:37 -0400
Subject: [PATCH] diagnostics: fix tag nesting issues in experimental-html sink
 [PR120610]

I've been seeing issues in the experimental-html sink where the nesting
of tags goes wrong.

The two issues I've seen are:
* the pp_token_list from the diagnostic message that reaches the
  html_token_printer doesn't always have matching pairs of begin/end
  tokens (PR other/120610)
* a bug in diagnostic-show-locus where there was a stray xp.pop_tag,
  in print_trailing_fixits.

This patch:
* changes the xml::printer::pop_tag API so that it now takes the
  expected name of the element being popped (rather than expressing this
  in comments), and that, by default, the xml::printer asserts that this
  matches.
* gives the html_token_printer its own xml::printer instance to restrict
  the affected area of the DOM tree; this xml::printer doesn't enforce
  nesting (PR other/120610)
* adds RAII sentinel classes that automatically check for pushes/pops
  being balanced within a scope, using them in various places
* fixes the bug in print_trailing_fixits for html output

gcc/ChangeLog:
	PR other/120610
	* diagnostic-format-html.cc (html_builder::html_builder): Update
	for new param of xml::printer::pop_tag.
	(html_path_label_writer::end_label): Likewise.
	(html_builder::make_element_for_diagnostic::html_token_printer):
	Give the instance its own xml::printer.  Update for new param of
	xml::printer::pop_tag.
	(html_builder::make_element_for_diagnostic): Give the instance its
	own xml::printer.
	(html_builder::make_metadata_element): Update for new param of
	xml::printer::pop_tag.
	(html_builder::flush_to_file): Likewise.
	* diagnostic-path-output.cc (begin_html_stack_frame): Likewise.
	(begin_html_stack_frame): Likewise.
	(end_html_stack_frame): Likewise.
	(print_path_summary_as_html): Likewise.
	* diagnostic-show-locus.cc
	(struct to_text::auto_check_tag_nesting): New.
	(struct to_html:: auto_check_tag_nesting): New.
	(to_text::pop_html_tag): Change param to const char *.
	(to_html::pop_html_tag): Likewise; rename param to
	"expected_name".
	(default_diagnostic_start_span_fn<to_html>): Update for new param
	of xml::printer::pop_tag.
	(layout_printer<to_html>::end_label): Likewise.
	(layout_printer<Sink>::print_trailing_fixits): Add RAII sentinel
	to check tag nesting for the HTML case.  Delete stray popping
	of "td" in the presence of fix-it hints.
	(layout_printer<Sink>::print_line): Add RAII sentinel
	to check tag nesting for the HTML case.
	(diagnostic_source_print_policy::print_as_html): Likewise.
	(layout_printer<Sink>::print): Likewise.
	* xml-printer.h (xml::printer::printer): Add optional
	"check_popped_tags" param.
	(xml::printer::pop_tag): Add "expected_name" param.
	(xml::printer::get_num_open_tags): New accessor.
	(xml::printer::dump): New decl.
	(xml::printer::m_check_popped_tags): New field.
	(class xml::auto_check_tag_nesting): New.
	(class xml::auto_print_element): Update for new param of pop_tag.
	* xml.cc: Move pragma pop so that the pragma also covers
	xml::printer's member functions, "dump" in particular.
	(xml::printer::printer): Add param "check_popped_tags".
	(xml::printer::pop_tag): Add param "expected_name" and use it to assert
	that the popped tag is as expected.  Assert that we have a tag to
	pop.
	(xml::printer::dump): New.
	(selftest::test_printer): Update for new param of pop_tag.
	(selftest::test_attribute_ordering): Likewise.

gcc/testsuite/ChangeLog:
	PR other/120610
	* gcc.dg/format/diagnostic-ranges-html.py: Remove out-of-date
	comment.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
---
 gcc/diagnostic-format-html.cc                 | 29 +++++++-----
 gcc/diagnostic-path-output.cc                 | 28 +++++------
 gcc/diagnostic-show-locus.cc                  | 36 +++++++++++----
 .../gcc.dg/format/diagnostic-ranges-html.py   | 23 ----------
 gcc/xml-printer.h                             | 41 +++++++++++++++--
 gcc/xml.cc                                    | 46 ++++++++++++++-----
 6 files changed, 131 insertions(+), 72 deletions(-)

diff --git a/gcc/diagnostic-format-html.cc b/gcc/diagnostic-format-html.cc
index ddf6ba0f4cf..6010712b8a5 100644
--- a/gcc/diagnostic-format-html.cc
+++ b/gcc/diagnostic-format-html.cc
@@ -382,7 +382,7 @@ html_builder::html_builder (diagnostic_context &context,
 	  /* Escaping rules are different for HTML <script> elements,
 	     so add the script "raw" for now.  */
 	  xp.add_raw (HTML_SCRIPT);
-	  xp.pop_tag (); // script
+	  xp.pop_tag ("script");
 	}
     }
 
@@ -472,7 +472,7 @@ public:
 
   void end_label () final override
   {
-    m_xp.pop_tag (); // span
+    m_xp.pop_tag ("span"); // from begin_label
   }
 
 private:
@@ -489,8 +489,15 @@ html_builder::make_element_for_diagnostic (const diagnostic_info &diagnostic,
   class html_token_printer : public token_printer
   {
   public:
-    html_token_printer (xml::printer &xp)
-    : m_xp (xp)
+    html_token_printer (xml::element &parent_element)
+      /* Ideally pp_token_lists that reach a token_printer should be
+	 "balanced", but for now they can have mismatching pp_tokens
+	 e.g. a begin_color without an end_color (PR other/120610).
+	 Give html_token_printer its own xml::printer as a firewall to
+	 limit the scope of the mismatches in the HTML.  */
+    : m_xp (parent_element,
+	    /* Similarly we don't check that the popped tags match.  */
+	    false)
     {
     }
     void print_tokens (pretty_printer */*pp*/,
@@ -522,7 +529,7 @@ html_builder::make_element_for_diagnostic (const diagnostic_info &diagnostic,
 	    break;
 
 	  case pp_token::kind::end_color:
-	    m_xp.pop_tag ();
+	    m_xp.pop_tag ("span");
 	    break;
 
 	  case pp_token::kind::begin_quote:
@@ -533,7 +540,7 @@ html_builder::make_element_for_diagnostic (const diagnostic_info &diagnostic,
 	    break;
 	  case pp_token::kind::end_quote:
 	    {
-	      m_xp.pop_tag ();
+	      m_xp.pop_tag ("span");
 	      m_xp.add_text (close_quote);
 	    }
 	    break;
@@ -546,13 +553,13 @@ html_builder::make_element_for_diagnostic (const diagnostic_info &diagnostic,
 	    }
 	    break;
 	  case pp_token::kind::end_url:
-	    m_xp.pop_tag ();
+	    m_xp.pop_tag ("a");
 	    break;
 	  }
     }
 
   private:
-    xml::printer &m_xp;
+    xml::printer m_xp;
   };
 
   auto diag_element = make_div ("gcc-diagnostic");
@@ -574,7 +581,7 @@ html_builder::make_element_for_diagnostic (const diagnostic_info &diagnostic,
   add_focus_id (message_span_id);
 
   xml::printer xp (*message_span.get ());
-  html_token_printer tok_printer (xp);
+  html_token_printer tok_printer (*xp.get_insertion_point ());
   m_printer->set_token_printer (&tok_printer);
   pp_output_formatted_text (m_printer, m_context.get_urlifier ());
   m_printer->set_token_printer (nullptr);
@@ -680,7 +687,7 @@ html_builder::make_metadata_element (label_text label,
       }
     xp.add_text (label.get ());
     if (url.get ())
-      xp.pop_tag ();
+      xp.pop_tag ("a");
   }
   xp.add_text ("]");
   return item;
@@ -755,7 +762,7 @@ html_builder::flush_to_file (FILE *outf)
       m_ui_focus_ids.print (&pp, true);
       pp_string (&pp, ";\n");
       xp.add_raw (pp_formatted_text (&pp));
-      xp.pop_tag (); // script
+      xp.pop_tag ("script");
     }
   auto top = m_document.get ();
   top->dump (outf);
diff --git a/gcc/diagnostic-path-output.cc b/gcc/diagnostic-path-output.cc
index 4c17865f3c5..199028ea7f3 100644
--- a/gcc/diagnostic-path-output.cc
+++ b/gcc/diagnostic-path-output.cc
@@ -321,7 +321,7 @@ begin_html_stack_frame (xml::printer &xp,
       {
 	xp.push_tag_with_class ("td", "interprocmargin", false);
 	xp.set_attr ("style", "padding-left: 100px");
-	xp.pop_tag ();
+	xp.pop_tag ("td");
       }
       xp.push_tag_with_class ("td", "stack-frame", false);
       label_text funcname
@@ -331,8 +331,8 @@ begin_html_stack_frame (xml::printer &xp,
 	  xp.push_tag_with_class ("div", "frame-funcname", false);
 	  xp.push_tag ("span", true);
 	  xp.add_text (funcname.get ());
-	  xp.pop_tag (); // span
-	  xp.pop_tag (); // div
+	  xp.pop_tag ("span");
+	  xp.pop_tag ("div");
 	}
     }
   return std::make_unique<stack_frame> (std::move (parent),
@@ -350,9 +350,9 @@ end_html_stack_frame (xml::printer &xp,
   auto parent = std::move (frame->m_parent);
   if (frame->m_logical_loc)
     {
-      xp.pop_tag (); // td
-      xp.pop_tag (); // tr
-      xp.pop_tag (); // table
+      xp.pop_tag ("td");
+      xp.pop_tag ("tr");
+      xp.pop_tag ("table");
     }
   return parent;
 }
@@ -1230,7 +1230,7 @@ print_path_summary_as_html (const path_summary &ps,
 	    {
 	      xp.push_tag_with_class ("span", "funcname", true);
 	      xp.add_text (funcname.get ());
-	      xp.pop_tag (); //span
+	      xp.pop_tag ("span");
 	      xp.add_text (": ");
 	    }
 	}
@@ -1244,7 +1244,7 @@ print_path_summary_as_html (const path_summary &ps,
 	  pp_printf (&pp, "events %i-%i",
 		     range->m_start_idx + 1, range->m_end_idx + 1);
 	xp.add_text (pp_formatted_text (&pp));
-	xp.pop_tag (); // span
+	xp.pop_tag ("span");
       }
       if (show_depths)
 	{
@@ -1253,9 +1253,9 @@ print_path_summary_as_html (const path_summary &ps,
 	  pretty_printer pp;
 	  pp_printf (&pp, "(depth %i)", range->m_stack_depth);
 	  xp.add_text (pp_formatted_text (&pp));
-	  xp.pop_tag (); //span
+	  xp.pop_tag ("span");
 	}
-      xp.pop_tag (); // div
+      xp.pop_tag ("div");
 
       /* Print a run of events.  */
       thread_event_printer &tep = thread_event_printers[swimlane_idx];
@@ -1267,16 +1267,16 @@ print_path_summary_as_html (const path_summary &ps,
 						  range, &effect_info);
       last_out_edge_column = effect_info.m_trailing_out_edge_column;
 
-      xp.pop_tag (); // td
-      xp.pop_tag (); // tr
-      xp.pop_tag (); // table
+      xp.pop_tag ("td");
+      xp.pop_tag ("tr");
+      xp.pop_tag ("table");
     }
 
   /* Close outstanding frames.  */
   while (curr_frame)
     curr_frame = end_html_stack_frame (xp, std::move (curr_frame));
 
-  xp.pop_tag (); // div
+  xp.pop_tag ("div");
 }
 
 } /* end of anonymous namespace for path-printing code.  */
diff --git a/gcc/diagnostic-show-locus.cc b/gcc/diagnostic-show-locus.cc
index 3654102509b..575c7ec8d70 100644
--- a/gcc/diagnostic-show-locus.cc
+++ b/gcc/diagnostic-show-locus.cc
@@ -408,6 +408,12 @@ struct to_text
 {
   friend class layout_printer<to_text>;
 
+  // This is a RAII class for HTML, but is a no-op for text.
+  struct auto_check_tag_nesting
+  {
+    auto_check_tag_nesting (to_text &) {}
+  };
+
   to_text (pretty_printer &pp,
 	   colorizer &colorizer)
   : m_pp (pp),
@@ -446,7 +452,7 @@ struct to_text
   {
     // no-op for text
   }
-  void pop_html_tag (std::string)
+  void pop_html_tag (const char *)
   {
     // no-op for text
   }
@@ -538,6 +544,16 @@ struct to_html
 {
   friend class layout_printer<to_html>;
 
+  // RAII class for ensuring that the tags nested correctly
+  struct auto_check_tag_nesting : public xml::auto_check_tag_nesting
+  {
+  public:
+    auto_check_tag_nesting (to_html &sink)
+    : xml::auto_check_tag_nesting (sink.m_xp)
+    {
+    }
+  };
+
   to_html (xml::printer &xp,
 	   const rich_location *richloc,
 	   html_label_writer *html_label_writer)
@@ -569,9 +585,9 @@ struct to_html
 			      preserve_whitespace);
   }
 
-  void pop_html_tag (std::string /*name*/)
+  void pop_html_tag (const char *expected_name)
   {
-    m_xp.pop_tag ();
+    m_xp.pop_tag (expected_name);
   }
 
   void add_html_tag_with_class (std::string name,
@@ -728,7 +744,7 @@ default_diagnostic_start_span_fn<to_html> (const diagnostic_location_print_polic
 				       false);
   sink.m_xp.push_tag_with_class ("span", "location", true);
   sink.m_xp.add_text (text.get ());
-  sink.m_xp.pop_tag (); // span
+  sink.m_xp.pop_tag ("span");
 }
 
 namespace {
@@ -2719,7 +2735,7 @@ layout_printer<to_html>::end_label (int range_idx,
 				    bool is_label_text)
 {
   if (m_sink.get_highlight_color_for_range_idx (range_idx))
-    m_sink.m_xp.pop_tag ();
+    m_sink.m_xp.pop_tag ("span");
 
   if (is_label_text && m_sink.m_html_label_writer)
     m_sink.m_html_label_writer->end_label ();
@@ -3438,6 +3454,8 @@ template<typename Sink>
 void
 layout_printer<Sink>::print_trailing_fixits (linenum_type row)
 {
+  typename Sink::auto_check_tag_nesting sentinel (m_sink);
+
   /* Build a list of correction instances for the line,
      potentially consolidating hints (for the sake of readability).  */
   line_corrections corrections (m_layout.m_file_cache, m_layout.m_char_policy,
@@ -3509,9 +3527,6 @@ layout_printer<Sink>::print_trailing_fixits (linenum_type row)
 	}
     }
 
-  if (!corrections.m_corrections.is_empty ())
-    m_sink.pop_html_tag ("td");
-
   /* Add a trailing newline, if necessary.  */
   move_to_column (&column, 1 + m_layout.m_x_offset_display, false);
 }
@@ -3691,6 +3706,8 @@ template<typename Sink>
 void
 layout_printer<Sink>::print_line (linenum_type row)
 {
+  typename Sink::auto_check_tag_nesting sentinel (m_sink);
+
   char_span line
     = m_layout.m_file_cache.get_source_line (m_layout.m_exploc.file, row);
   if (!line)
@@ -3948,6 +3965,7 @@ diagnostic_source_print_policy::print_as_html (xml::printer &xp,
   to_html sink (xp, &richloc, label_writer);
   layout_printer<to_html> lp (sink, layout,
 			      diagnostic_kind == DK_DIAGNOSTIC_PATH);
+  xml::auto_check_tag_nesting sentinel (xp);
   lp.print (*this);
 }
 
@@ -3955,6 +3973,8 @@ template <typename Sink>
 void
 layout_printer<Sink>::print (const diagnostic_source_print_policy &source_policy)
 {
+  typename Sink::auto_check_tag_nesting sentinel (m_sink);
+
   m_sink.push_html_tag_with_class ("table", "locus", false);
 
   if (get_options ().show_ruler_p)
diff --git a/gcc/testsuite/gcc.dg/format/diagnostic-ranges-html.py b/gcc/testsuite/gcc.dg/format/diagnostic-ranges-html.py
index 91383d64457..1d798939e72 100644
--- a/gcc/testsuite/gcc.dg/format/diagnostic-ranges-html.py
+++ b/gcc/testsuite/gcc.dg/format/diagnostic-ranges-html.py
@@ -74,26 +74,3 @@ def test_annotations(html_tree):
     assert_class(tds[1], 'annotation')
     assert_highlighted_text(tds[1][0], 'highlight-a', 'int')
     assert_highlighted_text(tds[1][1], 'highlight-b', 'const char *')
-
-# For reference, here's the generated HTML:
-"""
-        <span class="gcc-message" id="gcc-diag-0-message">format &apos;<span class="gcc-quoted-text"><span class="high
-light-a">%i</span></span>&apos; expects argument of type &apos;<span class="gcc-quoted-text"><span class="highlight-a"
->int</span></span>&apos;, but argument 2 has type &apos;<span class="gcc-quoted-text"><span class="highlight-b">const 
-char *</span></span>&apos;</span>
-         
-        <span class="gcc-option">[<a href="https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat">-Wfo
-rmat=</a>]</span>
-        <table class="locus">
-          <tbody class="line-span">
-            <tr><td class="left-margin"> </td><td class="source">  printf(&quot;hello <span class="highlight-a">%i</span>&quot;, <span class="highlight-b">msg</span>);  /* { dg-warning &quot;format &apos;%i&apos; expects argument of type &apos;int&apos;, but argument 2 has type &apos;const char \\*&apos; &quot; } */</td></tr>
-            <tr><td class="left-margin"> </td><td class="annotation">                <span class="highlight-a">~^</spa
-n>   <span class="highlight-b">~~~</span></td></tr>
-            <tr><td class="left-margin"> </td><td class="annotation">                 <span class="highlight-a">|</spa
-n>   <span class="highlight-b">|</span></td></tr>
-            <tr><td class="left-margin"> </td><td class="annotation">                 <span class="highlight-a">int</s
-pan> <span class="highlight-b">const char *</span></td></tr>
-            <tr><td class="left-margin"> </td><td class="annotation">                %s</td></tr>
-          </tbody>
-        </table>
-"""
diff --git a/gcc/xml-printer.h b/gcc/xml-printer.h
index b90390cb0f6..24ac2f42e73 100644
--- a/gcc/xml-printer.h
+++ b/gcc/xml-printer.h
@@ -32,14 +32,14 @@ class node;
 class printer
 {
 public:
-  printer (element &insertion_point);
+  printer (element &insertion_point, bool check_popped_tags = true);
 
   void push_tag (std::string name,
 		 bool preserve_whitespace = false);
   void push_tag_with_class (std::string name,
 			    std::string class_,
 			    bool preserve_whitespace = false);
-  void pop_tag ();
+  void pop_tag (const char *expected_name);
 
   void set_attr (const char *name, std::string value);
 
@@ -53,9 +53,38 @@ public:
 
   element *get_insertion_point () const;
 
+  size_t get_num_open_tags () const { return m_open_tags.size (); }
+
+  void DEBUG_FUNCTION dump () const;
+
 private:
   // borrowed ptrs:
   std::vector<element *> m_open_tags;
+  bool m_check_popped_tags;
+};
+
+/* RAII class for ensuring that the tags nested correctly.
+   Verify that within an instance's lifetime that any pushes
+   to the printer's insertion point have been popped by the end.  */
+
+class auto_check_tag_nesting
+{
+public:
+  auto_check_tag_nesting (const printer &xp)
+  : m_xp (xp),
+    m_initial_insertion_element (xp.get_insertion_point ())
+  {
+  }
+  ~auto_check_tag_nesting ()
+  {
+    /* Verify that we didn't pop too many tags within the printer,
+       or leave any tags open.  */
+    gcc_assert (m_initial_insertion_element == m_xp.get_insertion_point ());
+  }
+
+private:
+  const printer &m_xp;
+  const element *m_initial_insertion_element;
 };
 
 // RAII for push/pop element on xml::printer
@@ -66,17 +95,19 @@ public:
   auto_print_element (printer &printer,
 		      std::string name,
 		      bool preserve_whitespace = false)
-  : m_printer (printer)
+  : m_printer (printer),
+    m_name (std::move (name))
   {
-    m_printer.push_tag (name, preserve_whitespace);
+    m_printer.push_tag (m_name, preserve_whitespace);
   }
   ~auto_print_element ()
   {
-    m_printer.pop_tag ();
+    m_printer.pop_tag (m_name.c_str ());
   }
 
 private:
   printer &m_printer;
+  std::string m_name;
 };
 
 } // namespace xml
diff --git a/gcc/xml.cc b/gcc/xml.cc
index 6c95288607d..0a925619f5d 100644
--- a/gcc/xml.cc
+++ b/gcc/xml.cc
@@ -196,13 +196,11 @@ raw::write_as_xml (pretty_printer *pp,
   pp_string (pp, m_xml_src.c_str ());
 }
 
-#if __GNUC__ >= 10
-#  pragma GCC diagnostic pop
-#endif
-
 // class printer
 
-printer::printer (element &insertion_point)
+printer::printer (element &insertion_point,
+		  bool check_popped_tags)
+: m_check_popped_tags (check_popped_tags)
 {
   m_open_tags.push_back (&insertion_point);
 }
@@ -227,9 +225,16 @@ printer::push_tag_with_class (std::string name, std::string class_,
   push_element (std::move (new_element));
 }
 
+/*  Pop the current topmost tag.
+    If m_check_popped_tags, assert that the tag we're popping is
+    EXPECTED_NAME.  */
+
 void
-printer::pop_tag ()
+printer::pop_tag (const char *expected_name ATTRIBUTE_UNUSED)
 {
+  gcc_assert (!m_open_tags.empty ());
+  if (m_check_popped_tags)
+    gcc_assert (expected_name == get_insertion_point ()->m_kind);
   m_open_tags.pop_back ();
 }
 
@@ -274,6 +279,25 @@ printer::get_insertion_point () const
   return m_open_tags.back ();
 }
 
+void
+printer::dump () const
+{
+  pretty_printer pp;
+  pp.set_output_stream (stderr);
+  pp_printf (&pp, "open tags: %i:", (int)m_open_tags.size ());
+  for (auto iter : m_open_tags)
+    pp_printf (&pp, " <%s>", iter->m_kind.c_str ());
+  pp_newline (&pp);
+  pp_printf (&pp, "xml:");
+  pp_newline (&pp);
+  m_open_tags[0]->write_as_xml (&pp, 1, true);
+  pp_flush (&pp);
+}
+
+#if __GNUC__ >= 10
+#  pragma GCC diagnostic pop
+#endif
+
 } // namespace xml
 
 #if CHECKING_P
@@ -303,9 +327,9 @@ test_printer ()
   xp.set_attr ("color", "red");
   xp.add_text ("world");
   xp.push_tag ("baz");
-  xp.pop_tag ();
-  xp.pop_tag ();
-  xp.pop_tag ();
+  xp.pop_tag ("baz");
+  xp.pop_tag ("bar");
+  xp.pop_tag ("foo");
 
   pretty_printer pp;
   top.write_as_xml (&pp, 0, true);
@@ -334,13 +358,13 @@ test_attribute_ordering ()
   xp.set_attr ("hastings", "1066");
   xp.set_attr ("edgehill", "1642");
   xp.set_attr ("naseby", "1645");
-  xp.pop_tag ();
+  xp.pop_tag ("chronological");
   xp.push_tag ("alphabetical");
   xp.set_attr ("edgehill", "1642");
   xp.set_attr ("hastings", "1066");
   xp.set_attr ("maldon", "991");
   xp.set_attr ("naseby", "1645");
-  xp.pop_tag ();
+  xp.pop_tag ("alphabetical");
 
   pretty_printer pp;
   top.write_as_xml (&pp, 0, true);
-- 
2.47.2