This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).
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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)