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
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r16-1404-g3dcce649a1e083.
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.
---
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 ddf6ba0f4cfc..6010712b8a5b 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 4c17865f3c55..199028ea7f3a 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 3654102509b9..575c7ec8d709 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 91383d64457a..1d798939e729 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 '<span
class="gcc-quoted-text"><span class="high
-light-a">%i</span></span>' expects argument of type '<span
class="gcc-quoted-text"><span class="highlight-a"
->int</span></span>', but argument 2 has type '<span
class="gcc-quoted-text"><span class="highlight-b">const
-char *</span></span>'</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("hello <span class="highlight-a">%i</span>", <span
class="highlight-b">msg</span>); /* { dg-warning "format '%i'
expects argument of type 'int', but argument 2 has type 'const
char \\*' " } */</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 b90390cb0f6e..24ac2f42e735 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 6c95288607de..0a925619f5d3 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.26.3
