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

[H.J. Lu]

On Fri, Aug 24, 2018 at 4:05 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> 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.

and

 /* Any ordinary locations ought to be "pure" at this point: no
     compressed ranges.  */
  linemap_assert (locus < RESERVED_LOCATION_COUNT
                  || locus >= LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES
                  || locus >= LINEMAPS_MACRO_LOWEST_LOCATION (set)
                  || pure_location_p (set, locus));

  /* Consider short-range optimization.  */
  if (can_be_stored_compactly_p (set, locus, src_range, data))
    {
      /* The low bits ought to be clear.  */
      linemap_assert (pure_location_p (set, locus));


> 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?

Yes, I did.  But it didn't work since callers of inemap_add expect
non-NULL return value for this case.

> 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
>

I will check it in.

Thanks.


-- 
H.J.