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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

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.

Martin


[-- Attachment #2: 0001-gcov-Mark-BBs-that-do-not-correspond-to-a-line-in-so.patch --]
[-- Type: text/x-patch, Size: 3149 bytes --]

From cc8738a287d5b0b98d61013ee065c96ed3e3cefa Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 10 Mar 2017 20:01:06 +0100
Subject: [PATCH] gcov: Mark BBs that do not correspond to a line in source
 code (PR gcov-profile/79891).

gcc/ChangeLog:

2017-03-10  Martin Liska  <mliska@suse.cz>

	PR gcov-profile/79891
	* gcov.c (read_graph_file): Mark BB that correspond to a real
	line in source code.
	(accumulate_line_counts): Do not sum these BBs.

gcc/testsuite/ChangeLog:

2017-03-10  Martin Liska  <mliska@suse.cz>

	PR gcov-profile/79891
	* gcc.misc-tests/gcov-17.c: New test.
---
 gcc/gcov.c                             | 12 +++++++++---
 gcc/testsuite/gcc.misc-tests/gcov-17.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-17.c

diff --git a/gcc/gcov.c b/gcc/gcov.c
index 4b5043c2f9f..10209b4c560 100644
--- a/gcc/gcov.c
+++ b/gcc/gcov.c
@@ -141,6 +141,7 @@ typedef struct block_info
 
   /* Block is a landing pad for longjmp or throw.  */
   unsigned is_nonlocal_return : 1;
+  unsigned has_line_assigned: 1;
 
   union
   {
@@ -1448,6 +1449,8 @@ read_graph_file (void)
 	      fn->num_blocks = num_blocks;
 
 	      fn->blocks = XCNEWVEC (block_t, fn->num_blocks);
+	      fn->blocks[ENTRY_BLOCK].has_line_assigned = 1;
+	      fn->blocks[EXIT_BLOCK].has_line_assigned = 1;
 	      for (ix = 0; ix != num_blocks; ix++)
 		fn->blocks[ix].flags = gcov_read_unsigned ();
 	    }
@@ -1529,6 +1532,7 @@ read_graph_file (void)
       else if (fn && tag == GCOV_TAG_LINES)
 	{
 	  unsigned blockno = gcov_read_unsigned ();
+	  fn->blocks[blockno].has_line_assigned = 1;
 	  unsigned *line_nos = XCNEWVEC (unsigned, length - 1);
 
 	  if (blockno >= fn->num_blocks || fn->blocks[blockno].u.line.encoding)
@@ -2458,9 +2462,11 @@ accumulate_line_counts (source_t *src)
 	  /* Cycle detection.  */
 	  for (block = line->u.blocks; block; block = block->chain)
 	    {
-	      for (arc_t *arc = block->pred; arc; arc = arc->pred_next)
-		if (!line->has_block (arc->src))
-		  count += arc->count;
+	      if (block->has_line_assigned)
+		for (arc_t *arc = block->pred; arc; arc = arc->pred_next)
+		  if (!line->has_block (arc->src))
+		    count += arc->count;
+
 	      for (arc_t *arc = block->succ; arc; arc = arc->succ_next)
 		arc->cs_count = arc->count;
 	    }
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-17.c b/gcc/testsuite/gcc.misc-tests/gcov-17.c
new file mode 100644
index 00000000000..1cff708be9b
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-17.c
@@ -0,0 +1,30 @@
+/* Test gcov block mode.  */
+
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+unsigned int
+UuT (void)
+{
+  unsigned int true_var = 1;
+  unsigned int false_var = 0;
+  unsigned int ret = 0;
+
+  if (true_var) /* count(1) */
+    {
+      if (false_var) /* count(1) */
+	ret = 111; /* count(#####) */
+    }
+  else
+    ret = 999; /* count(#####) */
+  return ret;
+}
+
+int
+main (int argc, char **argv)
+{
+  UuT ();
+  return 0;
+}
+
+/* { dg-final { run-gcov { -a gcov-17.c } } } */
-- 
2.11.1

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

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

Richard.

> Martin
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

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

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?

Richard.

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

[Martin Liška]

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%]

Martin

> 
> Richard.
> 

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

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.

Richard.

> Martin
> 
> > 
> > Richard.
> > 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

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

[Martin Liška]

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.

Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> Richard.
>>>
>>
>>
> 

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

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?

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)

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

