Skip to content

Commit 7969e48

Browse files
davidmalcolmdavidmalcolm
committed
diagnostics: improve support for nesting levels [PR116253]
This patch adds support to sarif-replay for "nestingLevel" from "P3358R0 SARIF for Structured Diagnostics" https://wg21.link/P3358R0 Doing so revealed a bug where libgdiagnostics was always creating new location_t values (and thus also diagnostic_physical_location instances), rather than reusing existing location_t values, leading to excess source printing. The patch also fixes this bug, adding a new flag to libgdiagnostics for debugging physical locations, and exposing this in sarif-replay via a new "-fdebug-physical-locations" maintainer option. Finally, the patch adds test coverage for the HTML sink's output of nested diagnostics (both from a GCC plugin, and from sarif-replay). gcc/ChangeLog: PR diagnostics/116253 * diagnostics/context.cc (context::set_nesting_level): New. * diagnostics/context.h (context::set_nesting_level): New decl. * doc/libgdiagnostics/topics/compatibility.rst (LIBGDIAGNOSTICS_ABI_5): New. * doc/libgdiagnostics/topics/physical-locations.rst (diagnostic_manager_set_debug_physical_locations): New. * libgdiagnostics++.h (manager::set_debug_physical_locations): New. * libgdiagnostics-private.h (private_diagnostic_set_nesting_level): New decl. * libgdiagnostics.cc (diagnostic_manager::diagnostic_manager): Initialize m_debug_physical_locations. (diagnostic_manager::new_location_from_file_and_line): Add debug printing. (diagnostic_manager::new_location_from_file_line_column): Likewise. (diagnostic_manager::new_location_from_range): Likewise. (diagnostic_manager::set_debug_physical_locations): New. (diagnostic_manager::ensure_linemap_for_file_and_line): Avoid redundant calls to linemap_add. (diagnostic_manager::new_location): Add debug printing. (diagnostic_manager::m_debug_physical_locations): New field. (diagnostic::diagnostic): Initialize m_nesting_level. (diagnostic::get_nesting_level): New accessor. (diagnostic::set_nesting_level): New. (diagnostic::m_nesting_level): New field. (diagnostic_manager::emit_va): Set and reset the nesting level of the context from that of the diagnostic. (diagnostic_manager_set_debug_physical_locations): New. (private_diagnostic_set_nesting_level): New. * libgdiagnostics.h (diagnostic_manager_set_debug_physical_locations): New decl. * libgdiagnostics.map (LIBGDIAGNOSTICS_ABI_5): New. * libsarifreplay.cc (sarif_replayer::handle_result_obj): Support the "nestingLevel" property. * libsarifreplay.h (replay_options::m_debug_physical_locations): New field. * sarif-replay.cc: Add -fdebug-physical-locations. gcc/testsuite/ChangeLog: PR diagnostics/116253 * gcc.dg/plugin/diagnostic-test-nesting-html.c: New test. * gcc.dg/plugin/diagnostic-test-nesting-html.py: New test script. * gcc.dg/plugin/plugin.exp: Add it. * libgdiagnostics.dg/test-multiple-lines.c: Update expected output to show fix-it hint. * sarif-replay.dg/2.1.0-valid/nested-diagnostics-1.sarif: New test. Signed-off-by: David Malcolm <[email protected]>
1 parent 0fb3000 commit 7969e48

17 files changed

+408
-10
lines changed

‎gcc/diagnostics/context.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1681,6 +1681,12 @@ context::pop_nesting_level ()
16811681
inhibit_notes_in_group (/*inhibit=*/false);
16821682
}
16831683

