v4 of diagnostic_show_locus and rich_location

On Sun, 2015-09-27 at 02:55 +0200, Dodji Seketeli wrote:
> [Note to libcpp, C, and Fortran maintainers: we still need your input :-)]

Updated version of patch attached (v4); a diff relative to v3 can be
seen at:
https://dmalcolm.fedorapeople.org/gcc/2015-10-12/0003-Eliminate-special-casing-for-Fortran.patch

v4 eliminates the lingering parts of the old implementation of
diagnostic_show_locus, porting the Fortran frontend to use the new
implementation.

In the process I discovered an issue with the Fortran frontend: some of
the caret locations appear to have an off-by-one error.
For example, in gcc/testsuite/gfortran.dg/associate_5.f03, the old
implementation would issue this diagnostic:

associate_5.f03:33:6:

       y = 5 ! { dg-error "variable definition context" }
      1
associate_5.f03:32:20:

     ASSOCIATE (y => x) ! { dg-error "variable definition context" }
                    2
Error: Associate-name ‘y’ can not appear in a variable definition
context (assignment) at (1) because its target at (2) can not, either

Note how the carets 1 and 2 appear one column before the "y" and the "x"
that they refer to.

This seems to be a pre-existing bug in the Fortran FE, which I've now
filed as PR fortran/67936.

On porting the Fortran FE to fully use the new implementation of
diagnostic_show_locus, I found that the "1" caret in the above
disappeared, due to v3 of the layout printer suppressed carets and
underlines appearing within the leading whitespace before the text in
its line.  So I updated that to only suppress underlines in such a
location, and not carets, to ensure that we at least faithfully print
both carets, at the given (erroneous) locations.   I added test coverage
for this (test_caret_on_leading_whitespace).

The existing Fortran testcase for diagnostics with multiple locations
don't seem to verify the -fdiagnostics-show-caret case; I visually
inspected the results, but perhaps we could add some automated test
coverage there using the dg-{begin|end}-multiline directives from
earlier in this kit (which is now in trunk).  I don't know if adding
such test coverage is necessary for acceptance of this patch though.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.  OK for
trunk?

Some other comments inline.

> Hello,
> 
> David Malcolm <dmalcolm@redhat.com> writes:
> 
> [...]
> 
> > Here's the revised comment I put in the attached patch:
> 
> [...]
> 
> > +   The class caches the lookup of the color codes for the above.
> > +
> > +   The class also has responsibility for tracking which of the above is
> > +   active, filtering out unnecessary changes.  This allows layout::print_line
> > +   to simply request a colorization code for *every* character it prints
> > +   thorough this class, and have the filtering be done for it here.
> 
> You probably meant "*through* this class" ?

Yes, thanks.  Fixed.

> > */
> 
> > Hopefully that comment explains the possible states the colorizer can
> > have.
> 
> Yes it does, great comment, thank you.
> 
> 
> > FWIW I have a follow-up patch to add support for fix-it hints, so they
> > might be another kind of colorization state.
> > (see https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00732.html for the
> > earlier version of said patch, in v1 of the kit).
> 
> Yeah, I'll comment on that one separatly.
> 
> >> Also, I am thinking that there should maybe be a layout::state type,
> >> which would have two notional properties (for now): range_index and
> >> draw_caret_p. So that this function:
> >> 
> >> +bool
> >> +layout::get_state_at_point (/* Inputs.  */
> >> +			    int row, int column,
> >> +			    int first_non_ws, int last_non_ws,
> >> +			    /* Outputs.  */
> >> +			    int *out_range_idx,
> >> +			    bool *out_draw_caret_p)
> >> 
> >> Would take just one output parameter, e.g, a reference to
> >> layout::state.
> >
> > Fixed, though I called it "struct point_state", given that it's coming
> > from get_state_at_point.  I passed it by pointer, since AFAIK our coding
> > standards don't yet approve of the use of references in the codebase
> > (outside of places where we need them e.g. container classes).
> 
> Great.  Thanks.
> 
> >
> > I also added a unit test for a rich_location with two caret locations
> > (mimicking one of the Fortran examples), to give us coverage for this
> > case:
> >
> > +void test_multiple_carets (void)
> > +{
> > +#if 0
> > +   x = x + y /* { dg-warning "8: test" } */
> > +/* { dg-begin-multiline-output "" }
> > +    x = x + y
> > +        A   B
> > +   { dg-end-multiline-output "" } */
> > +#endif
> > +}
> >
> > where the "A" and "B" as caret chars are coming from new code in the
> > show_locus unittest plugin.
> 
> Yeah, saw that.  Excellent, thanks.
> 
> [...]
> 
> >> +  if (0)
> >> +    show_ruler (context, line_width, m_x_offset);
> >> 
> >> This should probably be removed from the final code to be committed.
> >
> > FWIW, the ruler is very helpful to me when debugging the locus-printing
> > (e.g. when adding fix-it-hints), and if we remove that if (0) call, we
> > get:
> >
> > warning: ‘void show_ruler(diagnostic_context*, int, int)’ defined but
> > not used [-Wunused-function]
> >
> > which will break bootstrap, so perhaps it instead should be an option?
> > "-fdiagnostics-show-ruler" or somesuch?
> >
> > I don't know that it would be helpful to end-users though.
> >
> > I'd prefer to just keep it in the code with the
> >   if (0)
> > as-is, since it's useful "scaffolding" for hacking on the code.
> >
> 
> OK, I understand; though, as Manuel noted elsewhere, you might rename
> that function debug_show_ruler and declare it as:
> 
>     DEBUG_FUNCTION static void
>     debug_show_ruler (diagnostic_context *context, int max_width, int x_offset)
>     {
>       /* ...  */
>     }
> to comply with that is generally done in the compiler.

