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/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?
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]