1684+
void
1685+
context::set_nesting_level (int new_level)
1686+
{
1687+
m_diagnostic_groups.m_diagnostic_nesting_level = new_level;
1688+
}
1689+
16841690
void
16851691
sink::dump (FILE *out, int indent) const
16861692
{

‎gcc/diagnostics/context.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,7 @@ class context
301301

302302
void push_nesting_level ();
303303
void pop_nesting_level ();
304+
void set_nesting_level (int new_level);
304305

305306
bool warning_enabled_at (location_t loc, option_id opt_id);
306307

‎gcc/doc/libgdiagnostics/topics/compatibility.rst

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,3 +262,12 @@ working with :type:`diagnostic_message_buffer`.
262262
* :func:`diagnostic_add_location_with_label_via_msg_buf`
263263

264264
* :func:`diagnostic_execution_path_add_event_via_msg_buf`
265+
266+
.. _LIBGDIAGNOSTICS_ABI_5:
267+
268+
``LIBGDIAGNOSTICS_ABI_5``
269+
-------------------------
270+
271+
``LIBGDIAGNOSTICS_ABI_5`` covers the addition of this function:
272+
273+
* :func:`diagnostic_manager_set_debug_physical_locations`

‎gcc/doc/libgdiagnostics/topics/physical-locations.rst

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,3 +304,19 @@ This diagnostic has three locations
304304
.. code-block:: c
305305
306306
#ifdef LIBDIAGNOSTICS_HAVE_diagnostic_message_buffer
307+
308+
.. function:: void diagnostic_manager_set_debug_physical_locations (diagnostic_manager *mgr, \
309+
int value)
310+
311+
Calling ``diagnostic_manager_set_debug_physical_locations (mgr, 1);``
312+
will lead to debugging information being printed to ``stderr`` when
313+
creating :type:`diagnostic_physical_location` instances.
314+
315+
The precise format of these messages is subject to change.
316+
317+
This function was added in :ref:`LIBGDIAGNOSTICS_ABI_5`; you can
318+
test for its presence using
319+
320+
.. code-block:: c
321+
322+
#ifdef LIBDIAGNOSTICS_HAVE_diagnostic_manager_set_debug_physical_locations

‎gcc/libgdiagnostics++.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,9 @@ class manager
477477
void
478478
take_global_graph (graph g);
479479

480+
void
481+
set_debug_physical_locations (bool value);
482+
480483
diagnostic_manager *m_inner;
481484
bool m_owned;
482485
};
@@ -926,6 +929,13 @@ manager::take_global_graph (graph g)
926929
g.m_owned = false;
927930
}
928931

932+
inline void
933+
manager::set_debug_physical_locations (bool value)
934+
{
935+
diagnostic_manager_set_debug_physical_locations (m_inner,
936+
value ? 1 : 0);
937+
}
938+
929939
// class graph
930940

931941
inline void

‎gcc/libgdiagnostics-private.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,13 @@ private_diagnostic_execution_path_add_event_3 (diagnostic_execution_path *path,
5858
LIBGDIAGNOSTICS_PARAM_CAN_BE_NULL (5)
5959
LIBGDIAGNOSTICS_PARAM_MUST_BE_NON_NULL (6);
6060

61+
/* Entrypoint added in LIBGDIAGNOSTICS_ABI_5. */
62+
63+
extern void
64+
private_diagnostic_set_nesting_level (diagnostic *diag,
65+
int nesting_level)
66+
LIBGDIAGNOSTICS_PARAM_MUST_BE_NON_NULL (1);
67+
6168
} // extern "C"
6269

6370
#endif /* LIBGDIAGNOSTICS_PRIVATE_H */

‎gcc/libgdiagnostics.cc

