Re: [PATCH 1/6] Linemap infrastructure for virtual locations

[Dodji Seketeli <dodji@redhat.com>]

Hello,

Sorry for getting back to this just now, and thank you very much for the
review.  Please find below my reply to your comments.

Tom Tromey <tromey@redhat.com> writes:

> Dodji>    expanded_location xloc;
> Dodji>    if (loc <= BUILTINS_LOCATION)
> Dodji> -    {
> Dodji> -      xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
> Dodji> +    {      
> Dodji> +      /* If LOC is UNKNOWN_LOCATION and there was a map allocated for
> Dodji> +	 it then XLOC.FILE should be the file path of the map,
> Dodji> +	 i.e. the file path of the main file being compiled.
> Dodji> +	 Otherwise [if we are being called before any line map has
> Dodji> +	 been allocated, e.g. during parsing of commande line
> Dodji> +	 options] if no line map has been allocated yet, then
> Dodji> +	 XLOC.FILE should just be NULL.  */
> Dodji> +      if (loc == UNKNOWN_LOCATION)
> Dodji> +	{
> Dodji> +	  const struct line_map *map =
> Dodji> +	    linemap_lookup (line_table, UNKNOWN_LOCATION);
> Dodji> +	  xloc.file = map != NULL ? LINEMAP_FILE (map) : NULL;
> Dodji> +	}
> 
> I don't understand this.  I thought that UNKNOWN_LOCATION should never
> have an associated file.  This is handled in a funny way: each front end
> is responsible for ensuring that this holds true (this could be cleaned
> up...).
> 
> Am I mistaken about this?  Or is something weird happening after this
> patch?

I was confused.  I realize now this was some papering over what I later
realized was PR preprocessor/48532, that I filed separately along with a
patch.  I have thus removed this hunk from my local copy of the patch.

> 
> Dodji> @@ -883,14 +884,14 @@ static void
> Dodji>  do_line (cpp_reader *pfile)
> Dodji>  {
> Dodji>    const struct line_maps *line_table = pfile->line_table;
> Dodji> -  const struct line_map *map = &line_table->maps[line_table->used - 1];
> Dodji> +  const struct line_map *map = LINEMAPS_LAST_ORDINARY_MAP (line_table);
>  
> Dodji>    /* skip_rest_of_line() may cause line table to be realloc()ed so note down
> Dodji>       sysp right now.  */
>  
> Dodji> -  unsigned char map_sysp = map->sysp;
> Dodji> +  unsigned char map_sysp = map->d.ordinary.sysp;
> Dodji>    const cpp_token *token;
> Dodji> -  const char *new_file = map->to_file;
> Dodji> +  const char *new_file = map->d.ordinary.to_file;
> 
> Did you want to use the accessor macros here?
> If not, why not?

Oops.  Yes of course I wanted to use the accessor macros there.  I have
just done it on my local copy of the patch now.  Thanks.

> 
> Dodji>    /* C99 raised the minimum limit on #line numbers.  */
> Dodji> @@ -945,11 +946,11 @@ static void
> Dodji>  do_linemarker (cpp_reader *pfile)
> Dodji>  {
> Dodji>    const struct line_maps *line_table = pfile->line_table;
> Dodji> -  const struct line_map *map = &line_table->maps[line_table->used - 1];
> Dodji> +  const struct line_map *map = LINEMAPS_LAST_ORDINARY_MAP (line_table);
> Dodji>    const cpp_token *token;
> Dodji> -  const char *new_file = map->to_file;
> Dodji> +  const char *new_file = map->d.ordinary.to_file;
> Dodji>    linenum_type new_lineno;
> Dodji> -  unsigned int new_sysp = map->sysp;
> Dodji> +  unsigned int new_sysp = map->d.ordinary.sysp;
> 
> Likewise.
> 
> Dodji>    if (map != NULL)
> Dodji> -    linemap_line_start (pfile->line_table, map->to_line, 127);
> Dodji> +    linemap_line_start (pfile->line_table, map->d.ordinary.to_line, 127);
> 
> Likewise.
> 
> Dodji> +  int sysp = (LINEMAPS_ORDINARY_HIGHEST_LINE (pfile->line_table) > 1)
> Dodji> +    && (pfile->buffer ? pfile->buffer->sysp : 0);
> 
> The indentation is off here.
> Put another paren after the '=' and emacs will DTRT.

Done, thanks.

> 
> Dodji> +   physicall source locations are called "spelling locations".
> 
> Typo, should be "physical".
> 
> Dodji> +/* A set of chronological line_map structures.  */
> Dodji> +struct GTY(()) line_maps {
> Dodji> +  
> Dodji> +  struct maps_info info_ordinary;
> Dodji> +
> Dodji> +  struct maps_info info_macro;
> 
> One thing to note is that there may be some code that assumes an
> ordering of location values.  If you hand out ordinary and macro
> locations separately (which I think is what is going on), then code
> doing this may break.

Argh.  The reason why I am doing this is to avoid the (huge) increase of
memory usage due to the fact that the presence of a macro map was
triggering the creation of one more standard map.  Given the number of
macros in some TU, that was starting to add up.  We have discussed this
off-line and a possible way to avoid that was to separate the "integer
space" of locations mapped into macro maps from the integer space of
locations mapped into ordinary maps.  Of course, locations from a given
integer space (be it the space of macro tokens locations or ordinary
token location) is ordered.  This also eases the map lookup as a simple
comparisons is enough to know if a given location is of a token coming
from a macro expansion or not.

Now I think I am left with two options, I guess.  Either I find a way to
make the two kind of locations (those mapped into macro maps and those
mapped into ordinary maps) share the same integer space and be ordered,
or, I spot the places in the code that assume an ordering of all
location values and I change that.

Is the location ordering a strong property we want to keep?

> 
> My recollection is that maybe the diagnostic code did this?
> Or maybe it was someplace more obscure :-(
> Did you happen to run across this while working on this patch?

I haven't noticed that with my tests.  But after reading this, I have
found one spot of the in diagnostic_report_diagnostic that does this.  I
don't know yet if there are other places.

> 
> Dodji> -      fname = pfile->line_table->maps[pfile->line_table->used-1].to_file;
> Dodji> +      fname = (LINEMAPS_LAST_ORDINARY_MAP (pfile->line_table))->d.ordinary.to_file;
> 
> Accessor?

Oops, fixed in my local copy of the patch.

> 
> Dodji> +/* Add a mapping of logical source line to physical source file and
> Dodji> +   line number. This function creates an "ordinary map", which is a
> Dodji> +   map that records locations of tokens that are not part of macro
> Dodji> +   replacement-lists present at a macro expansion point.
> Dodji> +
> Dodji> +   The text pointed to by TO_FILE must have a lifetime
> Dodji> +   at least as long as the lifetime of SET.  An empty
> Dodji> +   TO_FILE means standard input.  If reason is LC_LEAVE, and
> Dodji> +   TO_FILE is NULL, then TO_FILE, TO_LINE and SYSP are given their
> Dodji> +   natural values considering the file we are returning to.
> Dodji> +
> Dodji> +   A call to this function can relocate the previous set of
> Dodji> +   maps, so any stored line_map pointers should not be used.  */
> Dodji> +
> Dodji> +const struct line_map *
> Dodji> +linemap_add (struct line_maps *set, enum lc_reason reason,
> Dodji> +	     unsigned int sysp, const char *to_file, linenum_type to_line)
> 
> I would generally prefer for explanatory comments for public functions
> to be in the header, not by the implementation.  This is more in keeping
> with the current style of the line map code, but I also like it because
> it provides a single text for users of the library to read.

Ah.  I did this because:

     - I thought it was in line of the rest of the code of libcpp (and
       GCC).  I didn't realize it was done on purpose in line-map.h
       only.
     - Having the documentation be at the place where etags points you
       to when you look for a function seemed very convenient to me.  -
       It seems to me that having the documentation near the definition
       helps in keeping the document and the definition in sync.

But if you want, I can just move the the comments to the header file,
no problem at all.

-- 
		Dodji