On Mon, May 20, 2013 at 9:37 AM, Cary Coutant <ccout...@google.com> wrote:
>>> 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).
Cool. So shall we get this patch in gcc-4_8 first, and after you
change to encode discriminator in adhoc_locus map in trunk, we then
backport it to 4_8 again?
>
>>> 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.
Sounds good. I'll updated the patch.
>
>>> 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?
I don't remember exactly. It might be large for C++ apps in which
templates/abstraction are heavily used.
Dehao
>
> -cary