Lines changed: 68 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,8 @@ struct diagnostic_manager
639639
public:
640640
diagnostic_manager ()
641641
: m_current_diag (nullptr),
642-
m_prev_diag_logical_loc (nullptr)
642+
m_prev_diag_logical_loc (nullptr),
643+
m_debug_physical_locations (false)
643644
{
644645
linemap_init (&m_line_table, BUILTINS_LOCATION);
645646
m_line_table.m_reallocator = xrealloc;
@@ -747,6 +748,9 @@ struct diagnostic_manager
747748
new_location_from_file_and_line (const diagnostic_file *file,
748749
diagnostic_line_num_t line_num)
749750
{
751+
if (m_debug_physical_locations)
752+
fprintf (stderr, "new_location_from_file_and_line (%s, %i)",
753+
file->get_name (), line_num);
750754
ensure_linemap_for_file_and_line (file, line_num);
751755
location_t loc = linemap_position_for_column (&m_line_table, 0);
752756
return new_location (loc);
@@ -757,6 +761,9 @@ struct diagnostic_manager
757761
diagnostic_line_num_t line_num,
758762
diagnostic_column_num_t column_num)
759763
{
764+
if (m_debug_physical_locations)
765+
fprintf (stderr, "new_location_from_file_line_column (%s, %i, %i)",
766+
file->get_name (), line_num, column_num);
760767
ensure_linemap_for_file_and_line (file, line_num);
761768
location_t loc = linemap_position_for_column (&m_line_table, column_num);
762769
return new_location (loc);
@@ -767,12 +774,23 @@ struct diagnostic_manager
767774
const diagnostic_physical_location *loc_start,
768775
const diagnostic_physical_location *loc_end)
769776
{
777+
if (m_debug_physical_locations)
778+
fprintf (stderr, "new_location_from_range (%p, %p, %p)",
779+
(const void *)loc_caret,
780+
(const void *)loc_start,
781+
(const void *)loc_end);
770782
return new_location
771783
(m_line_table.make_location (as_location_t (loc_caret),
772784
as_location_t (loc_start),
773785
as_location_t (loc_end)));
774786
}
775787

788+
void
789+
set_debug_physical_locations (bool value)
790+
{
791+
m_debug_physical_locations = value;
792+
}
793+
776794
const diagnostic_logical_location *
777795
new_logical_location (enum diagnostic_logical_location_kind_t kind,
778796
const diagnostic_logical_location *parent,
@@ -874,11 +892,17 @@ struct diagnostic_manager
874892
linemap_add (&m_line_table, LC_ENTER, false, file->get_name (), 0);
875893
else
876894
{
877-
line_map *map
878-
= const_cast<line_map *>
879-
(linemap_add (&m_line_table, LC_RENAME_VERBATIM, false,
880-
file->get_name (), 0));
881-
((line_map_ordinary *)map)->included_from = UNKNOWN_LOCATION;
895+
line_map_ordinary *last_map
896+
= LINEMAPS_LAST_ORDINARY_MAP (&m_line_table);
897+
if (last_map->to_file != file->get_name ()
898+
|| linenum < last_map->to_line)
899+
{
900+
line_map *map
901+
= const_cast<line_map *>
902+
(linemap_add (&m_line_table, LC_RENAME_VERBATIM, false,
903+
file->get_name (), 0));
904+
((line_map_ordinary *)map)->included_from = UNKNOWN_LOCATION;
905+
}
882906
}
883907
linemap_line_start (&m_line_table, linenum, 100);
884908
}
@@ -888,11 +912,19 @@ struct diagnostic_manager
888912
{
889913
if (loc == UNKNOWN_LOCATION)
890914
return nullptr;
915+
if (m_debug_physical_locations)
916+
fprintf (stderr, ": new_location (%lx)", loc);
891917
if (diagnostic_physical_location **slot = m_location_t_map.get (loc))
892-
return *slot;
918+
{
919+
if (m_debug_physical_locations)
920+
fprintf (stderr, ": cache hit: %p\n", (const void *)*slot);
921+
return *slot;
922+
}
893923
diagnostic_physical_location *phys_loc
894924
= new diagnostic_physical_location (this, loc);
895925
m_location_t_map.put (loc, phys_loc);
926+
if (m_debug_physical_locations)
927+
fprintf (stderr, ": cache miss: %p\n", (const void *)phys_loc);
896928
return phys_loc;
897929
}
898930

@@ -909,6 +941,7 @@ struct diagnostic_manager
909941
const diagnostic *m_current_diag;
910942
const diagnostic_logical_location *m_prev_diag_logical_loc;
911943
std::unique_ptr<diagnostics::changes::change_set> m_change_set;
944+
bool m_debug_physical_locations;
912945
};
913946

914947
class impl_rich_location : public rich_location
@@ -1221,7 +1254,8 @@ struct diagnostic
12211254
m_level (level),
12221255
m_rich_loc (diag_mgr.get_line_table ()),
12231256
m_logical_loc (nullptr),
1224-
m_path (nullptr)
1257+
m_path (nullptr),
1258+
m_nesting_level (0)
12251259
{
12261260
m_metadata.set_lazy_digraphs (&m_graphs);
12271261
}
@@ -1325,6 +1359,9 @@ struct diagnostic
13251359
return m_graphs;
13261360
}
13271361

1362+
int get_nesting_level () const { return m_nesting_level; }
1363+
void set_nesting_level (int value) { m_nesting_level = value; }
1364+
13281365
private:
13291366
diagnostic_manager &m_diag_mgr;
13301367
enum diagnostic_level m_level;
@@ -1335,6 +1372,7 @@ struct diagnostic
13351372
std::vector<std::unique_ptr<range_label>> m_labels;
13361373
std::vector<std::unique_ptr<impl_rule>> m_rules;
13371374
std::unique_ptr<diagnostic_execution_path> m_path;
1375+
int m_nesting_level;
13381376
};
13391377

