This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] line-map include-from representation


On Wed, 2018-08-08 at 09:06 -0400, Nathan Sidwell wrote:
> This patch changes the way line-maps hold the included-from
> information. 
>   Currently this is an index to another (earlier) line-map, and
> relies 
> on the fact that #include cause the termination of the current map
> and 
> emission of a new map.  It's then possible to determine the location
> of 
> the include directive as the last line of the just terminated line
> map 
> (by examining the first location of the next map).
> 
> With modules, I'd like to show the import path in a similar
> way.  But 
> importing a module doesn't (necessarily) cause termination of a line
> map 
> -- the imported module's 'imported-from' location is in the middle
> of 
> some other map.  Here's what that looks like:
> In file of module bob,
>            imported at loc-2_b.C:5,
>          of module stuart,
>            imported at loc-2_d.C:2:
> loc-2_a.C:5:18: note:   initializing argument 1 of 'int frob(int*)'
> 
> [included-from lines may appear in the middle of that stack too]
> 
> Thus, this patch uses a source_location to represent the included
> from 
> location.  No data size changes, as old and new forms are really
> ints. 
> IMHO it makes the interface somewhat cleaner, as we stop exposing
> the 
> map indexes to the library users.
> 
> [I noticed that a piece of this patch already escaped, we were using 
> LAST_SOURCE_COLUMN in diagnostic_report_current_module, but I'd 
> inadvertently changed that in the prepatch that was intended to
> prepare 
> the ground for this one.  Sorry.]
> 
> booted & tested on x86_64-linux, ok?

> 2018-08-08  Nathan Sidwell  <nathan@acm.org>
> 
> 	Make linemap::included_at a location
> 	libcpp/
> 	* include/line-map.h (struct line_map_ordinary): Replace
> 	included_from map index with included_at source_location.
> 	(ORDINARY_MAP_INCLUDER_FILE_INDEX): Delete.
> 	(LAST_SOURCE_LINE_LOCATION): Move to line-map.c.

Looks like LAST_SOURCE_LINE was also deleted.

> 	(LAST_SOURCE_COLUMN): Delete.
> 	(INCLUDED_AT): New.
> 	(linemap_included_at): Declare.
> 	(MAIN_FILE_P): Adjust.
> 	* line-map.c (linemap_included_at): New.
> 	(lonemap_check_files_exited): Use linemap_included_at.
> 	(LAST_SOURCE_LINE_LOCATION): Made internal from header file.
> 	(linemap_add): Adjust inclusion setting.
> 	(linemap_dump, linemap_dump_location): Adjust.
> 	* directives.c (do_linemarker): Use linemap_included_at.
> 	gcc/
> 	* diagnostic.c (diagnostic_report_current_module): Use INCLUDED_AT
> 	& linemap_included_at.
> 	gcc/c-family/
> 	* c-common.c (try_to_locate_new_include_inertion_point): Use
> 	linemap_included_at.
> 	* c-lex.c (fe_file_change): Use INCLUDED_AT.
> 	* c-ppoutput.c (pp_file_change): Likewise.
> 	gcc/fortran/
> 	* cpp.c (cb_file_change): Use INCLUDED_AT.
> 
> Index: gcc/c-family/c-common.c
> ===================================================================
> --- gcc/c-family/c-common.c	(revision 263365)
> +++ gcc/c-family/c-common.c	(working copy)
> @@ -8413,8 +8413,8 @@ try_to_locate_new_include_insertion_poin
>        const line_map_ordinary *ord_map
>  	= LINEMAPS_ORDINARY_MAP_AT (line_table, i);
>  
> -      const line_map_ordinary *from = INCLUDED_FROM (line_table, ord_map);
> -      if (from)
> +      if (const line_map_ordinary *from
> +	  = linemap_included_at (line_table, ord_map))

I think "linemap_included_from" would be a better name for what this
patch calls "linemap_included_at".

Doing so would make a better distinction between the location at which
the include happened, vs the linemap in which it happened.

[...snip...]

