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 { ... } > 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)