FWIW. I didn't do this.  The function is probably only meaningful to
call from where I was calling it, not necessarily from gdb.

> [...]
> 
> >> +/* Get the column beyond the rightmost one that could contain a caret or
> >> +   range marker, given that we stop rendering at trailing whitespace.  */
> >> +
> >> +int
> >> +layout::get_x_bound_for_row (int row, int caret_column,
> >> +			     int last_non_ws)
> >> 
> >> Please describe what the parameters mean here, especially last_non_ws.
> >> I had to read its code to know that last_non_ws was the *column* of
> >> the last non white space character.
> >
> > I renamed it to "last_non_ws_column", and fleshed out the comment
> 
> OK.
> 
> [...]
> 
> >>  void
> >>  diagnostic_show_locus (diagnostic_context * context,
> >>  		       const diagnostic_info *diagnostic)
> >> @@ -75,16 +710,25 @@ diagnostic_show_locus (diagnostic_context * context,
> >>      return;
> >> 
> >> +      /* The GCC 5 routine. */
> >> 
> >> I'd say the GCC <= 5 routine ;-)
> >
> >> +  else
> >> +    /* The GCC 6 routine.  */
> >> 
> >> And here, the GCC > 5 routine.
> >
> > Changed to "GCC < 6" and "GCC >= 6", on the pedantic grounds that e.g.
> > 5.1 > 5
> 
> OK.
> 
> >
> >> I would be surprised to see this patch in particular incur any
> >> noticeable increase in time and space consumption, but, have you noticed
> >> anythying related to that during bootstrap?
> >
> > I hadn't noticed it, but I wasn't timing.  I'll have a look.
> 
> Ok, thanks.
> 
> > One possible nit here is that the patch expands locations when
> > constructing rich_location instances, and it does that for warnings
> > before the logic to ignore them.  So there may be some extra calls there
> > that aren't present in trunk, for discarded warnings.  I don't expect
> > that to affect the speed of the compiler though (I expect it to be lost
> > in the noise).
> 
> Fair enough.
> 
> > OK for trunk if it passes bootstrap/regrtest?
> 
> The core diagnostics bits are IMHO in good shape.  I'd like to see the
> discussion with Manuel be resolved before it goes in, though.  And we
> still need the "Go" from the FE maintainers.
> 
> So I am continuing the discussion with Manuel below.
> 
> Manuel López-Ibáñez <lopezibanez@gmail.com> writes:
> 
> > On 25 September 2015 at 23:15, David Malcolm <dmalcolm@redhat.com> wrote:
> >> My recollection is that I saw that the Fortran frontend has logic for
> >> calling into diagnostic_print_caret_line, noticed that the fortran
> >> testsuite has dg- assertions about finding specific messages, and I got
> >> worried that they embed assumptions about how the old printer worked.
> >> Hence I wanted to avoid touching that for the first version, and so in
> >> this patch it's a hybrid of the old Fortran printing code with the new
> >> representation for multiple locations.
> >
> > It is quite simple, one you understand the logic. Fortran has three
> > types of output:
> >
> > (a) #     [name]:[locus]:
> >     #
> >     #        some code
> >     #              1
> >     #     Error: Some error at (1)
> >
> > which can call the same function used by other FEs to print the caret
> > line (I call the caret line, the line that contains the caret
> > character/ranges, 1 in this case).
> >
> > (b) #     [name]:[locus]:
> >     #
> >     #       some code and some more code
> >     #              1       2
> >     #     Error: Some error at (1) and (2)
> >
> >
> > which according to what you explained should also be possible by
> > calling diagnostic_show_locus with the appropriate location info and
> >
> > (c) #     [name]:[locus]:
> >     #
> >     #       some code
> >     #              1
> >     #     [name]:[locus2]:
> >     #
> >     #       some other code
> >     #         2
> >     #     Error: Some error at (1) and (2)
> >     # or
> >
> > which was implemented by calling diagnostic_show_locus with just the
> > location of 1, then calling diagnostic_print_caret_line with just the
> > expanded_location of 2. I could have just called diagnostic_show_locus
> > also to print 2 by overriding diagnostic->location[0] =
> > diagnostic->location[1] and caret_char[0] = caret_char[1], but that
> > seemed a bit hackish and more expensive (but perhaps less confusing?).
> >
> > If you have a function that you can call with one or more
> > location_t/expanded_location  (or something that can be converted from
> > a location_t) and pass explicitly the caret_char, then you just need
> > to call that function with the right parameters to get the second part
> > of (c).

That's what I've ended up going with.

> Otherwise, you may simply temporarily do caret_char[0] =
> > caret_char[1], before calling the same function that prints the
> > caret-line for (a).
> >
> >> Maybe that's a cop-out.  Would you prefer that the patch goes all the
> >> way, and that I attempt to eliminate all calls to
> >> diagnostic_print_caret_line from the Fortran FE, and eliminate the old
> >> implementation?  (either now, or as a followup patch?)  I may need
> >> assistance with that; I suspect that some of the dg- assertions in the
> >> Fortran test suite may need updating.
> >
> > There is only one call! I just think this hack is really not necessary
> > (in fact, it seems more complicated than the alternatives outlined
> > above).
> 
> I agree that David's approach of adding an impedance adaptation layer in
> gcc/diagnostic-show-locus.c to keep the Fortran FE unchanged *for now*
> complicates diagnostic-show-locus.c.  But then the benefit is to
> break-up the tasks at hand in smaller (less coupled) tasks, making the
> whole process more manageable.
> 
> IOW, I think the Fortran FE can then be updated later in a follow-up
> patch, and then the adaptation layer code can be removed from
> diagnostic-show-locus.c *if* we all agree that the Fortran FE can be
> adapted to use the foundation being put in place in this current patch.
> 
> And I think all the functionalities the Fortran FE needs for that
> matter, are supported by this patch.  Please correct me if I am wrong.
> 
> > And I'm afraid that once it goes in, it will stay there forever.
> 
> :-)
> 
> Or maybe not :-)
> 
> It looks we are in motion (with some good energy) to change these things
> at the moment, thanks to David and you, amongst others.  So I would
> agree to bet on a positive outcome of this, and to let the patch go in.
> I guess if the Fortran FE doesn't get updated afterwards, I'll be the
> one on the hook; I'd take the bet nevertheless.
> 
> > You are in a far better position than the Fortran devs to understand
> > how to call your new interfaces to get the output you desire.
> 
> That is correct.  And hopefully, letting the current patch go in won't
> prevent the further needed changes of the FEs to happen, including the
> Fortran one.
> 
> [...]

