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