[Martin Liška]

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

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 5367 bytes --]

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

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

[Martin Liška]

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

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 6176 bytes --]

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


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

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

[Martin Liška]

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.

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

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 7142 bytes --]

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.

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

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

[Martin Liška]

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.

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

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 7819 bytes --]

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.

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)

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

[Martin Liška]

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

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 9359 bytes --]

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?

            }

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)

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

[Martin Liška]

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

Huh, yeah. Currently line count is a sum of all basic blocks that are emitted
by profile.c with GCOV_TAG_LINES. That explains why considered loop has count == 11:

/tmp/gcov-1.gcno:	block 2:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':10, 14
/tmp/gcov-1.gcno:	block 4:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':14

where blocks 2 and 4 are:

  <bb 2> [0.00%]:
  i_3 = 0;
  goto <bb 4>; [0.00%]

...

  <bb 4> [0.00%]:
  # i_1 = PHI <i_3(2), i_7(3)>
  if (i_1 <= 9)
    goto <bb 3>; [0.00%]
  else
    goto <bb 5>; [0.00%]

The same happens to int a = b < 1 ? (c < 3 ? d : c) : a;

/tmp/gcov2.gcno:	block 2:`/tmp/gcov2.c':1, 3

  <bb 2> [0.00%]:
  if (b_3(D) <= 0)
    goto <bb 3>; [0.00%]
  else
    goto <bb 7>; [0.00%]

That showed a caching of locations actually magically handles loops and ternary operations.
I'm still wondering how should be defined line count for a multiple statements happening
on the line? Having that we can find a proper solution.

Martin

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

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

[Martin Liška]

On 03/14/2017 12:55 PM, 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
> 
> Huh, yeah. Currently line count is a sum of all basic blocks that are emitted
> by profile.c with GCOV_TAG_LINES. That explains why considered loop has count == 11:
> 
> /tmp/gcov-1.gcno:	block 2:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':10, 14
> /tmp/gcov-1.gcno:	block 4:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':14
> 
> where blocks 2 and 4 are:
> 
>   <bb 2> [0.00%]:
>   i_3 = 0;
>   goto <bb 4>; [0.00%]
> 
> ...
> 
>   <bb 4> [0.00%]:
>   # i_1 = PHI <i_3(2), i_7(3)>
>   if (i_1 <= 9)
>     goto <bb 3>; [0.00%]
>   else
>     goto <bb 5>; [0.00%]
> 
> The same happens to int a = b < 1 ? (c < 3 ? d : c) : a;
> 
> /tmp/gcov2.gcno:	block 2:`/tmp/gcov2.c':1, 3
> 
>   <bb 2> [0.00%]:
>   if (b_3(D) <= 0)
>     goto <bb 3>; [0.00%]
>   else
>     goto <bb 7>; [0.00%]
> 
> That showed a caching of locations actually magically handles loops and ternary operations.
> I'm still wondering how should be defined line count for a multiple statements happening
> on the line? Having that we can find a proper solution.
> 
> Martin

Out of curiosity, there's another example that's broken:

        1:   10:int main ()
        -:   11:{
        -:   12:  int i;
        -:   13:
       12:   14:  for (i = 0;
        -:   15:       i < 10;
       10:   16:       i++)	/* count(11) */
       10:   17:    noop ();			/* count(10) */
        -:   18:
        1:   19:  return 0;			/* count(1) */
        -:   20:}

