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