Re: [PATCH v4 2/8] libcpp: diagnostics: Support generated data in expanded locations

[Lewis Hyatt <lhyatt@gmail.com>]

On Fri, Aug 11, 2023 at 07:02:49PM -0400, David Malcolm wrote:
> On Wed, 2023-08-09 at 18:14 -0400, Lewis Hyatt wrote:
> > The previous patch in this series introduced the concept of LC_GEN line
> > maps. This patch continues on the path to using them to improve _Pragma
> > diagnostics, by adding a new source_id SRC member to struct
> > expanded_location, which is populated by linemap_expand_location. This
> > member allows call sites to detect and handle when a location refers to
> > generated data rather than a plain file name.
> > 
> > The previous FILE member of expanded_location is preserved (although
> > redundant with SRC), so that call sites which do not and never will care
> > about generated data do not need to be concerned about it. Call sites that
> > will care are modified here, to use SRC rather than FILE for comparing
> > locations.
> 
> Thanks; this seems like a good approach.
> 
> 
> [...snip...]
> 
> > diff --git a/gcc/edit-context.cc b/gcc/edit-context.cc
> > index 6f5bc6b9d8f..15052aec417 100644
> > --- a/gcc/edit-context.cc
> > +++ b/gcc/edit-context.cc
> > @@ -295,7 +295,7 @@ edit_context::apply_fixit (const fixit_hint *hint)
> >  {
> >    expanded_location start = expand_location (hint->get_start_loc ());
> >    expanded_location next_loc = expand_location (hint->get_next_loc ());
> > -  if (start.file != next_loc.file)
> > +  if (start.src != next_loc.src || start.src.is_buffer ())
> >      return false;
> >    if (start.line != next_loc.line)
> >      return false;
> 
> Thinking about fix-it hints, it makes sense to reject attempts to
> create fix-it hints within generated strings, as we can't apply them or
> visualize them.
> 
> Does anywhere in the patch kit do that?  Either of 
>   rich_location::maybe_add_fixit
> or
>   rich_location::reject_impossible_fixit
> would be good places to do that.
>

So rich_location::reject_impossible_fixit does reject them for _Pragmas now,
because what the frontend sees and passes to it is a virtual location, and it
always rejects virtual locations. But it doesn't reject arbitrary generated
data locations that may be created in an ordinary non-virtual location. I
think it's this one-line change to reject those:

-- >8 --

diff --git a/libcpp/line-map.cc b/libcpp/line-map.cc
index 835e8e1b8cd..382594637ad 100644
--- a/libcpp/line-map.cc
+++ b/libcpp/line-map.cc
@@ -2545,7 +2545,8 @@ rich_location::maybe_add_fixit (location_t start,
     = linemap_client_expand_location_to_spelling_point (next_loc,
                                                        LOCATION_ASPECT_START);
   /* They must be within the same file...  */
-  if (exploc_start.src != exploc_next_loc.src)
+  if (exploc_start.src != exploc_next_loc.src
+      || exploc_start.src.is_buffer ())
     {
       stop_supporting_fixits ();
       return;

-- >8 --

However, there are many selftests in diagnostic-show-locus.cc that actually
verify we generate the fixit hints for generated data, so I would need also to
change those to skip the test in this case as well. That looks like this:

-- >8 --

diff --git a/gcc/diagnostic-show-locus.cc b/gcc/diagnostic-show-locus.cc
index 62c60645e88..884c55e91e9 100644
--- a/gcc/diagnostic-show-locus.cc
+++ b/gcc/diagnostic-show-locus.cc
@@ -3824,6 +3824,8 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_)
   test_one_liner_simple_caret ();
   test_one_liner_caret_and_range ();
   test_one_liner_multiple_carets_and_ranges ();
+  if (!ltt.m_generated_data)
+    {
       test_one_liner_fixit_insert_before ();
       test_one_liner_fixit_insert_after ();
       test_one_liner_fixit_remove ();
@@ -3835,6 +3837,7 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_)
       test_one_liner_many_fixits_2 ();
       test_one_liner_labels ();
     }
+}

 /* Version of all one-liner tests exercising multibyte awareness.  For
    simplicity we stick to using two multibyte characters in the test, U+1F602
@@ -4419,6 +4422,8 @@ test_diagnostic_show_locus_one_liner_utf8 (const line_table_case &case_)
   test_one_liner_simple_caret_utf8 ();
   test_one_liner_caret_and_range_utf8 ();
   test_one_liner_multiple_carets_and_ranges_utf8 ();
+  if (!ltt.m_generated_data)
+    {
       test_one_liner_fixit_insert_before_utf8 ();
       test_one_liner_fixit_insert_after_utf8 ();
       test_one_liner_fixit_remove_utf8 ();
@@ -4428,6 +4433,7 @@ test_diagnostic_show_locus_one_liner_utf8 (const line_table_case &case_)
       test_one_liner_fixit_validation_adhoc_locations_utf8 ();
       test_one_liner_many_fixits_1_utf8 ();
       test_one_liner_many_fixits_2_utf8 ();
+    }
   test_one_liner_labels_utf8 ();
   test_one_liner_colorized_utf8 ();
 }
@@ -5726,15 +5732,15 @@ diagnostic_show_locus_cc_tests ()
   for_each_line_table_case (test_diagnostic_show_locus_one_liner, true);
   for_each_line_table_case (test_diagnostic_show_locus_one_liner_utf8, true);
   for_each_line_table_case (test_add_location_if_nearby, true);
-  for_each_line_table_case (test_diagnostic_show_locus_fixit_lines, true);
-  for_each_line_table_case (test_fixit_consolidation, true);
-  for_each_line_table_case (test_overlapped_fixit_printing, true);
-  for_each_line_table_case (test_overlapped_fixit_printing_utf8, true);
-  for_each_line_table_case (test_overlapped_fixit_printing_2, true);
-  for_each_line_table_case (test_fixit_insert_containing_newline, true);
-  for_each_line_table_case (test_fixit_insert_containing_newline_2, true);
-  for_each_line_table_case (test_fixit_replace_containing_newline, true);
-  for_each_line_table_case (test_fixit_deletion_affecting_newline, true);
+  for_each_line_table_case (test_diagnostic_show_locus_fixit_lines, false);
+  for_each_line_table_case (test_fixit_consolidation, false);
+  for_each_line_table_case (test_overlapped_fixit_printing, false);
+  for_each_line_table_case (test_overlapped_fixit_printing_utf8, false);
+  for_each_line_table_case (test_overlapped_fixit_printing_2, false);
+  for_each_line_table_case (test_fixit_insert_containing_newline, false);
+  for_each_line_table_case (test_fixit_insert_containing_newline_2, false);
+  for_each_line_table_case (test_fixit_replace_containing_newline, false);
+  for_each_line_table_case (test_fixit_deletion_affecting_newline, false);
   for_each_line_table_case (test_tab_expansion, true);
   for_each_line_table_case (test_escaping_bytes_1, true);
   for_each_line_table_case (test_escaping_bytes_2, true);

-- >8 --

(The above diff was with -w to avoid a lot of useless indent changes, just for
illustration what it does.)

> 
> [...snip...]
> 
> > diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> > index e59123b18c5..76617fe6129 100644
> > --- a/libcpp/include/line-map.h
> > +++ b/libcpp/include/line-map.h
> > @@ -1410,18 +1410,22 @@ linemap_location_before_p (class line_maps *set,
> >  
> >  typedef struct
> >  {
> > -  /* The name of the source file involved.  */
> > -  const char *file;
> > +  /* The file name of the location involved, or NULL if the location
> > +     is not in an external file.  */
> > +  const char *file = nullptr;
> >  
> > -  /* The line-location in the source file.  */
> > -  int line;
> > -
> > -  int column;
> > +  /* A source_id recording the file name and/or the in-memory content,
> > +     as appropriate.  Users that need to handle in-memory content need
> > +     to use this rather than FILE.  */
> > +  source_id src;
> >  
> > -  void *data;
> > +  /* The line-location in the source file.  */
> > +  int line = 0;
> > +  int column = 0;
> > +  void *data = nullptr;
> >  
> > -  /* In a system header?. */
> > -  bool sysp;
> > +  /* In a system header?  */
> > +  bool sysp = false;
> >  } expanded_location;
> 
> I don't know if we've been using default member initialization yet, but
> apparently it's C++11, and thus OK.
>

Thanks, I feel like it does make things more maintainable. FWIW, I did verify
it builds with gcc 4.8.5.

> [...snip...]
> 
> 
> This patch looks good to me, but obviously it has dependencies on the
> rest of the kit.
> 
> Dave
>

Thank you, please let me know if I should also apply the above tweaks or no?