Re: [PATCH 4/4] Replace line_map union with C++ class hierarchy

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2015-05-04 at 14:45 -0600, Jeff Law wrote:
> On 05/01/2015 06:56 PM, David Malcolm wrote:
> > This patch eliminates the union in struct line_map in favor of
> > a simple class hierarchy, making struct line_map a base class,
> > with line_map_ordinary and line_map_macro subclasses.
> >
> > The patch eliminates all usage of linemap_check_ordinary and
> > linemap_check_macro from line-map.h, updating return types and
> > signatures throughout libcpp and gcc's usage of it to use the
> > appropriate subclasses.
> >
> > This moves the checking of linemap kind from run-time to
> > compile-time, and also implicitly documents everywhere where
> > the code is expecting an ordinary map vs a macro map vs
> > either kind of map.  I believe it makes the code significantly
> > simpler: most of the accessor functions in line-map.h become
> > trivial field-lookups.
> >
> > I attemped to use templates for maps_info, but was stymied by
> > gengtype, so in the end I simply split it manually into
> > maps_info_ordinary and maps_info_macro.  In theory it's just
> > a vec<>, but vec.h is in gcc, and thus not available
> > for use from libcpp.
> >
> > In a similar vein, gcc/is-a.h is presumably not usable
> > from within libcpp.  If it were, there would be the following
> > rough equivalences:
> >
> > ---------------------------------  --------------------------------
> > line-map.h                         is-a.h
> > ---------------------------------  --------------------------------
> > linemap_check_ordinary (m)         as_a <line_map_ordinary *> (m)
> > linemap_check_macro (m)            as_a <line_map_macro *> (m)
> > linemap_macro_expansion_map_p (m)  (M ? is_a <line_map_macro *> (m)
> >                                        : false)
> > ---------------------------------  --------------------------------
> >
> > There are numerous places in libcpp that offset a
> > line_map * using array notation to get the next/prev line_map of the
> > same kind, e.g.:
> > MAP_START_LOCATION (&cached[1])
> > which breaks due to the different sizes of line_map vs its subclasses.
> >
> > On x86_64 host, before:
> > (gdb) p sizeof(line_map)
> > $1 = 40
> >
> > after:
> > (gdb) p sizeof(line_map)
> > $1 = 8
> > (gdb) p sizeof(line_map_ordinary)
> > $2 = 32
> > (gdb) p sizeof(line_map_macro)
> > $3 = 40
> >
> > Tracking down all of these array-based offsets to use a pointer to the
> > appropriate subclass (and thus use the correct offset) was rather
> > involved, but I believe the patch fixes them all now.
> >
> > (the patch thus also gives a very modest saving of 8 bytes per ordinary
> > line map).
> >
> > I've tried to use the naming convention "ord_map" and "macro_map"
> > whenever the typesystem ensures we're dealing with such a map,
> > wherever this is doable without needing to touch lines of code that
> > would otherwise not need touching by the patch.
> >
> > gcc/ChangeLog:
> > 	* diagnostic.c (diagnostic_report_current_module): Strengthen
> > 	local "new_map" from const line_map * to
> > 	const line_map_ordinary *.
> > 	* genmatch.c (error_cb): Likewise for local "map".
> > 	(output_line_directive): Likewise for local "map".
> > 	* input.c (expand_location_1): Likewise for local "map".
> > 	Pass NULL rather than &map to
> > 	linemap_unwind_to_first_non_reserved_loc, since the value is never
> > 	read from there, and the value written back not read from here.
> > 	(is_location_from_builtin_token): Strengthen local "map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	(dump_location_info): Strengthen locals "map" from
> > 	line_map *, one to const line_map_ordinary *, the other
> > 	to const line_map_macro *.
> > 	* tree-diagnostic.c (loc_map_pair): Strengthen field "map" from
> > 	const line_map * to const line_map_macro *.
> > 	(maybe_unwind_expanded_macro_loc): Add a call to
> > 	linemap_check_macro when writing to the "map" field of the
> > 	loc_map_pair.
> > 	Introduce local const line_map_ordinary * "ord_map", using it in
> > 	place of "map" in the part of the function where we know we have
> > 	an ordinary map.  Strengthen local "m" from const line_map * to
> > 	const line_map_ordinary *.
> >
> > gcc/ada/ChangeLog:
> > 	* gcc-interface/trans.c (Sloc_to_locus1): Strenghthen local "map"
> > 	from line_map * to line_map_ordinary *.
> e>
> > gcc/c-family/ChangeLog:
> > 	* c-common.h (fe_file_change): Strengthen param from
> > 	const line_map * to const line_map_ordinary *.
> > 	(pp_file_change): Likewise.
> > 	* c-lex.c (fe_file_change): Likewise.
> > 	(cb_define): Use linemap_check_ordinary when invoking
> > 	SOURCE_LINE.
> > 	(cb_undef): Likewise.
> > 	* c-opts.c (c_finish_options): Use linemap_check_ordinary when
> > 	invoking cb_file_change.
> > 	(c_finish_options): Likewise.
> > 	(push_command_line_include): Likewise.
> > 	(cb_file_change): Strengthen param "new_map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	* c-ppoutput.c (cb_define): Likewise for local "map".
> > 	(pp_file_change): Likewise for param "map" and local "from".
> >
> > gcc/fortran/ChangeLog:
> > 	* cpp.c (maybe_print_line): Strengthen local "map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	(cb_file_change): Likewise for param "map" and local "from".
> > 	(cb_line_change): Likewise for local "map".
> >
> > libcpp/ChangeLog:
> > 	* directives.c (do_line): Strengthen local "map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	(do_linemarker): Likewise.
> > 	(_cpp_do_file_change): Assert that we're not dealing with
> > 	a macro map.  Introduce local "ord_map" via a call to
> > 	linemap_check_ordinary, guarded within the check for
> > 	non-NULL.  Use it for typesafety.
> > 	* files.c (cpp_make_system_header): Strengthen local "map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	* include/cpplib.h (struct cpp_callbacks): Likewise for second
> > 	parameter of "file_change" callback.
> > 	* include/line-map.h (struct line_map): Convert from a struct
> > 	containing a union to a base class.
> > 	(struct line_map_ordinary): Convert to a subclass of line_map.
> > 	(struct line_map_macro): Likewise.
> > 	(linemap_check_ordinary): Strengthen return type from line_map *
> > 	to line_map_ordinary *, and add a const-variant.
> > 	(linemap_check_macro): New pair of functions.
> > 	(ORDINARY_MAP_STARTING_LINE_NUMBER): Strengthen param from
> > 	const line_map * to const line_map_ordinary *, eliminating call
> > 	to linemap_check_ordinary.  Likewise for the non-const variant.
> > 	(ORDINARY_MAP_INCLUDER_FILE_INDEX): Likewise.
> > 	(ORDINARY_MAP_IN_SYSTEM_HEADER_P): Likewise.
> > 	(ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): Likewise.
> > 	(ORDINARY_MAP_FILE_NAME): Likewise.
> > 	(MACRO_MAP_MACRO): Strengthen param from const line_map * to
> > 	const line_map_macro *.  Likewise for the non-const variant.
> > 	(MACRO_MAP_NUM_MACRO_TOKENS): Likewise.
> > 	(MACRO_MAP_LOCATIONS): Likewise.
> > 	(MACRO_MAP_EXPANSION_POINT_LOCATION): Likewise.
> > 	(struct maps_info): Replace with...
> > 	(struct maps_info_ordinary):...this and...
> > 	(struct maps_info_macro): ...this.
> > 	(struct line_maps): Convert fields "info_ordinary" and
> > 	"info_macro" to the above new structs.
> > 	(LINEMAPS_MAP_INFO): Delete both functions.
> > 	(LINEMAPS_MAPS): Likewise.
> > 	(LINEMAPS_ALLOCATED): Rewrite both variants to avoid using
> > 	LINEMAPS_MAP_INFO.
> > 	(LINEMAPS_USED): Likewise.
> > 	(LINEMAPS_CACHE): Likewise.
> > 	(LINEMAPS_MAP_AT): Likewise.
> > 	(LINEMAPS_ORDINARY_MAPS): Strengthen return type from line_map *
> > 	to line_map_ordinary *.
> > 	(LINEMAPS_ORDINARY_MAP_AT): Likewise.
> > 	(LINEMAPS_LAST_ORDINARY_MAP): Likewise.
> > 	(LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP): Likewise.
> > 	(LINEMAPS_MACRO_MAPS): Strengthen return type from line_map * to
> > 	line_map_macro *.
> > 	(LINEMAPS_MACRO_MAP_AT): Likewise.
> > 	(LINEMAPS_LAST_MACRO_MAP): Likewise.
> > 	(LINEMAPS_LAST_ALLOCATED_MACRO_MAP): Likewise.
> > 	(linemap_map_get_macro_name): Strengthen param from
> > 	const line_map * to const line_map_macro *.
> > 	(SOURCE_LINE): Strengthen first param from const line_map * to
> > 	const line_map_ordinary *, removing call to
> > 	linemap_check_ordinary.
> > 	(SOURCE_COLUMN): Likewise.
> > 	(LAST_SOURCE_LINE_LOCATION): Likewise.
> > 	(LAST_SOURCE_LINE): Strengthen first param from const line_map *
> > 	to const line_map_ordinary *.
> > 	(LAST_SOURCE_COLUMN): Likewise.
> > 	(INCLUDED_FROM): Strengthen return type from line_map * to
> > 	line_map_ordinary *., and second param from const line_map *
> > 	to const line_map_ordinary *, removing call to
> > 	linemap_check_ordinary.
> > 	(MAIN_FILE_P): Strengthen param from const line_map * to
> > 	const line_map_ordinary *, removing call to
> > 	linemap_check_ordinary.
> > 	(linemap_position_for_line_and_column): Strengthen param from
> > 	const line_map * to const line_map_ordinary *.
> > 	(LINEMAP_FILE): Strengthen param from const line_map * to
> > 	const line_map_ordinary *, removing call to
> > 	linemap_check_ordinary.
> > 	(LINEMAP_LINE): Likewise.
> > 	(LINEMAP_SYSP): Likewise.
> > 	(linemap_resolve_location): Strengthen final param from
> > 	const line_map ** to const line_map_ordinary **.
> > 	* internal.h (CPP_INCREMENT_LINE): Likewise for local "map".
> > 	(linemap_enter_macro): Strengthen return type from
> > 	const line_map * to const line_map_macro *.
> > 	(linemap_add_macro_token): Likewise for first param.
> > 	* line-map.c (linemap_check_files_exited): Strengthen local "map"
> > 	from const line_map * to const line_map_ordinary *.
> > 	(new_linemap): Introduce local "map_size" and use it when
> > 	calculating how large the buffer should be.  Rewrite based
> > 	on change of info_macro and info_ordinary into distinct types.
> > 	(linemap_add): Strengthen locals "map" and "from" from line_map *
> > 	to line_map_ordinary *.
> > 	(linemap_enter_macro): Strengthen return type from
> > 	const line_map * to const line_map_macro *, and local "map" from
> > 	line_map * to line_map_macro *.
> > 	(linemap_add_macro_token): Strengthen param "map" from
> > 	const line_map * to const line_map_macro *.
> > 	(linemap_line_start): Strengthen local "map" from line_map * to
> > 	line_map_ordinary *.
> > 	(linemap_position_for_column): Likewise.
> > 	(linemap_position_for_line_and_column): Strengthen first param
> > 	from const line_map * to const line_map_ordinary *.
> > 	(linemap_position_for_loc_and_offset): Strengthen local "map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	(linemap_ordinary_map_lookup): Likewise for return type and locals
> > 	"cached" and "result".
> > 	(linemap_macro_map_lookup): Strengthen return type and locals
> > 	"cached" and "result" from const line_map * to
> > 	const line_map_macro *.
> > 	(linemap_macro_map_loc_to_exp_point): Likewise for param "map".
> > 	(linemap_macro_map_loc_to_def_point): Likewise.
> > 	(linemap_macro_map_loc_unwind_toward_spelling): Likewise.
> > 	(linemap_get_expansion_line): Strengthen local "map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	(linemap_get_expansion_filename): Likewise.
> > 	(linemap_map_get_macro_name): Strengthen param from
> > 	const line_map * to const line_map_macro *.
> > 	(linemap_location_in_system_header_p): Add call to
> > 	linemap_check_ordinary in region guarded by
> > 	!linemap_macro_expansion_map_p.  Introduce local "macro_map" via
> > 	linemap_check_macro in other region, using it in place of "map"
> > 	for typesafety.
> > 	(first_map_in_common_1): Add calls to linemap_check_macro.
> > 	(trace_include): Strengthen param "map" from const line_map * to
> > 	const line_map_ordinary *.
> > 	(linemap_macro_loc_to_spelling_point): Strengthen final param from
> > 	const line_map ** to const line_map_ordinary **.  Replace a
> > 	C-style cast with a const_cast, and add calls to
> > 	linemap_check_macro and linemap_check_ordinary.
> > 	(linemap_macro_loc_to_def_point): Likewise.
> > 	(linemap_macro_loc_to_exp_point): Likewise.
> > 	(linemap_resolve_location): Strengthen final param from
> > 	const line_map ** to const line_map_ordinary **.
> > 	(linemap_unwind_toward_expansion): Introduce local "macro_map" via
> > 	a checked cast and use it in place of *map.
> > 	(linemap_unwind_to_first_non_reserved_loc): Strengthen local
> > 	"map1" from const line_map * to const line_map_ordinary *.
> > 	(linemap_expand_location): Introduce local "ord_map" via a checked
> > 	cast and use it in place of map.
> > 	(linemap_dump): Make local "map" const.  Strengthen local
> > 	"includer_map" from line_map * to const line_map_ordinary *.
> > 	Introduce locals "ord_map" and "macro_map" via checked casts and
> > 	use them in place of "map" for typesafety.
> > 	(linemap_dump_location): Strengthen local "map" from
> > 	const line_map * to const line_map_ordinary *.
> > 	(linemap_get_file_highest_location): Update for elimination of
> > 	union.
> > 	(linemap_get_statistics): Strengthen local "cur_map" from
> > 	line_map * to const line_map_macro *.  Update uses of sizeof to
> > 	use the appropriate line_map subclasses.
> > 	* macro.c (_cpp_warn_if_unused_macro): Add call to
> > 	linemap_check_ordinary.
> > 	(builtin_macro): Strengthen local "map" from const line_map * to
> > 	const line_map_macro *.
> > 	(enter_macro_context): Likewise.
> > 	(replace_args): Likewise.
> > 	(tokens_buff_put_token_to): Likewise for param "map".
> > 	(tokens_buff_add_token): Likewise.
> > ---
> 
> 
> > @@ -158,7 +158,7 @@ expand_location_1 (source_location loc,
> >   	     location (toward the expansion point) that is not reserved;
> >   	     that is, the first location that is in real source code.  */
> >   	  loc = linemap_unwind_to_first_non_reserved_loc (line_table,
> > -							  loc, &map);
> > +							  loc, NULL);
> 
> Is that really correct?