Martin

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

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 11457 bytes --]

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
> 
> Huh, yeah. Currently line count is a sum of all basic blocks that are emitted
> by profile.c with GCOV_TAG_LINES. That explains why considered loop has count == 11:
> 
> /tmp/gcov-1.gcno:	block 2:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':10, 14
> /tmp/gcov-1.gcno:	block 4:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':14
> 
> where blocks 2 and 4 are:
> 
>   <bb 2> [0.00%]:
>   i_3 = 0;
>   goto <bb 4>; [0.00%]
> 
> ...
> 
>   <bb 4> [0.00%]:
>   # i_1 = PHI <i_3(2), i_7(3)>
>   if (i_1 <= 9)
>     goto <bb 3>; [0.00%]
>   else
>     goto <bb 5>; [0.00%]
> 
> The same happens to int a = b < 1 ? (c < 3 ? d : c) : a;
> 
> /tmp/gcov2.gcno:	block 2:`/tmp/gcov2.c':1, 3
> 
>   <bb 2> [0.00%]:
>   if (b_3(D) <= 0)
>     goto <bb 3>; [0.00%]
>   else
>     goto <bb 7>; [0.00%]
> 
> That showed a caching of locations actually magically handles loops and ternary operations.
> I'm still wondering how should be defined line count for a multiple statements happening
> on the line? Having that we can find a proper solution.

It should be number of times the line is _entered_, that is, lineno
changed from something != lineno to lineno.  Consider

foo (); goto baz; lab: bar ();   // line 1
baz:
goto lab;

should increment line 1 when entering to foo () as well as when
entering through goto lab.  but both times just once.

Richard.


> Martin
> 
> > 
> >           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?
> > 
> >             }
> > 
> > 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)

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]

On Tue, 14 Mar 2017, Richard Biener wrote:

> On Tue, 14 Mar 2017, Martin Liška wrote:
> > 
> > /tmp/gcov-1.gcno:	block 2:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':10, 14
> > /tmp/gcov-1.gcno:	block 4:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':14
> > 
> > where blocks 2 and 4 are:
> > 
> >   <bb 2> [0.00%]:
> >   i_3 = 0;
> >   goto <bb 4>; [0.00%]
> > 
> > ...
> > 
> >   <bb 4> [0.00%]:
> >   # i_1 = PHI <i_3(2), i_7(3)>
> >   if (i_1 <= 9)
> >     goto <bb 3>; [0.00%]
> >   else
> >     goto <bb 5>; [0.00%]
> > 
> > The same happens to int a = b < 1 ? (c < 3 ? d : c) : a;
> > 
> > /tmp/gcov2.gcno:	block 2:`/tmp/gcov2.c':1, 3
> > 
> >   <bb 2> [0.00%]:
> >   if (b_3(D) <= 0)
> >     goto <bb 3>; [0.00%]
> >   else
> >     goto <bb 7>; [0.00%]
> > 
> > That showed a caching of locations actually magically handles loops and ternary operations.
> > I'm still wondering how should be defined line count for a multiple statements happening
> > on the line? Having that we can find a proper solution.
> 
> It should be number of times the line is _entered_, that is, lineno
> changed from something != lineno to lineno.  Consider
> 
> foo (); goto baz; lab: bar ();   // line 1
> baz:
> goto lab;
> 
> should increment line 1 when entering to foo () as well as when
> entering through goto lab.  but both times just once.

Of course with -a you are supposed to get sub-line accuracy for these
cases, but then you'll get not a single number per line (and not sure
how you can reasonably interpret -a info without column info or so).

Richard.

> Richard.
> 
> 
> > Martin
> > 
> > > 
> > >           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?
> > > 
> > >             }
> > > 
> > > 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)

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

[Martin Liška]

On 03/14/2017 01:33 PM, Richard Biener wrote:
> 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
>>
>> Huh, yeah. Currently line count is a sum of all basic blocks that are emitted
>> by profile.c with GCOV_TAG_LINES. That explains why considered loop has count == 11:
>>
>> /tmp/gcov-1.gcno:	block 2:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':10, 14
>> /tmp/gcov-1.gcno:	block 4:`/home/marxin/Programming/gcc/gcc/testsuite/gcc.misc-tests/gcov-1.c':14
>>
>> where blocks 2 and 4 are:
>>
>>   <bb 2> [0.00%]:
>>   i_3 = 0;
>>   goto <bb 4>; [0.00%]
>>
>> ...
>>
>>   <bb 4> [0.00%]:
>>   # i_1 = PHI <i_3(2), i_7(3)>
>>   if (i_1 <= 9)
>>     goto <bb 3>; [0.00%]
>>   else
>>     goto <bb 5>; [0.00%]
>>
>> The same happens to int a = b < 1 ? (c < 3 ? d : c) : a;
>>
>> /tmp/gcov2.gcno:	block 2:`/tmp/gcov2.c':1, 3
>>
>>   <bb 2> [0.00%]:
>>   if (b_3(D) <= 0)
>>     goto <bb 3>; [0.00%]
>>   else
>>     goto <bb 7>; [0.00%]
>>
>> That showed a caching of locations actually magically handles loops and ternary operations.
>> I'm still wondering how should be defined line count for a multiple statements happening
>> on the line? Having that we can find a proper solution.
> 
> It should be number of times the line is _entered_, that is, lineno
> changed from something != lineno to lineno.  Consider
> 
> foo (); goto baz; lab: bar ();   // line 1
> baz:
> goto lab;
> 
> should increment line 1 when entering to foo () as well as when
> entering through goto lab.  but both times just once.

