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