> Index: libcpp/include/line-map.h
> ===================================================================
> --- libcpp/include/line-map.h	(revision 263365)
> +++ libcpp/include/line-map.h	(working copy)

[...snip...]

> -/* Return the last column number within an ordinary map.  */
> -
> -inline linenum_type
> -LAST_SOURCE_COLUMN (const line_map_ordinary *map)
> +inline source_location
> +INCLUDED_AT (const line_map_ordinary *ord_map)
>  {
> -  return SOURCE_COLUMN (map, LAST_SOURCE_LINE_LOCATION (map));
> +  return ord_map->included_at;
>  }

These used to be macros, hence the SCARY_UPPER_CASE; I converted them
to inline functions as a precursor to adding range support to
locations; I didn't rename them at the time, to minimize churn.

This is a new inline function, and so doesn't need to be in upper case.
How about "linemap_included_at" - which you're already using for
something else, but which I think should be renamed...

> -/* Returns the map a given map was included from, or NULL if the map
> -   belongs to the main file, i.e, a file that wasn't included by
> -   another one.  */
> -inline line_map_ordinary *
> -INCLUDED_FROM (struct line_maps *set, const line_map_ordinary *ord_map)
> -{
> -  return ((ord_map->included_from == -1)
> -	  ? NULL
> -	  : LINEMAPS_ORDINARY_MAP_AT (set, ord_map->included_from));
> -}
> +/* The linemap containing the included-from location.  */
> +const line_map_ordinary *linemap_included_at (line_maps *set,
> +					      const line_map_ordinary *);

...indeed, please rename this one to "linemap_included_from".

[...snip...]

>  /* Encode and return a source_location from a column number. The
> Index: libcpp/line-map.c
> ===================================================================
> --- libcpp/line-map.c	(revision 263366)
> +++ libcpp/line-map.c	(working copy)
> @@ -355,17 +355,25 @@ linemap_init (struct line_maps *set,
>    set->builtin_location = builtin_location;
>  }
>  
> +/* Return the ordinary line map from whence MAP was included.  Returns
> +   NULL if MAP was not an included.  */
> +
> +const line_map_ordinary *
> +linemap_included_at (line_maps *set, const line_map_ordinary *map)
> +{
> +  return linemap_ordinary_map_lookup (set, INCLUDED_AT (map));
> +}

So, with the renamings, this would look like:

const line_map_ordinary *
linemap_included_from (line_maps *set, const line_map_ordinary *map)
{
  return linemap_ordinary_map_lookup (set, linemap_included_at (map));
}

[...snip...]

> @@ -435,6 +443,17 @@ new_linemap (struct line_maps *set,  sou
>    return result;
>  }
>  
> +/* Return the location of the last source line within an ordinary
> +   map.  */
> +inline source_location
> +LAST_SOURCE_LINE_LOCATION (const line_map_ordinary *map)
> +{
> +  return (((map[1].start_location - 1
> +	    - map->start_location)
> +	   & ~((1 << map->m_column_and_range_bits) - 1))
> +	  + map->start_location);
> +}

Can this be made "static" now?  (and maybe lose the "inline" hint?)  I
think it's only used in one place after your patch.

[...snip...]

We don't seem to have much test coverage for this code.  Sorry to be a
pain, but could you please try adding a testcase of a diagnostic issued
from within a couple of levels of nested includes, perhaps using

/* { dg-options "-fdiagnostics-show-caret" } */

and

/* { dg-begin-multiline-output "" }
   { dg-end-multiline-output "" } */

(or dg-regexp if need be for dealing with arbitrary paths in filenames)

to give us some explicit test coverage of what the user sees? (and to
verify that this doesn't change the output).  None of our existing test
cases directly verify this, as far as I can tell. (I did some grepping,
but I only found a few dg-message directives referring to parts of the
output, and the stuff in prune.exp)

We'd presumably want something similar when you add modules support, to
verify that the "imported from" and "included from" stuff interact
appropriately and look sane to the end-user, so it'd be nice to test
the simple case first.

Thanks
Dave


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]