Re: [RFC/PATCH] More precise diagnostic locations: dynamic locations for columns vs explicit offset

[Dodji Seketeli <dodji at redhat dot com>]

Manuel LÃpez-IbÃÃez <lopezibanez@gmail.com> a Ãcrit:

> In some situations, we would like to point to a location which was not
> encoded when tokenizing. This happens, for example, in two prominent
> cases:
>
> 1) To get precise locations within strings
> (https://gcc.gnu.org/PR52952) for example, for Wformat warnings.

This feature would be very welcome indeed.

>
> 2) In the Fortran FE, which gives quite precise location information
> by tracking the characters that it wants to warn about instead of
> relying on the line-map machinery.

So with this feature, the Fortran FE would then use the then more
"generic" diagnostics machinery, right?

> The most straightforward way to implement this is by adding variants
> of diagnostic functions that take an explicit "offset" argument and
> pass this offset through the whole diagnostics machinery. This is what
> I implemented in the patch format_offset.diff attached. The downside
> is that we would need to add even more variants (with/without offset)
> of various diagnostic functions and track the offset/no-offset cases
> explicitly.

I would be inclined to go for this route at first sight because of its
conceptual simplicity, even if it might be heavy in terms of the
number of entry points to maintain for users of the diagnostics
sub-system but then ...

> The nicer/cleaner alternative is to somehow (re)compute a single
> location value from a given location plus the new offset.

... I agree with this.  It's more elegant and maintainable to go this
way.  But it might involve some hair splitting.


> This is what I implemented in patch fortran-diagnostics-part3.diff
> in linemap_redo_position_for_column(). As far as I understand, this
> method only works reliably if the location+offset does not jump to a
> different line map, that is, if to_column < (1u <<
> map->d.ordinary.column_bits). Otherwise, we may need to recompute
> all successive line-maps to accommodate the new location. The best
> way to do the latter (or to work-around that issue) is not clear to
> me at the moment.

I think it might be more involved than that.

There are two kinds of locations:

 1/ spelling locations.  They represent a real point in the source
 code.  For now, the beginning of a token.

 2/ virtual locations.  They are an abstract number, calculated in a
 convoluted way to encode the fact that a given token (rather, the
 location of that token) was e.g, possibly passed to a function-like
 macro that used it in its expansion, and that macro was expanded
 somewhere.  And from that number, we can get back to the macro into
 which it was used, expanded, where the macro was expanded and we can
 also get the original spelling location of the token we are looking
 at.

I might be maybe missing something, but if the location is not virtual
(case 1/), I *think* that in practice we are not likely to see that
location + column jumps to the "next" map, unless we are running low
on line maps space -- in which case, either columns tracking or even
line maps are turned off -- or the token we are looking at it
is *huge*.  In the later case, when we start tracking the location of
the *end* of tokens (as said in the roadmap), I think that later issue
is going to vanish because a given line map is going to be allocated big
enough to contain locations until at least the end of the last token
it "contains".

If the location is virtual (case 2/), then the "location + offset"
value you are referring to is meaningless, unfortunately.  You must
get back to to the spelling location of that token first; that is, you
have to consider "spelling_location(location) + offset", and we are
back to the first case (case 1/).


> Thus, I am putting forward these two alternative implementations and
> seeking comments/advice/help in deciding what would be the best way to
> fix this key missing piece of GCC diagnostics.

Thanks.

> Related to this, perhaps I should make a more general call for help.
> Despite the heroic, constant torrent of diagnostic fixes by Paolo,
> Marek and others, I have not seen much progress on the key
> infrastructure issues in the roadmap
> (https://gcc.gnu.org/wiki/Better_Diagnostics). We have had at least
> one major item per release since GCC 4.5, but I don't see any
> particular item being tackled for GCC 5.0. Are you planning to tackle
> any of them?

Unfortunately, it's unlikely that I'll have time to tackle any of
this.  I am quite busy on libabigail
(http://https://sourceware.org/libabigail/) in this cycle.  And it's
also important for us.  So I'd rather shoot for the next cycle.

But that shouldn't prevent interested hackers to jump in :-)

> I have a simple patch to implement Fix-it hints but it needs more
> work. Unfortunately, I have very little free time to dedicate to GCC
> nowadays, so I'm afraid I might not even be able to finish this in
> time. Any item in that list would be a nice major feature for GCC
> 5.0.

Thank you for the effort you are putting in this, despite your tight
schedule.  This is really appreciated.

> Perhaps we need to ask for help in gcc/gcc-help or some other forum.

Yes, please do so.

[...]

> [3. text/plain; fortran-diagnostics-part3.diff]

[...]

> Index: gcc/fortran/error.c
> ===================================================================
> --- gcc/fortran/error.c	(revision 214251)
> +++ gcc/fortran/error.c	(working copy)
> @@ -956,10 +956,67 @@ gfc_warning_now (const char *gmsgid, ...
>      gfc_increment_error_count();
>  
>    buffer_flag = i;
>  }
>  
> +/* Encode and return a source_location from a column number. The
> +   source line considered is the last source line used to call
> +   linemap_line_start, i.e, the last source line which a location was
> +   encoded from.  */
> +
> +source_location
> +linemap_redo_position_for_column (struct line_maps *set, source_location loc,
> +                                  unsigned int to_column)
> +{

I would put this kind of function in the line-map module, rather than
here.


> +  const struct line_map * map = linemap_lookup (set, loc);
> +  gcc_assert(to_column < (1u << map->d.ordinary.column_bits));

To write that, you must be sure that map is an ordinary map.  That is,
one that encodes non-virtual locations; IOW, that loc is a non-virtual
location.  Otherwise, you'd need to resolve loc to its associated
spelling location and get the map for that spelling location.

Also, to make sure that loc + to_column belong to this map, a better
assertion I could think of would be:

    gcc_assert(MAP_START_LOCATION(map[1]) < loc + to_column);

This is because by design, locations emitted by a given map are
smaller than MAP_START_LOCATION of the next map.  In the particular
case of 'map' being the last map of the set of maps, I guess we can
and should do away with the assert completely.

More generally, I'd rather use:

    linemap_position_for_line_and_column(map,
					 SOURCE_LINE(map, loc),
					 column)

instead of doing (loc + to_column) to get to the new location.

[...]

> +  gcc_assert (map == linemap_lookup (set, r));

OK.

> +  return r;
> +}

[...]

I hope this helps.

Cheers,

-- 
		Dodji