diff mbox

[GOOGLE] Updates highest_location when updating next_discriminator_location

Message ID CAO2gOZUow-9Dgt6qDwZSeuJw1Z+5=2PjkmPsoQcrWWsAoFZrBg@mail.gmail.com
State New
Headers show

Commit Message

Dehao Chen May 13, 2014, 4:28 p.m. UTC
The previous checkin will break build for most application:

http://gcc.gnu.org/viewcvs/gcc/branches/google/gcc-4_9/gcc/?view=log

This patch fixes the regression by updating highest_location.

Testing on-going,

OK for google-4_9 branch?

Thanks,
Dehao

Comments

Cary Coutant May 13, 2014, 4:47 p.m. UTC | #1
> Index: gcc/input.c
> ===================================================================
> --- gcc/input.c (revision 210338)
> +++ gcc/input.c (working copy)
> @@ -910,6 +910,8 @@ location_with_discriminator (location_t locus, int
>        : next_discriminator_location);
>
>    next_discriminator_location++;
> +  if (next_discriminator_location > line_table->highest_location)
> +    line_table->highest_location = next_discriminator_location;
>    return ret;
>  }

It seems to me that if something is breaking because highest_location
isn't getting updated, there's a deeper problem. The discriminator
handling depends on the fact that the line table doesn't ever add any
new values after min_discriminator_location is set, and the routines
in libcpp shouldn't ever see a location_t value above
min_discriminator_location (it should be converted to a "real"
location_t via map_discriminator_location()). If libcpp now assigns
new location_t values after we start handing out discriminators, then
we'll probably need to have libcpp start handling discriminator values
(which is the solution for LTO as well). If you're just seeing leakage
of discriminator locations into libcpp, there's probably a spot where
we need to call map_discriminator_location().

-cary
Dehao Chen May 13, 2014, 4:54 p.m. UTC | #2
The problem is that linemap_location_from_macro_expansion_p will
always return true if locus has discriminator. And in linemap_lookup,
this will lead to call linemap_macro_map_lookup, in which there is an
assertion:

linemap_assert (line >= LINEMAPS_MACRO_LOWEST_LOCATION (set));

However, line is actually not a macro location.
Dehao

On Tue, May 13, 2014 at 9:47 AM, Cary Coutant <ccoutant@google.com> wrote:
>> Index: gcc/input.c
>> ===================================================================
>> --- gcc/input.c (revision 210338)
>> +++ gcc/input.c (working copy)
>> @@ -910,6 +910,8 @@ location_with_discriminator (location_t locus, int
>>        : next_discriminator_location);
>>
>>    next_discriminator_location++;
>> +  if (next_discriminator_location > line_table->highest_location)
>> +    line_table->highest_location = next_discriminator_location;
>>    return ret;
>>  }
>
> It seems to me that if something is breaking because highest_location
> isn't getting updated, there's a deeper problem. The discriminator
> handling depends on the fact that the line table doesn't ever add any
> new values after min_discriminator_location is set, and the routines
> in libcpp shouldn't ever see a location_t value above
> min_discriminator_location (it should be converted to a "real"
> location_t via map_discriminator_location()). If libcpp now assigns
> new location_t values after we start handing out discriminators, then
> we'll probably need to have libcpp start handling discriminator values
> (which is the solution for LTO as well). If you're just seeing leakage
> of discriminator locations into libcpp, there's probably a spot where
> we need to call map_discriminator_location().
>
> -cary
Cary Coutant May 13, 2014, 5:04 p.m. UTC | #3
> The problem is that linemap_location_from_macro_expansion_p will
> always return true if locus has discriminator. And in linemap_lookup,
> this will lead to call linemap_macro_map_lookup, in which there is an
> assertion:
>
> linemap_assert (line >= LINEMAPS_MACRO_LOWEST_LOCATION (set));
>
> However, line is actually not a macro location.

That sounds like we're leaking a discriminator location into the
linemap code. Before you can call
linemap_location_from_macro_expansion_p, you need to do this (as in
expand_location_1):

  /* If LOC describes a location with a discriminator, extract the
     discriminator and map it to the real location.  */
  if (min_discriminator_location != UNKNOWN_LOCATION
      && loc >= min_discriminator_location
      && loc < next_discriminator_location)
    loc = map_discriminator_location (loc);

-cary
diff mbox

Patch

Index: gcc/input.c
===================================================================
--- gcc/input.c (revision 210338)
+++ gcc/input.c (working copy)
@@ -910,6 +910,8 @@  location_with_discriminator (location_t locus, int
       : next_discriminator_location);

   next_discriminator_location++;
+  if (next_discriminator_location > line_table->highest_location)
+    line_table->highest_location = next_discriminator_location;
   return ret;
 }