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