Re: [GOOGLE] Back port discriminator patches to gcc-4_8

[Cary Coutant <ccoutant at google dot com>]

>> I think the problem is with same_line_p. It's using expand_location to
>> test whether two locations refer to the same line, but expand_location
>> always unwinds the macro stack so that it's looking at the line number
>> of the macro expansion point. That means that every token in the macro
>> expansion will appear to be at the same line, so the loop over the
>> basic_block will replace all of the locations with the new_locus that
>> we just allocated for the new discriminator. Thus, it's clobbering the
>> locus for the instructions at line 27 in the macro definition with the
>> new locus we create to put a discriminator at line 26, and the warning
>> that should appear for line 27 now says line 26.
>
> Sounds like instead of checking the macro-expansion location,
> same_line_p should check the expanded macro location instead, so the
> problem will be resolved?

Yes, I changed same_line_p to call expand_location_to_spelling_point
instead, and the test runs as expected (one expected pass, one
expected failure).

>> In assign_discriminator:
>>
>> +      location_t new_locus = location_with_discriminator (locus,
>> discriminator);
>> +      gimple_stmt_iterator gsi;
>> +
>> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> +        {
>> +          gimple stmt = gsi_stmt (gsi);
>> +          if (same_line_p (locus, gimple_location (stmt)))
>> +            gimple_set_location (stmt, block ?
>> +                  COMBINE_LOCATION_DATA (line_table, new_locus, block) :
>> +                  LOCATION_LOCUS (new_locus));
>> +        }
>>
>> I'm not convinced the COMBINE_LOCATION_DATA is needed here, since
>> location_with_discriminator has already done that. And when block ==
>> NULL, calling LOCATION_LOCUS is effectively a no-op, isn't it? It
>> seems this could simply be:
>>
>>           gimple_set_location (stmt, new_locus);
>
> But what if block != NULL?

If block != NULL, new_locus will already be the ad hoc location
created for that block, because location_with_discriminator does the
COMBINE_LOCATION_DATA.

>> I'm still not happy with a straightforward port of the old patch,
>> though, since discriminator assignment might allocate locations that
>> overlap those used for macro definitions -- there's no checking to see
>> if we've run out of locations in that case. I guess this could be OK
>> for the google branch for now, but not for trunk. I'm looking at
>> alternatives using the adhoc mapping -- do you think it would work if
>> we extend the adhoc mapping to track both a "data" (i.e., the block
>> pointer) and a discriminator?
>
> Yeah, I think that works. The only concern is that it would increase
> the memory because each locus_adhoc map entry need to have another bit
> to store the extra info?

Yes, that's true. I'll give it a try and measure it. Do you have a
rough feel for how many ad hoc locations get created, relative to
spelling locations?

-cary