I believe so.  The old code had (in gcc/input.c's expand_location_1):

  const struct line_map *map;


  [...snip...]

  if (loc >= RESERVED_LOCATION_COUNT)
    {
      if (!expansion_point_p)
	{
	  [...snip...]
	  loc = linemap_unwind_to_first_non_reserved_loc (line_table,
A>>>							  loc, &map); 
	  lrk = LRK_SPELLING_LOCATION;
	}
      loc = linemap_resolve_location (line_table, loc,
B>>>				      lrk, &map);
C>>>  xloc = linemap_expand_location (line_table, map, loc);
    }

I've highlighted the three lines referring to the local "map".

Consider line A, the call to linemap_unwind_to_first_non_reserved_loc.
This function takes a ptr to a ptr to a map; in this case &map.  It does
not read from this ptr; it can merely write to it in the final line of
one of the exit paths:

  if (map != NULL)
    *map = map0;
  return loc;
}

and it appears that the ptr that's written back can point to  either
kind of map (both ordinary and macro).

So in the old code, line A's the passing in of "&map" lead to map
sometimes being written to.


Consider line B, the call to linemap_resolve_location.

linemap_resolve_location takes a ptr to a ptr to a map.  It can call:
    case LRK_MACRO_EXPANSION_POINT:
      loc = linemap_macro_loc_to_exp_point (set, loc, map);
         (doesn't read *map; for non-NULL map, *always* writes to it,
	   either NULL or an ordinary map)
      break;
    case LRK_SPELLING_LOCATION:
      loc = linemap_macro_loc_to_spelling_point (set, loc, map);
         (as above: doesn't read *map, ; for non-NULL map,
	   *always* writes to it, either NULL or an ordinary map)
      break;
    case LRK_MACRO_DEFINITION_LOCATION:
      loc = linemap_macro_loc_to_def_point (set, loc, map);
         (as above)

