On Tue, 14 Mar 2017, Martin Liška wrote: > On 03/14/2017 11:48 AM, Richard Biener wrote: > > On Tue, 14 Mar 2017, Martin Liška wrote: > > > >> On 03/14/2017 11:30 AM, Richard Biener wrote: > >>> On Tue, 14 Mar 2017, Martin Liška wrote: > >>> > >>>> On 03/14/2017 11:13 AM, Richard Biener wrote: > >>>>> On Tue, 14 Mar 2017, Martin Liška wrote: > >>>>> > >>>>>> On 03/14/2017 10:12 AM, Richard Biener wrote: > >>>>>>> On Tue, 14 Mar 2017, Martin Liška wrote: > >>>>>>> > >>>>>>>> On 03/14/2017 09:13 AM, Richard Biener wrote: > >>>>>>>>> On Tue, 14 Mar 2017, Martin Liška wrote: > >>>>>>>>> > >>>>>>>>>> On 03/13/2017 04:16 PM, Richard Biener wrote: > >>>>>>>>>>> On Mon, 13 Mar 2017, Martin Liška wrote: > >>>>>>>>>>> > >>>>>>>>>>>> On 03/13/2017 02:53 PM, Richard Biener wrote: > >>>>>>>>>>>>> On Mon, 13 Mar 2017, Martin Liška wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>>> On 03/13/2017 02:01 PM, Richard Biener wrote: > >>>>>>>>>>>>>>> On Mon, 13 Mar 2017, Richard Biener wrote: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> On Fri, 10 Mar 2017, Martin Liška wrote: > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Hello. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> As briefly discussed in the PR, there are BB that do not > >>>>>>>>>>>>>>>>> correspond to a real > >>>>>>>>>>>>>>>>> line in source > >>>>>>>>>>>>>>>>> code. profile.c emits locations for all BBs that have a > >>>>>>>>>>>>>>>>> gimple statement > >>>>>>>>>>>>>>>>> belonging to a line. > >>>>>>>>>>>>>>>>> I hope these should be marked in gcov utility and not added > >>>>>>>>>>>>>>>>> in --all-block > >>>>>>>>>>>>>>>>> mode to counts of lines. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Patch survives make check RUNTESTFLAGS="gcov.exp". > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Thanks for review and feedback. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Humm, the patch doesn't seem to change the BBs associated > >>>>>>>>>>>>>>>> with a line > >>>>>>>>>>>>>>>> but rather somehow changes how we compute counts (the patch > >>>>>>>>>>>>>>>> lacks a > >>>>>>>>>>>>>>>> description of how you arrived at it). I expected the > >>>>>>>>>>>>>>>> line-to-BB > >>>>>>>>>>>>>>>> assignment process to be changed (whereever that is...). > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Currently, each basic block must belong to a source line. It's > >>>>>>>>>>>>>> how gcov > >>>>>>>>>>>>>> iterates all blocks (via lines). > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Ah, ok, looking at where output_location is called on I see > >>>>>>>>>>>>>>> we do not > >>>>>>>>>>>>>>> assign any line to that block. But still why does > >>>>>>>>>>>>>>> line->has_block (arc->src) return true? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Good objection! Problematic that 4->5 edge really comes from > >>>>>>>>>>>>>> the same line. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> <bb 4> [0.00%]: > >>>>>>>>>>>>>> ret_7 = 111; > >>>>>>>>>>>>>> PROF_edge_counter_10 = __gcov0.UuT[0]; > >>>>>>>>>>>>>> PROF_edge_counter_11 = PROF_edge_counter_10 + 1; > >>>>>>>>>>>>>> __gcov0.UuT[0] = PROF_edge_counter_11; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> <bb 5> [0.00%]: > >>>>>>>>>>>>>> # ret_1 = PHI <ret_5(3), ret_7(4)> > >>>>>>>>>>>>>> goto <bb 7>; [0.00%] > >>>>>>>>>>>>> > >>>>>>>>>>>>> Yes, but that's basically meaningless, unless not all edges > >>>>>>>>>>>>> come from the > >>>>>>>>>>>>> same line? I see nowhere where we'd explicitely assign lines to > >>>>>>>>>>>>> edges so it must be gcov "reconstructing" this somewhere. > >>>>>>>>>>>> > >>>>>>>>>>>> That's why I added the another flag. We stream locations for > >>>>>>>>>>>> basic blocks via > >>>>>>>>>>>> 'output_location' function. And assignment blocks to lines > >>>>>>>>>>>> happens here: > >>>>>>>>>>>> > >>>>>>>>>>>> static void > >>>>>>>>>>>> add_line_counts (coverage_t *coverage, function_t *fn) > >>>>>>>>>>>> { > >>>>>>>>>>>> ... > >>>>>>>>>>>> if (!ix || ix + 1 == fn->num_blocks) > >>>>>>>>>>>> /* Entry or exit block */; > >>>>>>>>>>>> else if (flag_all_blocks) > >>>>>>>>>>>> { > >>>>>>>>>>>> line_t *block_line = line; > >>>>>>>>>>>> > >>>>>>>>>>>> if (!block_line) > >>>>>>>>>>>> block_line = &sources[fn->src].lines[fn->line]; > >>>>>>>>>>>> > >>>>>>>>>>>> block->chain = block_line->u.blocks; > >>>>>>>>>>>> block_line->u.blocks = block; > >>>>>>>>>>>> } > >>>>>>>>>>>> > >>>>>>>>>>>> where line is always changes when we reach a BB that has a > >>>>>>>>>>>> source line assigned. Thus it's changed > >>>>>>>>>>>> for BB 4 and that's why BB 5 is added to same line. > >>>>>>>>>>> > >>>>>>>>>>> Ah, so this means we should "clear" the current line for BB 5 in > >>>>>>>>>>> output_location? At least I don't see how your patch may not > >>>>>>>>>>> regress > >>>>>>>>>>> some cases where the line wasn't output as an optimization? > >>>>>>>>>> > >>>>>>>>>> The new flag on block is kind of clearing that the BB is > >>>>>>>>>> artificial and in fact does not > >>>>>>>>>> belong to the line. I didn't want to do a bigger refactoring how > >>>>>>>>>> blocks are iterated via lines. > >>>>>>>>>> > >>>>>>>>>> Can you be please more specific about such a case? > >>>>>>>>> > >>>>>>>>> in profile.c I see > >>>>>>>>> > >>>>>>>>> if (name_differs || line_differs) > >>>>>>>>> { > >>>>>>>>> if (!*offset) > >>>>>>>>> { > >>>>>>>>> *offset = gcov_write_tag (GCOV_TAG_LINES); > >>>>>>>>> gcov_write_unsigned (bb->index); > >>>>>>>>> name_differs = line_differs=true; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> ... > >>>>>>>>> > >>>>>>>>> so if line_differs is false we might not output GCOV_TAG_LINES > >>>>>>>>> either > >>>>>>>>> because 1) optimization, less stuff output, 2) the block has no > >>>>>>>>> line. > >>>>>>>>> Looks like we can't really distinguish those. > >>>>>>>> > >>>>>>>> Agree with that. > >>>>>>>> > >>>>>>>>> > >>>>>>>>> Not sure how on the input side we end up associating a BB with > >>>>>>>>> a line if nothing was output for it though. > >>>>>>>>> > >>>>>>>>> That is, with your change don't we need > >>>>>>>>> > >>>>>>>>> Index: gcc/profile.c > >>>>>>>>> =================================================================== > >>>>>>>>> --- gcc/profile.c (revision 246082) > >>>>>>>>> +++ gcc/profile.c (working copy) > >>>>>>>>> @@ -941,8 +941,6 @@ output_location (char const *file_name, > >>>>>>>>> name_differs = !prev_file_name || filename_cmp (file_name, > >>>>>>>>> prev_file_name); > >>>>>>>>> line_differs = prev_line != line; > >>>>>>>>> > >>>>>>>>> - if (name_differs || line_differs) > >>>>>>>>> - { > >>>>>>>>> if (!*offset) > >>>>>>>>> { > >>>>>>>>> *offset = gcov_write_tag (GCOV_TAG_LINES); > >>>>>>>>> @@ -950,6 +948,9 @@ output_location (char const *file_name, > >>>>>>>>> name_differs = line_differs=true; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> + if (name_differs || line_differs) > >>>>>>>>> + { > >>>>>>>>> + > >>>>>>>>> /* If this is a new source file, then output the > >>>>>>>>> file's name to the .bb file. */ > >>>>>>>>> if (name_differs) > >>>>>>>>> > >>>>>>>>> to resolve this ambiguity? That is, _always_ emit GCOV_TAG_LINES > >>>>>>>>> for a BB? So then a BB w/o GCOV_TAG_LINES does _not_ have any > >>>>>>>>> lines associated. > >>>>>>>> > >>>>>>>> That should revolve it. Let me find and example where we do not emit > >>>>>>>> GCOV_TAG_LINES jsut because there's not difference in lines. > >>>>>>> > >>>>>>> sth like > >>>>>>> > >>>>>>> a = b < 1 ? (c < 3 ? d : c); > >>>>>>> > >>>>>>> or even > >>>>>>> > >>>>>>> if (..) { ... } else { ... } > >>>>>> > >>>>>> These samples work, however your patch would break situations like: > >>>>>> > >>>>>> 1: 10:int main () > >>>>>> -: 11:{ > >>>>>> -: 12: int i; > >>>>>> -: 13: > >>>>>> 22: 14: for (i = 0; i < 10; i++) /* count(11) */ > >>>>>> 10: 15: noop (); /* count(10) */ > >>>>>> > >>>>>> where 22 is summed as (1+10+11), which kind of makes sense as it > >>>>>> contains > >>>>>> of 3 statements. > >>>>> > >>>>> 22 is with my patch or without? I think 22 makes no sense. > >>>>> > >>>>> Richard. > >>>> > >>>> With your patch. > >>> > >>> I see. As said, I have zero (well, now some little ;)) knowledge > >>> about gcov. > >> > >> :) I'll continue twiddling with that because even loop-less construct > >> like: > >> > >> 1: 1:int foo(int b, int c, int d) > >> -: 2:{ > >> 5: 3: int a = b < 1 ? (c < 3 ? d : c) : a; > >> 2: 4: return a; > >> -: 5:} > >> > >> gives bogus output with your patch (which I believe does proper thing). > > > > Reading into the code (yes, it really seems it's for caching purposes > > given we walk BBs in "random" order) I also observe > > > > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) > > { > > gimple *stmt = gsi_stmt (gsi); > > if (!RESERVED_LOCATION_P (gimple_location (stmt))) > > output_location (gimple_filename (stmt), gimple_lineno > > (stmt), > > &offset, bb); > > > > should use expand_location and then look at the spelling location, > > otherwise we'll get interesting effects with macro expansion? > > Current code does: > > -: 1:#define foo(a) \ > -: 2:{ \ > -: 3: bar(a); \ > -: 4: bar(a); \ > -: 5:} > -: 6: > 2: 7:int bar(int a) > -: 8:{ > 2: 9: return a; > -: 10:} > -: 11: > 1: 12:int main() > -: 13:{ > 1: 14: foo(123); > -: 15:} > > while using expand_location_to_spelling_point will produce: > > -: 1:#define foo(a) \ > -: 2:{ \ > 1: 3: bar(a); \ > 1: 4: bar(a); \ > -: 5:} > -: 6: > 2: 7:int bar(int a) > -: 8:{ > 2: 9: return a; > -: 10:} > -: 11: > 1: 12:int main() > -: 13:{ > -: 14: foo(123); > -: 15:} > > I hope the current implementation looks fine. Or?
Yes, that looks fine. Richard. > Martin > > > > > } > > > > Richard. > > > >> Martin > >> > >> > >>> > >>> Richard. > >>> > >>>> Martin > >>>> > >>>>> > >>>>>> Martin > >>>>>> > >>>>>>> > >>>>>>> > >>>>>>>> Martin > >>>>>>>> > >>>>>>>>> > >>>>>>>>> Richard. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>> Hope Nathan will find time to provide review as he's familiar with > >>>>>>>>>> content of gcov.c. > >>>>>>>>>> > >>>>>>>>>> Martin > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> OTOH I don't know much about gcov format. > >>>>>>>>>>> > >>>>>>>>>>> Richard. > >>>>>>>>>>> > >>>>>>>>>>>> Martin > >>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> Richard. > >>>>>>>>>>>>> > >>>>>>>>>>>>>> Martin > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Richard. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>>> > >>> > >> > >> > > > > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)