On Tue, 2018-06-26 at 14:28 -0400, Nathan Sidwell wrote:
> I've been wandering around the line-map machinery on the modules
> branch.
> One thing I noticed was the padding of the line_map type hierarchy
> was
> unfortunate. That made me sad. This patch fixes that.
>
> Rather than keep the the reason field in line_map, I move it to
> line_map_ordinary. That allows line_map to be a 4 byte struct,
> rather
> than 5 bytes data and 3 padding. We can use the source_location it
> contains to distinguish between ordinary and macro maps, by moving
> the
> LINE_MAP_MAX_LOCATION constant into the header file. While one
> interpretation of the comments is that the macro and ordinary line
> maps
> can grow arbitrarily, until they meet, the actual code presumes the
> boundary is fixed, so this is not a new restriction. (see the assert
> in
> linemap_enter_macro for instance)
Which assert are you referring to?
I'm not convinced that there already is a fixed boundary between the
ordinary maps and macro maps (though that's not necessarily an argument
against the patch); my reading of the existing linemap_enter_macro is
that it handles the two maps meeting, but doesn't set any restrictions
on where that meeting point is.
Previously there was an upper bound on the expansion of ordinary
locations:
const source_location LINE_MAP_MAX_SOURCE_LOCATION = 0x70000000;
but I believe that macro maps could in theory expand below this limit,
and this check in linemap_line_start is meant to prevent collisions
when this happens:
/* Locations of ordinary tokens are always lower than locations of
macro tokens. */
if (r >= LINEMAPS_MACRO_LOWEST_LOCATION (set))
return 0;
So if I'm reading the patch correctly, it introduces a new limit on how
big the macro maps can be: 0x10000000, hence about 268 million macro
argument expansions, I believe, before it will stop tracking macro
expansions.
Has this been smoketested on some very large C++ TUs?
> Reordering the fields in the ordinary and macro map objects
> continues
> the reduction in padding. On a 64-bit target the ordinary map goes
> from
> 32 to 24 bytes.
> booted & tested on x86_64-linux. I'll commit in a few days, unless
> there are objections (it is part of libcpp, but has an explicit
> maintainer listed).
>
> nathan
It would be nice to extend:
gcc/testsuite/gcc.dg/plugin/location_overflow_plugin.c
gcc/testsuite/gcc.dg/plugin/location-overflow-test-*.c
to get some test coverage of what happens when the two kinds of map
collide.
Hope this is constructive
Dave
