On Fri, May 17, 2013 at 4:35 PM, Cary Coutant <ccout...@google.com> wrote:
>> The warning was attributed to the wrong lineno. Looks like
>> discriminator does not work well when it coexists with macros.
>
> 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?
>
> Other comments on the patch...
>
> In expand_location_1:
>
> + if (min_discriminator_location != UNKNOWN_LOCATION
> + && loc >= min_discriminator_location
> + && loc < min_discriminator_location + next_discriminator_location)
>
> The last comparison should just be "loc < next_discriminator_location".
>
> Likewise in has_discriminator.
Acked, will update the patch.
>
> 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?
>
> Also, you need to add vec.o to OBJS-libcommon in Makefile.in, since
> input.o now depends on it. Other binaries, like gcov, won't build
> otherwise.
Acked, will update 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?
Thanks,
Dehao
>
> -cary
