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 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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)