v4 of the patch does the conversion of Fortran, and eliminates the
adaptation layer.  No partial transitions here!

As noted above, a diff relative to v3 can be seen at:
https://dmalcolm.fedorapeople.org/gcc/2015-10-12/0003-Eliminate-special-casing-for-Fortran.patch

Manu: I hope this addresses your concerns.

> Manuel López-Ibáñez <lopezibanez@gmail.com> writes:
> 
> > On 25 September 2015 at 23:24, David Malcolm <dmalcolm@redhat.com> wrote:
> >> On Fri, 2015-09-25 at 23:13 +0200, Manuel López-Ibáñez wrote:
> >>> +   If SHOW_CARET_P is true, then the range should be rendered with
> >>> +   a caret at its starting location.  This
> >>> +   is for use by the Fortran frontend, for implementing the
> >>> +   "%C" and "%L" format codes.  */
> >>> +
> >>> +void
> >>> +rich_location::set_range (unsigned int idx, source_range src_range,
> >>> +              bool show_caret_p, bool overwrite_loc_p)
> >>>
> >>> I do not understand when is this show_caret_p used by Fortran given
> >>> the diagnostic_show_locus code mentioned earlier.
> >
> > [...]
> >> rich_location::set_range exists to ensure that the %C and %L codes used
> >> by Fortran (and "+" in the C family of FEs) can write back into the
> >> rich_location instance, faithfully emulating the old code that wrote
> >> back to
> >> struct text_info's:
> >>   location_t locations[MAX_LOCATIONS_PER_MESSAGE];
> >
> > Why Fortran cannot use text->set_location like the other FEs? This way
> > you do not need set_range at all.
> 
> Yes, it does look to me that this change:
> 
>     @@ -938,10 +939,12 @@ gfc_format_decoder (pretty_printer *pp,
>             /* If location[0] != UNKNOWN_LOCATION means that we already
>                processed one of %C/%L.  */
>             int loc_num = text->get_location (0) == UNKNOWN_LOCATION ? 0 : 1;
>     -	text->set_location (loc_num,
>     -			    linemap_position_for_loc_and_offset (line_table,
>     -								 loc->lb->location,
>     -								 offset));
>     +	source_range range
>     +	  = source_range::from_location (
>     +	      linemap_position_for_loc_and_offset (line_table,
>     +						   loc->lb->location,
>     +						   offset));
>     +	text->set_range (loc_num, range, true);
> 
> is unnecessary because text_info::set_location() got updated as:
> 
>     @@ -40,21 +35,17 @@ struct text_info
>        va_list *args_ptr;
>        int err_no;  /* for %m */
>        void **x_data;
>     +  rich_location *m_richloc;
> 
>     -  inline void set_location (unsigned int index_of_location, location_t loc)
>     +  inline void set_location (unsigned int idx, location_t loc, bool caret_p)
>        {
>     -    gcc_checking_assert (index_of_location < MAX_LOCATIONS_PER_MESSAGE);
>     -    this->locations[index_of_location] = loc;
>     +    source_range src_range;
>     +    src_range.m_start = loc;
>     +    src_range.m_finish = loc;
>     +    set_range (idx, src_range, caret_p);
>        }
> 
> So what is needed AIUI is to just add a 'caret_p' argument to the
> initial call to:
> 
>         text->set_location (loc_num,
>     			    linemap_position_for_loc_and_offset (line_table,
>     								 loc->lb->location,
>     								 offset));
> 
> > The other issue that confuses me is that show_caret_p is always true
> > when reaching this function via the pretty-printer. Thus, show_caret_p
> > is also used by C/C++. In fact, I'm not sure when it can be false.
> 
> I think the reason why show_caret_p is always true here is that the
> patch updates the existing FE code so that it keeps emitting the *same*
> diagnostics as before.  And today, FEs emits diagnostics with ranges
> that always starts with a caret.
> 
> But I think it's easy to imagine hypothetic (and reasonably probable)
> examples where we'd have only one range with a caret, associated to other
> ranges with no caret.  The ranges with no carets would thus require the
> show_caret_p argument to be non-null.  In fact, patch 2/5 has one of
> these examples in a comment in line-map.h:
> 
>     +   Example C
>     +   *********
>     +      a = (foo && bar)
>     +           ~~~ ^~ ~~~
>     +   This rich location has three ranges:
>     +   - Range 0 has its caret and start location at the first "&" and
>     +     end at the second "&.
>     +   - Range 1 has its start and finish at the "f" and "o" of "foo";
>     +     the caret is not flagged for display, but is perhaps at the "f"
>     +     of "foo".
>     +   - Similarly, range 2 has its start and finish at the "b" and "r" of
>     +     "bar"; the caret is not flagged for display, but is perhaps at the
>     +     "b" of "bar".
> 
> We'll then start seeing cases with show_caret_p being false when *new*
> diagnostics are added to FEs to leverage on this new feature.
> 
> So I don't think this "show_caret_p" argument's value is an issue, quite
> the contrary; it's an interesting feature.
> 
> Cheers,

Thanks for the review and other comments.
Dave

v4 of diagnostic_show_locus and rich_location

Commit Message

Comments

Patch