This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] gcov: Mark BBs that do not correspond to a line in source code (PR gcov-profile/79891).


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

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]