Hence linemap_resolve_location shares this behavior of never reading its
*map, it definitely will write back to *map (assuming the map is
non-NULL), and it writes back an ordinary map to *map.

Hence at line B, the old code was always overwriting whatever value (if
any) had been written to map by line A.

Hence I believe it's safe to change line A's &map to simply NULL, and
strengthen the local map from a
  const line_map *
to a
  const line_map_ordinary *.

Assuming I've read all of this code correctly...


>    /* Return the map at a given index.  */
> >   inline line_map *
> >   LINEMAPS_MAP_AT (const line_maps *set, bool map_kind, int index)
> >   {
> > -  return &(LINEMAPS_MAPS (set, map_kind)[index]);
> > +  if (map_kind)
> > +    return &set->info_macro.maps[index];
> > +  else
> > +    return &set->info_ordinary.maps[index];
> >   }
> I see this pattern repeated regularly.  Any thoughts on how painful 
> it'll be to drop the map_kind argument and instead not vary the kind?

I'm not sure I understand your question.

Essentially we have two vecs here, a vec<line_map_ordinary> and a
vec<line_map_macro>, without using vec.h, and the code already assumes
in various places that the maps of one kind are laid out next to each
other in memory, with the nit that the list members have different sizes
(as noted above).

I attempted both
  (a) to turn the maps_info into a template, but ran afoul of gengtype
and it got overcomplicated, and
  (b) to try to move vec.h to libiberty, but it was non-trivial, and I
got nervous about pushback about putting C++ code in libiberty, so it
seemed simpler to manually duplicate the type, the result was much
simpler than either (a) or (b).

Note that LINEMAPS_MAPS_AT already took a map_kind argument; before
these changes, it looked like this:
#define LINEMAPS_MAP_AT(SET, MAP_KIND, INDEX)	\
  (&((LINEMAPS_MAPS (SET, MAP_KIND))[(INDEX)]))

and if you manually expand all those macros, it's basically doing the
same thing, with that inline function replacing:

#define LINEMAPS_MAPS(SET, MAP_KIND) \
  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps

#define LINEMAPS_MAP_INFO(SET, MACRO_MAP_P)				\
  ((MACRO_MAP_P)							\
   ? &((SET)->info_macro)						\
   : &((SET)->info_ordinary))

etc (all of these nested macros were one area where I found the existing
code hard to follow).

> I just spot checked all the mechanical changes.  Looks reasonable me to me.
> 
> Just need to sort out if the hunk with the call to l_u_t_f_n_r_l is 
> correct and get the pre-reqs installed and I think this is good to go.
> 
> 
> Jeff