| Message ID | 113a3fb2-817c-ff7c-e57a-d5ee1656d4b9@suse.cz |
|---|---|
| State | New |
| Headers | show |
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 > >
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.
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. >
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. > > > >
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. >>> >> >> >
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. > >>> > >> > >> > > > >
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. >>>>> >>>> >>>> >>> >> >> >
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