Re: [PATCH] c++: fix source printing for "required from here" message

[Patrick Palka <ppalka@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7749 bytes --]

On Thu, 25 Apr 2024, David Malcolm wrote:

> On Wed, 2024-04-24 at 17:05 -0400, Patrick Palka wrote:
> > On Wed, 24 Apr 2024, Jason Merrill wrote:
> > 
> > > On 4/24/24 13:22, Patrick Palka wrote:
> > > > Tested on x86_64-pc-linux-gnu, full bootstrap+regtest in
> > > > progress,
> > > > does this look OK if successful?
> > > > 
> > > > -- >8 --
> > > > 
> > > > It seems the diagnostic machinery's source line printing respects
> > > > the pretty printer prefix, but this is undesirable for the call
> > > > to
> > > > diagnostic_show_locus in print_instantiation_partial_context_line
> > > > added in r14-4388-g1c45319b66edc9 since the prefix may have been
> > > > set when issuing an earlier, unrelated diagnostic and we just
> > > > want
> > > > to print an unprefixed source line.
> > > > 
> > > > This patch naively fixes this by clearing the prefix before
> > > > calling
> > > > diagnostic_show_locus.
> > > > 
> > > > Before this patch, for error60a.C below we'd print
> > > > 
> > > > gcc/testsuite/g++.dg/template/error60a.C: In function ‘void
> > > > usage()’:
> > > > gcc/testsuite/g++.dg/template/error60a.C:24:3: error:
> > > > ‘unrelated_error’ was
> > > > not declared in this scope
> > > >     24 |   unrelated_error; // { dg-error "not declared" }
> > > >        |   ^~~~~~~~~~~~~~~
> > > > gcc/testsuite/g++.dg/template/error60a.C: In instantiation of
> > > > ‘void
> > > > test(Foo) [with Foo = int]’:
> > > > gcc/testsuite/g++.dg/template/error60a.C:25:13:   required from
> > > > here
> > > > gcc/testsuite/g++.dg/template/error60a.C:24:3: error:    25 |  
> > > > test<int>
> > > > (42); // { dg-message " required from here" }
> > > > gcc/testsuite/g++.dg/template/error60a.C:24:3: error:       |
> > > > ~~~~~~~~~~^~~~
> > > > gcc/testsuite/g++.dg/template/error60a.C:19:24: error: invalid
> > > > conversion
> > > > from ‘int’ to ‘int*’ [-fpermissive]
> > > >     19 |   my_pointer<Foo> ptr (val); // { dg-error "invalid
> > > > conversion from
> > > > 'int' to 'int\\*'" }
> > > >        |                        ^~~
> > > >        |                        |
> > > >        |                        int
> > > > gcc/testsuite/g++.dg/template/error60a.C:9:20: note:  
> > > > initializing argument
> > > > 1 of ‘my_pointer<Foo>::my_pointer(Foo*) [with Foo = int]’
> > > >      9 |   my_pointer (Foo *ptr) // { dg-message " initializing
> > > > argument 1"
> > > > }
> > > >        |               ~~~~~^~~
> > > > 
> > > > and afterward we print
> > > > 
> > > > gcc/testsuite/g++.dg/template/error60a.C: In function ‘void
> > > > usage()’:
> > > > gcc/testsuite/g++.dg/template/error60a.C:24:3: error:
> > > > ‘unrelated_error’ was
> > > > not declared in this scope
> > > >     24 |   unrelated_error; // { dg-error "not declared" }
> > > >        |   ^~~~~~~~~~~~~~~
> > > > gcc/testsuite/g++.dg/template/error60a.C: In instantiation of
> > > > ‘void
> > > > test(Foo) [with Foo = int]’:
> > > > gcc/testsuite/g++.dg/template/error60a.C:25:13:   required from
> > > > here
> > > >     25 |   test<int> (42); // { dg-message " required from here"
> > > > }
> > > >        |   ~~~~~~~~~~^~~~
> > > > gcc/testsuite/g++.dg/template/error60a.C:19:24: error: invalid
> > > > conversion
> > > > from ‘int’ to ‘int*’ [-fpermissive]
> > > >     19 |   my_pointer<Foo> ptr (val); // { dg-error "invalid
> > > > conversion from
> > > > 'int' to 'int\\*'" }
> > > >        |                        ^~~
> > > >        |                        |
> > > >        |                        int
> > > > gcc/testsuite/g++.dg/template/error60a.C:9:20: note:  
> > > > initializing argument
> > > > 1 of ‘my_pointer<Foo>::my_pointer(Foo*) [with Foo = int]’
> > > >      9 |   my_pointer (Foo *ptr) // { dg-message " initializing
> > > > argument 1"
> > > > }
> > > >        |               ~~~~~^~~
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > >         * error.cc (print_instantiation_partial_context_line):
> > > > Clear
> > > >         context->printer->prefix around the call to
> > > > diagnostic_show_locus.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 
> > > >         * g++.dg/concepts/diagnostic2.C: Expect source line
> > > > printed
> > > >         for the required from here message.
> > > >         * g++.dg/template/error60a.C: New test.
> > > > ---
> > > >   gcc/cp/error.cc                             |  2 +
> > > >   gcc/testsuite/g++.dg/concepts/diagnostic2.C |  6 ++-
> > > >   gcc/testsuite/g++.dg/template/error60a.C    | 46
> > > > +++++++++++++++++++++
> > > >   3 files changed, 53 insertions(+), 1 deletion(-)
> > > >   create mode 100644 gcc/testsuite/g++.dg/template/error60a.C
> > > > 
> > > > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
> > > > index 7074845154e..a7067d4d2ed 100644
> > > > --- a/gcc/cp/error.cc
> > > > +++ b/gcc/cp/error.cc
> > > > @@ -3793,7 +3793,9 @@ print_instantiation_partial_context_line
> > > > (diagnostic_context *context,
> > > >                    : _("required from here\n"));
> > > >       }
> > > >     gcc_rich_location rich_loc (loc);
> > > > +  char *saved_prefix = pp_take_prefix (context->printer);
> > > >     diagnostic_show_locus (context, &rich_loc, DK_NOTE);
> > > > +  context->printer->prefix = saved_prefix;
> > > 
> > > I would follow the pattern of c_diagnostic_finalizer here, i.e.
> > > using
> > > pp_set_prefix for the restore.
> > 
> > FWIW that's what I originally went with, but I don't really
> > understand
> > the other things pp_set_prefix does besides setting the prefix and
> > then I noticed cp_print_error_function restores ->prefix directly so
> > I ended up doing that instead.
> 
> I have a slight preference for using pp_set_prefix for the restore, but
> the patch as written is also OK; thanks.

