Re: PING^1: [PATCH] Set start_location to 0 if we ran out of line map space

[David Malcolm <dmalcolm at redhat dot com>]

On Wed, 2018-08-22 at 12:48 -0700, H.J. Lu wrote:
> PING.

[stripping out-of-date address for Tom Tromey from recipients]

Sorry for the belated response.

> ---------- Forwarded message ----------
> From: H.J. Lu <hjl.tools@gmail.com>
> Date: Wed, Aug 15, 2018 at 4:33 AM
> Subject: [PATCH] Set start_location to 0 if we ran out of line map
> space
> To: gcc-patches@gcc.gnu.org
> 
> 
> With profiledbootstrap and --with-build-config=bootstrap-lto,
> linemap_add
> may create a macro map when we run out of line map space.  This patch
> changes start_location to UNKNOWN_LOCATION (0) in this case.
> 
> Tested with profiledbootstrap and --with-build-config=bootstrap-lto
> on
> Linux/x86-64.
> 
>         PR bootstrap/86872
>         * line-map.c (pure_location_p): Return true if linemap_lookup
>         returns NULL.
>         (linemap_add): Set start_location to 0 if we run out of line
> map
>         space.
> ---
>  libcpp/line-map.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libcpp/line-map.c b/libcpp/line-map.c
> index 555cd129a9c..cafe42273eb 100644
> --- a/libcpp/line-map.c
> +++ b/libcpp/line-map.c
> @@ -304,6 +304,8 @@ pure_location_p (line_maps *set, source_location
> loc)
>      return false;
> 
>    const line_map *map = linemap_lookup (set, loc);
> +  if (map == NULL)
> +    return true;
>    const line_map_ordinary *ordmap = linemap_check_ordinary (map);

I was wondering why this is needed, but it's presumably due to this
clause:

  if (set ==  NULL || line < RESERVED_LOCATION_COUNT)
    return NULL;

in linemap_ordinary_map_lookup.

Normally, the ordinary_maps are monotonically increasing by start
location, but with the next hunk to linemap_add there will be a map at
the end for location 0 which thus isn't monotonic.

>    if (loc & ((1U << ordmap->m_range_bits) - 1))
> @@ -492,6 +494,11 @@ linemap_add (struct line_maps *set, enum
> lc_reason reason,
>      }
> 
>    linemap_assert (reason != LC_ENTER_MACRO);
> +
> +  if (start_location >= LINE_MAP_MAX_LOCATION)
> +    /* We ran out of line map space.   */
> +    start_location = 0;
> +

If I'm reading this right, this means that we create an ordinary map
that starts at location 0.  The caller, linemap_line_start should then
bail out, returning location 0, and subsequent calls to
linemap_line_start should return 0 without attempting to create another
map.

My first thought was that instead linemap_add ought to return NULL for
this case, and linemap_line_start should check for NULL return and bail
out, but I believe this approach would mean doing slightly more work
per line.  Out of interest, did you try this other approach?

I wonder if there's a sane way to reproduce this failure using 
gcc.dg/plugin/location_overflow_plugin.c ?

>    line_map_ordinary *map
>      = linemap_check_ordinary (new_linemap (set, start_location));
>    map->reason = reason;
> --
> 2.17.1

Given the testing this has had, and the awkwardness of reproducing,
this is OK for trunk as-is.

Thanks
Dave