Ah, I see, such explanation works for me. However, the test-case you introduced is broken:

        -:    1:int a = 0;
        -:    2:
        1:    3:void foo()
        -:    4:{
        1:    5:  a = 1;
        1:    6:}
        -:    7:
        1:    8:void bar()
        -:    9:{
        1:   10:  a++;
        1:   11:}
        -:   12:
        1:   13:int main()
        -:   14:{
        1:   15:  foo (); goto baz; lab: bar ();
        -:   16:
        -:   17:  baz:
        2:   18:    if (a == 1)
        1:   19:      goto lab;
        -:   20:}

Line 15 should be executed twice.

Martin

> 
> Richard.
> 
> 
>> Martin
>>
>>>
>>>           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?
>>>
>>>             }
>>>
>>> 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.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
> 

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

[Martin Liška]

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?

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

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

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 11002 bytes --]

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)

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

[Nathan Sidwell]

Martin, Richard,
I've read up on the thread, but I'm not sure where you guys are with an 
actual patch.  From what I Richard nailed it in BZ with the comment that 
the BB should not be associated with any source line.  That's a new 
thing, so I think the gcov format needs extending (at least).

gcov must associate every BB with source line, so that it can present 
data to users.  That's a conflict with the above observation.

I don't understand why gcov does NOT think the line is executed when -a 
is not used:

         -:   29:					/* Following line falsely marked as covered when 
parameter "--rc lcov_branch_coverage=1" is set */
     #####:   30:					ReturnStatus_t = 0;

At least one block associated with line 30 is executed.  Why's it not 
being counted?

But that does seem to be the right output -- we shouldn't count this 
'artificial' block.  So then the question morphs into the -a case:

         1:   30:					ReturnStatus_t = 0;
     $$$$$:   30-block  0
         1:   30-block  1

We do count the artificial block.  Which is inconsistent.

Sorry, at this point I'm lost as to what is being suggested as a solution.

nathan

-- 
Nathan Sidwell

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

[Martin Liška]

On 03/21/2017 07:39 PM, Nathan Sidwell wrote:
> Martin, Richard,
> I've read up on the thread, but I'm not sure where you guys are with an actual patch.  From what I Richard nailed it in BZ with the comment that the BB should not be associated with any source line.  That's a new thing, so I think the gcov format needs extending (at least).
> 
> gcov must associate every BB with source line, so that it can present data to users.  That's a conflict with the above observation.

Hi.

Well, discussion shows that currently gcov is based on mapping between basic blocks and source lines. More precisely, when
a basic block is reached than we know that a bunch of source lines is executed. We also assume that all basic blocks must belong
to a line in a source code (counter example triggered this discussion). I believe the proper approach should be more focused on
edges to cover situations like:

int main()
{
  foo (); goto baz; lab: bar ();

  baz:
    if (a == 1)
      goto lab;
}

Moreover, there are 2 uncovered situations:
a) creation of a deleting constructor maps counters from 2 functions to a single line of code (PR 48463).
b) multi-versioning of a function can do very similar stuff; I know that it's rare that one has a valid profile
compound of 2 runs on a different machine

I'm planning to come back the these in next stage1. I would be happy for any feedback.

Martin

> 
> I don't understand why gcov does NOT think the line is executed when -a is not used:
> 
>         -:   29:                    /* Following line falsely marked as covered when parameter "--rc lcov_branch_coverage=1" is set */
>     #####:   30:                    ReturnStatus_t = 0;
> 
> At least one block associated with line 30 is executed.  Why's it not being counted?
> 
> But that does seem to be the right output -- we shouldn't count this 'artificial' block.  So then the question morphs into the -a case:
> 
>         1:   30:                    ReturnStatus_t = 0;
>     $$$$$:   30-block  0
>         1:   30-block  1
> 
> We do count the artificial block.  Which is inconsistent.
> 
> Sorry, at this point I'm lost as to what is being suggested as a solution.
> 
> nathan
>