Thanks a lot, pushed as r15-4-g7d5479a2ecf630 which uses pp_set_prefix
for the restore as suggested.

> 
> I confess that I don't have a strong sense of how the prefix code is
> meant to work.
> 
> It seems to be for use with this combination of options:
>   -fmessage-length=NON_ZERO -fdiagnostics-show-location=every-line
> which triggers line-wrapping, adding the prefix at each line when line
> wrapping occurs.  This seems to have existed at least as far back as 
> 856b62442f6fc5e4302ae9ee1ebce8a19bbd8681
> re this "C++ err msgs" thread from 2000:
> 
> https://gcc.gnu.org/pipermail/libstdc++/2000-May/004650.html
> https://gcc.gnu.org/pipermail/libstdc++/2000-May/004664.html
> https://gcc.gnu.org/pipermail/gcc-patches/2000-May/031143.html
> 
> which made the prefixing optional on continuation lines.
> 
> I suspect that if anyone was using that combination of options, we've
> probably broken it at some point with quoting of source code,
> underlining, fix-it hints, etc etc
> 
> Dave
> 
> 
> 
> 
> > 
> > > 
> > > Though I think the pp_set_prefix to NULL in c_diagnostic_finalizer
> > > is
> > > redundant and should have been removed in r9-2240-g653fee1961981f
> > > when the
> > > previous line changed from _set_ to _take_.  If it isn't redundant,
> > > then it
> > > should be, i.e. pp_take_prefix should call it instead of directly
> > > setting
> > > NULL.
> > > 
> > > Some _take_ callers do set(NULL) and others don't; they should
> > > definitely be
> > > consistent one way or the other.
> > > 
> > > David, what do you think?
> > > 
> > > Jason
> > > 
> 
>