13401378
static enum diagnostics::kind
@@ -1624,8 +1662,9 @@ GCC_DIAGNOSTIC_PUSH_IGNORED(-Wsuggest-attribute=format)
16241662
GCC_DIAGNOSTIC_POP
16251663
info.m_metadata = diag.get_metadata ();
16261664
info.m_x_data = &diag;
1665+
m_dc.set_nesting_level (diag.get_nesting_level ());
16271666
diagnostic_report_diagnostic (&m_dc, &info);
1628-
1667+
m_dc.set_nesting_level (0);
16291668
m_dc.end_group ();
16301669
}
16311670

@@ -2972,3 +3011,23 @@ private_diagnostic_execution_path_add_event_3 (diagnostic_execution_path *path,
29723011

29733012
return as_diagnostic_event_id (result);
29743013
}
3014+
3015+
/* Public entrypoint. */
3016+
3017+
void
3018+
diagnostic_manager_set_debug_physical_locations (diagnostic_manager *mgr,
3019+
int value)
3020+
{
3021+
FAIL_IF_NULL (mgr);
3022+
mgr->set_debug_physical_locations (value);
3023+
}
3024+
3025+
/* Private entrypoint. */
3026+
3027+
void
3028+
private_diagnostic_set_nesting_level (diagnostic *diag,
3029+
int nesting_level)
3030+
{
3031+
FAIL_IF_NULL (diag);
3032+
diag->set_nesting_level (nesting_level);
3033+
}

‎gcc/libgdiagnostics.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1115,6 +1115,16 @@ diagnostic_node_set_label_via_msg_buf (diagnostic_node *node,
11151115
LIBGDIAGNOSTICS_PARAM_MUST_BE_NON_NULL (1)
11161116
LIBGDIAGNOSTICS_PARAM_CAN_BE_NULL (2);
11171117

1118+
/* If non-zero, print debugging information to stderr when
1119+
creating diagnostic_physical_location instances.
1120+
1121+
Added in LIBGDIAGNOSTICS_ABI_5. */
1122+
#define LIBDIAGNOSTICS_HAVE_diagnostic_manager_set_debug_physical_locations
1123+
1124+
extern void
1125+
diagnostic_manager_set_debug_physical_locations (diagnostic_manager *mgr,
1126+
int value);
1127+
11181128
/* DEFERRED:
11191129
- thread-safety
11201130
- plural forms

‎gcc/libgdiagnostics.map

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,3 +141,11 @@ LIBGDIAGNOSTICS_ABI_4 {
141141
# Private hook used by sarif-replay
142142
private_diagnostic_execution_path_add_event_3;
143143
} LIBGDIAGNOSTICS_ABI_3;
144+
145+
LIBGDIAGNOSTICS_ABI_5 {
146+
global:
147+
diagnostic_manager_set_debug_physical_locations;
148+
149+
# Private hook used by sarif-replay
150+
private_diagnostic_set_nesting_level;
151+
} LIBGDIAGNOSTICS_ABI_4;

‎gcc/libsarifreplay.cc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1408,10 +1408,22 @@ sarif_replayer::handle_result_obj (const json::object &result_obj,
14081408
rule_obj));
14091409
if (!msg_buf.m_inner)
14101410
return status::err_invalid_sarif;
1411+
14111412
auto note (m_output_mgr.begin_diagnostic (DIAGNOSTIC_LEVEL_NOTE));
14121413
note.set_location (physical_loc);
14131414
note.set_logical_location (logical_loc);
14141415
add_any_annotations (note, annotations);
1416+
1417+
/* Look for "nestingLevel" property, as per
1418+
"P3358R0 SARIF for Structured Diagnostics"
1419+
https://wg21.link/P3358R0 */
1420+
if (auto nesting_level
1421+
= maybe_get_property_bag_value<json::integer_number>
1422+
(*location_obj,
1423+
"nestingLevel"))
1424+
private_diagnostic_set_nesting_level (note.m_inner,
1425+
nesting_level->get ());
1426+
14151427
notes.push_back ({std::move (note), std::move (msg_buf)});
14161428
}
14171429
else

0 commit comments

Comments
 (0)