* [PATCH 0/2] Condition coverage fixes @ 2024-04-05 19:58 Jørgen Kvalsvik 2024-04-05 19:58 ` [PATCH 1/2] Remove unecessary and broken MC/DC compile test Jørgen Kvalsvik ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Jørgen Kvalsvik @ 2024-04-05 19:58 UTC (permalink / raw) To: gcc-patches; +Cc: hjl.tools, rguenther, hubicka, ro, Jørgen Kvalsvik Hi, I propose these fixes for the current issues with the condition coverage. Rainer, I propose to simply delete the test with __sigsetjmp. I don't think it actually detects anything reasonable any more, I kept it around to prevent a regression. Since then I have built a lot of programs (with optimization enabled) and not really seen this problem. H.J., the problem you found with -O2 was really a problem of tree-inlining, which was actually caught earlier by Jan [1]. It probably warrants some more testing, but I could reproduce by tuning your test case to use always_inline and not -O2 and trigger the error. [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html Thanks, Jørgen Jørgen Kvalsvik (2): Remove unecessary and broken MC/DC compile test Copy condition->expr map when inlining [PR114599] gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ gcc/tree-inline.cc | 20 +++++++++++++++- 3 files changed, 44 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] Remove unecessary and broken MC/DC compile test 2024-04-05 19:58 [PATCH 0/2] Condition coverage fixes Jørgen Kvalsvik @ 2024-04-05 19:58 ` Jørgen Kvalsvik 2024-04-15 8:56 ` Rainer Orth 2024-04-05 19:58 ` [PATCH 2/2] Copy condition->expr map when inlining [PR114599] Jørgen Kvalsvik ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Jørgen Kvalsvik @ 2024-04-05 19:58 UTC (permalink / raw) To: gcc-patches; +Cc: hjl.tools, rguenther, hubicka, ro, Jørgen Kvalsvik The __sigsetjmp test was added as a regression test, which an early iteration of the MC/DC support caused an internal compiler error, triggered by a code path which did not make it through to the final revision. Since this test really only worked on linux and does not serve a purpose any more it can be removed. gcc/testsuite/ChangeLog: * gcc.misc-tests/gcov-19.c: Remove test. --- gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/gcc/testsuite/gcc.misc-tests/gcov-19.c b/gcc/testsuite/gcc.misc-tests/gcov-19.c index 17f1fb4e923..b83a38531ba 100644 --- a/gcc/testsuite/gcc.misc-tests/gcov-19.c +++ b/gcc/testsuite/gcc.misc-tests/gcov-19.c @@ -869,17 +869,6 @@ dest: goto * 0; } -int __sigsetjmp (); - -/* This should compile, but not called. */ -void -mcdc021c () -{ - while (x) /* conditions(0/2) true(0) false(0)*/ - /* conditions(end) */ - __sigsetjmp (); -} - /* If edges are not properly contracted the a && id (b) will be interpreted as two independent expressions. */ void -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Remove unecessary and broken MC/DC compile test 2024-04-05 19:58 ` [PATCH 1/2] Remove unecessary and broken MC/DC compile test Jørgen Kvalsvik @ 2024-04-15 8:56 ` Rainer Orth 0 siblings, 0 replies; 10+ messages in thread From: Rainer Orth @ 2024-04-15 8:56 UTC (permalink / raw) To: Jørgen Kvalsvik; +Cc: gcc-patches, hjl.tools, rguenther, hubicka Hi Jørgen, > The __sigsetjmp test was added as a regression test, which an early > iteration of the MC/DC support caused an internal compiler error, > triggered by a code path which did not make it through to the final > revision. Since this test really only worked on linux and does not > serve a purpose any more it can be removed. > > gcc/testsuite/ChangeLog: > > * gcc.misc-tests/gcov-19.c: Remove test. just a nit (and too late since it's already checked in): the ChangeLog entry should specify *which test* was removed. As is, it reads like the whole file has been removed. Something like * gcc.misc-tests/gcov-19.c (__sigsetjmp, mcdc021c): Remove. Rainer -- ----------------------------------------------------------------------------- Rainer Orth, Center for Biotechnology, Bielefeld University ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] Copy condition->expr map when inlining [PR114599] 2024-04-05 19:58 [PATCH 0/2] Condition coverage fixes Jørgen Kvalsvik 2024-04-05 19:58 ` [PATCH 1/2] Remove unecessary and broken MC/DC compile test Jørgen Kvalsvik @ 2024-04-05 19:58 ` Jørgen Kvalsvik 2024-04-06 5:50 ` [PATCH 0/2] Condition coverage fixes Richard Biener 2024-04-08 6:31 ` Sam James 3 siblings, 0 replies; 10+ messages in thread From: Jørgen Kvalsvik @ 2024-04-05 19:58 UTC (permalink / raw) To: gcc-patches; +Cc: hjl.tools, rguenther, hubicka, ro, Jørgen Kvalsvik When a function is tree-inlined, copy the condition -> expression mapping from the inlined function into the caller, shifted so uids are not mixed Tree inlining was always problematic under condition coverage - either through a nullptr dereference (like in the test case), or through quietly mixing conditions when the assigned IDs overlapped. PR middle-end/114599 gcc/ChangeLog: * tree-inline.cc (add_local_variables): Copy cond_uids mappings. gcc/testsuite/ChangeLog: * gcc.misc-tests/gcov-pr114599.c: New test. --- gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ gcc/tree-inline.cc | 20 +++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr114599.c b/gcc/testsuite/gcc.misc-tests/gcov-pr114599.c new file mode 100644 index 00000000000..e4c78c9c121 --- /dev/null +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr114599.c @@ -0,0 +1,25 @@ +/* PR middle-end/114599 */ +/* { dg-do compile } */ +/* { dg-options "-fcondition-coverage" } */ + +/* Check that a function with a condition inlined into a function without a + conditional works. When inlining happens the condition -> expression + mapping must be carried over. */ + +extern int type; + +void fn (void); + +__attribute__((always_inline)) +inline void +do_all_fn_doall_arg (void) +{ + if (type) + fn (); +} + +void +do_all_fn_LHASH_DOALL_ARG_arg2 (void) +{ + do_all_fn_doall_arg (); +} diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index eebcea8a029..b18917707cc 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -4659,7 +4659,8 @@ prepend_lexical_block (tree current_block, tree new_block) BLOCK_SUPERCONTEXT (new_block) = current_block; } -/* Add local variables from CALLEE to CALLER. */ +/* Add local variables from CALLEE to CALLER. If set for condition coverage, + copy basic condition -> expression mapping to CALLER. */ static inline void add_local_variables (struct function *callee, struct function *caller, @@ -4689,6 +4690,23 @@ add_local_variables (struct function *callee, struct function *caller, } add_local_decl (caller, new_var); } + + /* If -fcondition-coverage is used and the caller has conditions, copy the + mapping into the caller but and the end so the caller and callee + expressions aren't mixed. */ + if (callee->cond_uids) + { + if (!caller->cond_uids) + caller->cond_uids = new hash_map <gcond*, unsigned> (); + + unsigned dst_max_uid = 0; + for (auto itr : *callee->cond_uids) + if (itr.second >= dst_max_uid) + dst_max_uid = itr.second + 1; + + for (auto itr : *callee->cond_uids) + caller->cond_uids->put (itr.first, itr.second + dst_max_uid); + } } /* Add to BINDINGS a debug stmt resetting SRCVAR if inlining might -- 2.30.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Condition coverage fixes 2024-04-05 19:58 [PATCH 0/2] Condition coverage fixes Jørgen Kvalsvik 2024-04-05 19:58 ` [PATCH 1/2] Remove unecessary and broken MC/DC compile test Jørgen Kvalsvik 2024-04-05 19:58 ` [PATCH 2/2] Copy condition->expr map when inlining [PR114599] Jørgen Kvalsvik @ 2024-04-06 5:50 ` Richard Biener 2024-04-06 11:15 ` Jørgen Kvalsvik 2024-04-08 6:31 ` Sam James 3 siblings, 1 reply; 10+ messages in thread From: Richard Biener @ 2024-04-06 5:50 UTC (permalink / raw) To: Jørgen Kvalsvik; +Cc: gcc-patches, hjl.tools, hubicka, ro > Am 05.04.2024 um 21:59 schrieb Jørgen Kvalsvik <j@lambda.is>: > > Hi, > > I propose these fixes for the current issues with the condition > coverage. > > Rainer, I propose to simply delete the test with __sigsetjmp. I don't > think it actually detects anything reasonable any more, I kept it around > to prevent a regression. Since then I have built a lot of programs (with > optimization enabled) and not really seen this problem. > > H.J., the problem you found with -O2 was really a problem of > tree-inlining, which was actually caught earlier by Jan [1]. It probably > warrants some more testing, but I could reproduce by tuning your test > case to use always_inline and not -O2 and trigger the error. > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html Ok Thanks, Richard > Thanks, > Jørgen > > Jørgen Kvalsvik (2): > Remove unecessary and broken MC/DC compile test > Copy condition->expr map when inlining [PR114599] > > gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- > gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ > gcc/tree-inline.cc | 20 +++++++++++++++- > 3 files changed, 44 insertions(+), 12 deletions(-) > create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c > > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Condition coverage fixes 2024-04-06 5:50 ` [PATCH 0/2] Condition coverage fixes Richard Biener @ 2024-04-06 11:15 ` Jørgen Kvalsvik 2024-04-06 20:41 ` Jørgen Kvalsvik 0 siblings, 1 reply; 10+ messages in thread From: Jørgen Kvalsvik @ 2024-04-06 11:15 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, hjl.tools, hubicka, ro On 06/04/2024 07:50, Richard Biener wrote: > > >> Am 05.04.2024 um 21:59 schrieb Jørgen Kvalsvik <j@lambda.is>: >> >> Hi, >> >> I propose these fixes for the current issues with the condition >> coverage. >> >> Rainer, I propose to simply delete the test with __sigsetjmp. I don't >> think it actually detects anything reasonable any more, I kept it around >> to prevent a regression. Since then I have built a lot of programs (with >> optimization enabled) and not really seen this problem. >> >> H.J., the problem you found with -O2 was really a problem of >> tree-inlining, which was actually caught earlier by Jan [1]. It probably >> warrants some more testing, but I could reproduce by tuning your test >> case to use always_inline and not -O2 and trigger the error. >> >> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html > > Ok Thanks, committed. I am wondering if the fn->cond_uids access should always be guarded (in tree-profile.cc) should always be guarded. Right now there is the assumption that if condition coverage is requested the will exist and be populated, but as this shows there may be other circumstances where this is not true. Or perhaps there should be a gcc_assert to (reliably) detect cases where the map is not constructed properly? Thanks, Jørgen > > Thanks, > Richard > > >> Thanks, >> Jørgen >> >> Jørgen Kvalsvik (2): >> Remove unecessary and broken MC/DC compile test >> Copy condition->expr map when inlining [PR114599] >> >> gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- >> gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ >> gcc/tree-inline.cc | 20 +++++++++++++++- >> 3 files changed, 44 insertions(+), 12 deletions(-) >> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c >> >> -- >> 2.30.2 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Condition coverage fixes 2024-04-06 11:15 ` Jørgen Kvalsvik @ 2024-04-06 20:41 ` Jørgen Kvalsvik 2024-04-07 6:26 ` Richard Biener 0 siblings, 1 reply; 10+ messages in thread From: Jørgen Kvalsvik @ 2024-04-06 20:41 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, hjl.tools, hubicka, ro On 06/04/2024 13:15, Jørgen Kvalsvik wrote: > On 06/04/2024 07:50, Richard Biener wrote: >> >> >>> Am 05.04.2024 um 21:59 schrieb Jørgen Kvalsvik <j@lambda.is>: >>> >>> Hi, >>> >>> I propose these fixes for the current issues with the condition >>> coverage. >>> >>> Rainer, I propose to simply delete the test with __sigsetjmp. I don't >>> think it actually detects anything reasonable any more, I kept it around >>> to prevent a regression. Since then I have built a lot of programs (with >>> optimization enabled) and not really seen this problem. >>> >>> H.J., the problem you found with -O2 was really a problem of >>> tree-inlining, which was actually caught earlier by Jan [1]. It probably >>> warrants some more testing, but I could reproduce by tuning your test >>> case to use always_inline and not -O2 and trigger the error. >>> >>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html >> >> Ok > > Thanks, committed. > > I am wondering if the fn->cond_uids access should always be guarded (in > tree-profile.cc) should always be guarded. Right now there is the > assumption that if condition coverage is requested the will exist and be > populated, but as this shows there may be other circumstances where this > is not true. > > Or perhaps there should be a gcc_assert to (reliably) detect cases where > the map is not constructed properly? > > Thanks, > Jørgen I gave this some more thought, and realised I was too eager to fix the segfault. While trunk no longer crashes (at least on my x86_64 linux) the fix itself is bad. It copies the gcond -> uid mappings into the caller, but the stmts are deep copied into the caller, so no gcond will ever be a hit when we look up the condition_uids in tree-profile.cc. I did a very quick prototype to confirm. By applying this patch: @@ -2049,6 +2049,9 @@ copy_bb (copy_body_data *id, basic_block bb, copy_gsi = gsi_start_bb (copy_basic_block); + if (!cfun->cond_uids && id->src_cfun->cond_uids) + cfun->cond_uids = new hash_map <gcond*, unsigned> (); + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) { gimple_seq stmts; @@ -2076,6 +2079,12 @@ copy_bb (copy_body_data *id, basic_block bb, if (gimple_nop_p (stmt)) continue; + if (id->src_cfun->cond_uids && is_a <gcond*> (orig_stmt)) + { + unsigned *v = id->src_cfun->cond_uids->get (as_a<gcond*> (orig_stmt)); + if (v) cfun->cond_uids->put (as_a <gcond*> (stmt), *v); + } + and this test program: __attribute__((always_inline)) inline int inlinefn (int a) { if (a > 5) { printf ("a > 5\n"); return a; } else printf ("a < 5, was %d\n", a); return a * a - 2; } int mcdc027e (int a, int b) { int y = inlinefn (a); return y + b; } gcov reports: 2: 18:mcdc027e (int a, int b) condition outcomes covered 1/2 condition 0 not covered (true) -: 19:{ 2: 20: int y = inlinefn (a); 2: 21: return y + b; -: 22:} but without the patch, gcov prints nothing. I am not sure if this approach is even ideal. Probably the most problematic is the source line mapping which is all messed up. I checked with gcov --branch-probabilities and it too reports the callee at the top of the caller. If you think it is a good strategy I can clean up the prototype and submit a patch. I suppose the function _totals_ should be accurate, even if the source mapping is a bit surprising. What do you think? I am open to other strategies, too Thanks, Jørgen > >> >> Thanks, >> Richard >> >> >>> Thanks, >>> Jørgen >>> >>> Jørgen Kvalsvik (2): >>> Remove unecessary and broken MC/DC compile test >>> Copy condition->expr map when inlining [PR114599] >>> >>> gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- >>> gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ >>> gcc/tree-inline.cc | 20 +++++++++++++++- >>> 3 files changed, 44 insertions(+), 12 deletions(-) >>> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c >>> >>> -- >>> 2.30.2 >>> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Condition coverage fixes 2024-04-06 20:41 ` Jørgen Kvalsvik @ 2024-04-07 6:26 ` Richard Biener 2024-04-07 7:28 ` Jørgen Kvalsvik 0 siblings, 1 reply; 10+ messages in thread From: Richard Biener @ 2024-04-07 6:26 UTC (permalink / raw) To: Jørgen Kvalsvik; +Cc: gcc-patches, hjl.tools, hubicka, ro > Am 06.04.2024 um 22:41 schrieb Jørgen Kvalsvik <j@lambda.is>: > > On 06/04/2024 13:15, Jørgen Kvalsvik wrote: >>> On 06/04/2024 07:50, Richard Biener wrote: >>> >>> >>>> Am 05.04.2024 um 21:59 schrieb Jørgen Kvalsvik <j@lambda.is>: >>>> >>>> Hi, >>>> >>>> I propose these fixes for the current issues with the condition >>>> coverage. >>>> >>>> Rainer, I propose to simply delete the test with __sigsetjmp. I don't >>>> think it actually detects anything reasonable any more, I kept it around >>>> to prevent a regression. Since then I have built a lot of programs (with >>>> optimization enabled) and not really seen this problem. >>>> >>>> H.J., the problem you found with -O2 was really a problem of >>>> tree-inlining, which was actually caught earlier by Jan [1]. It probably >>>> warrants some more testing, but I could reproduce by tuning your test >>>> case to use always_inline and not -O2 and trigger the error. >>>> >>>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html >>> >>> Ok >> Thanks, committed. >> I am wondering if the fn->cond_uids access should always be guarded (in tree-profile.cc) should always be guarded. Right now there is the assumption that if condition coverage is requested the will exist and be populated, but as this shows there may be other circumstances where this is not true. >> Or perhaps there should be a gcc_assert to (reliably) detect cases where the map is not constructed properly? >> Thanks, >> Jørgen > > I gave this some more thought, and realised I was too eager to fix the segfault. While trunk no longer crashes (at least on my x86_64 linux) the fix itself is bad. It copies the gcond -> uid mappings into the caller, but the stmts are deep copied into the caller, so no gcond will ever be a hit when we look up the condition_uids in tree-profile.cc. > > I did a very quick prototype to confirm. By applying this patch: > > @@ -2049,6 +2049,9 @@ copy_bb (copy_body_data *id, basic_block bb, > > copy_gsi = gsi_start_bb (copy_basic_block); > > + if (!cfun->cond_uids && id->src_cfun->cond_uids) > + cfun->cond_uids = new hash_map <gcond*, unsigned> (); > + > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) > { > gimple_seq stmts; > @@ -2076,6 +2079,12 @@ copy_bb (copy_body_data *id, basic_block bb, > if (gimple_nop_p (stmt)) > continue; > > + if (id->src_cfun->cond_uids && is_a <gcond*> (orig_stmt)) > + { > + unsigned *v = id->src_cfun->cond_uids->get (as_a<gcond*> (orig_stmt)); > + if (v) cfun->cond_uids->put (as_a <gcond*> (stmt), *v); > + } > + > > > and this test program: > > __attribute__((always_inline)) > inline int > inlinefn (int a) > { > if (a > 5) > { > printf ("a > 5\n"); > return a; > } > else > printf ("a < 5, was %d\n", a); > return a * a - 2; > } > > int > mcdc027e (int a, int b) > { > int y = inlinefn (a); > return y + b; > } > > > gcov reports: > > 2: 18:mcdc027e (int a, int b) > condition outcomes covered 1/2 > condition 0 not covered (true) > -: 19:{ > 2: 20: int y = inlinefn (a); > 2: 21: return y + b; > -: 22:} > > but without the patch, gcov prints nothing. > > I am not sure if this approach is even ideal. Probably the most problematic is the source line mapping which is all messed up. I checked with gcov --branch-probabilities and it too reports the callee at the top of the caller. > > If you think it is a good strategy I can clean up the prototype and submit a patch. I suppose the function _totals_ should be accurate, even if the source mapping is a bit surprising. > > What do you think? I am open to other strategies, too I think the most important bit is that the segfault is gone. The interaction of coverage with inlining or even other optimization when applying optimization to coverage should be documented better. Does condition coverage apply ontop of regular coverage counting or is it an either/or? Thanks, Richard > Thanks, > Jørgen > >>> >>> Thanks, >>> Richard >>> >>> >>>> Thanks, >>>> Jørgen >>>> >>>> Jørgen Kvalsvik (2): >>>> Remove unecessary and broken MC/DC compile test >>>> Copy condition->expr map when inlining [PR114599] >>>> >>>> gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- >>>> gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ >>>> gcc/tree-inline.cc | 20 +++++++++++++++- >>>> 3 files changed, 44 insertions(+), 12 deletions(-) >>>> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c >>>> >>>> -- >>>> 2.30.2 >>>> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Condition coverage fixes 2024-04-07 6:26 ` Richard Biener @ 2024-04-07 7:28 ` Jørgen Kvalsvik 0 siblings, 0 replies; 10+ messages in thread From: Jørgen Kvalsvik @ 2024-04-07 7:28 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, hjl.tools, hubicka, ro On 07/04/2024 08:26, Richard Biener wrote: > > >> Am 06.04.2024 um 22:41 schrieb Jørgen Kvalsvik <j@lambda.is>: >> >> On 06/04/2024 13:15, Jørgen Kvalsvik wrote: >>>> On 06/04/2024 07:50, Richard Biener wrote: >>>> >>>> >>>>> Am 05.04.2024 um 21:59 schrieb Jørgen Kvalsvik <j@lambda.is>: >>>>> >>>>> Hi, >>>>> >>>>> I propose these fixes for the current issues with the condition >>>>> coverage. >>>>> >>>>> Rainer, I propose to simply delete the test with __sigsetjmp. I don't >>>>> think it actually detects anything reasonable any more, I kept it around >>>>> to prevent a regression. Since then I have built a lot of programs (with >>>>> optimization enabled) and not really seen this problem. >>>>> >>>>> H.J., the problem you found with -O2 was really a problem of >>>>> tree-inlining, which was actually caught earlier by Jan [1]. It probably >>>>> warrants some more testing, but I could reproduce by tuning your test >>>>> case to use always_inline and not -O2 and trigger the error. >>>>> >>>>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html >>>> >>>> Ok >>> Thanks, committed. >>> I am wondering if the fn->cond_uids access should always be guarded (in tree-profile.cc) should always be guarded. Right now there is the assumption that if condition coverage is requested the will exist and be populated, but as this shows there may be other circumstances where this is not true. >>> Or perhaps there should be a gcc_assert to (reliably) detect cases where the map is not constructed properly? >>> Thanks, >>> Jørgen >> >> I gave this some more thought, and realised I was too eager to fix the segfault. While trunk no longer crashes (at least on my x86_64 linux) the fix itself is bad. It copies the gcond -> uid mappings into the caller, but the stmts are deep copied into the caller, so no gcond will ever be a hit when we look up the condition_uids in tree-profile.cc. >> >> I did a very quick prototype to confirm. By applying this patch: >> >> @@ -2049,6 +2049,9 @@ copy_bb (copy_body_data *id, basic_block bb, >> >> copy_gsi = gsi_start_bb (copy_basic_block); >> >> + if (!cfun->cond_uids && id->src_cfun->cond_uids) >> + cfun->cond_uids = new hash_map <gcond*, unsigned> (); >> + >> for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) >> { >> gimple_seq stmts; >> @@ -2076,6 +2079,12 @@ copy_bb (copy_body_data *id, basic_block bb, >> if (gimple_nop_p (stmt)) >> continue; >> >> + if (id->src_cfun->cond_uids && is_a <gcond*> (orig_stmt)) >> + { >> + unsigned *v = id->src_cfun->cond_uids->get (as_a<gcond*> (orig_stmt)); >> + if (v) cfun->cond_uids->put (as_a <gcond*> (stmt), *v); >> + } >> + >> >> >> and this test program: >> >> __attribute__((always_inline)) >> inline int >> inlinefn (int a) >> { >> if (a > 5) >> { >> printf ("a > 5\n"); >> return a; >> } >> else >> printf ("a < 5, was %d\n", a); >> return a * a - 2; >> } >> >> int >> mcdc027e (int a, int b) >> { >> int y = inlinefn (a); >> return y + b; >> } >> >> >> gcov reports: >> >> 2: 18:mcdc027e (int a, int b) >> condition outcomes covered 1/2 >> condition 0 not covered (true) >> -: 19:{ >> 2: 20: int y = inlinefn (a); >> 2: 21: return y + b; >> -: 22:} >> >> but without the patch, gcov prints nothing. >> >> I am not sure if this approach is even ideal. Probably the most problematic is the source line mapping which is all messed up. I checked with gcov --branch-probabilities and it too reports the callee at the top of the caller. >> >> If you think it is a good strategy I can clean up the prototype and submit a patch. I suppose the function _totals_ should be accurate, even if the source mapping is a bit surprising. >> >> What do you think? I am open to other strategies, too > > I think the most important bit is that the segfault is gone. The interaction of coverage with inlining or even other optimization when applying optimization to coverage should be documented better. > > Does condition coverage apply ontop of regular coverage counting or is it an either/or? On top, it is perfectly reasonable (and desirable) to measure statement/line coverage in addition to condition coverage. That being said, if you achieve MC/DC you also achieve branch coverage, but gcc -fprofile-arcs + --branch-counts/--branch-probabilities measure more than just taken/not taken, so -fcondition-coverage does not completely replace it. You might also not care about MC/DC, only branch coverage. Personally, I have come around to this strategy being alright. It can, and even might be, documented that inlined functions will be anchored to the top of the calling function, and the summaries will be useful still. A future project could be to improve the source mapping also through inlining. In practice this is ok because code under test tends to not be inlined so much in practice. Thanks, Jørgen > > Thanks, > Richard > >> Thanks, >> Jørgen >> >>>> >>>> Thanks, >>>> Richard >>>> >>>> >>>>> Thanks, >>>>> Jørgen >>>>> >>>>> Jørgen Kvalsvik (2): >>>>> Remove unecessary and broken MC/DC compile test >>>>> Copy condition->expr map when inlining [PR114599] >>>>> >>>>> gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- >>>>> gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ >>>>> gcc/tree-inline.cc | 20 +++++++++++++++- >>>>> 3 files changed, 44 insertions(+), 12 deletions(-) >>>>> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c >>>>> >>>>> -- >>>>> 2.30.2 >>>>> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Condition coverage fixes 2024-04-05 19:58 [PATCH 0/2] Condition coverage fixes Jørgen Kvalsvik ` (2 preceding siblings ...) 2024-04-06 5:50 ` [PATCH 0/2] Condition coverage fixes Richard Biener @ 2024-04-08 6:31 ` Sam James 3 siblings, 0 replies; 10+ messages in thread From: Sam James @ 2024-04-08 6:31 UTC (permalink / raw) To: Jørgen Kvalsvik; +Cc: gcc-patches, hjl.tools, rguenther, hubicka, ro Jørgen Kvalsvik <j@lambda.is> writes: > Hi, > > I propose these fixes for the current issues with the condition > coverage. > > Rainer, I propose to simply delete the test with __sigsetjmp. I don't > think it actually detects anything reasonable any more, I kept it around > to prevent a regression. Since then I have built a lot of programs (with > optimization enabled) and not really seen this problem. > > H.J., the problem you found with -O2 was really a problem of > tree-inlining, which was actually caught earlier by Jan [1]. It probably > warrants some more testing, but I could reproduce by tuning your test > case to use always_inline and not -O2 and trigger the error. > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-April/648785.html I couldn't find your BZ account, but FWIW: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114627. Thanks. > > Thanks, > Jørgen > > Jørgen Kvalsvik (2): > Remove unecessary and broken MC/DC compile test > Copy condition->expr map when inlining [PR114599] > > gcc/testsuite/gcc.misc-tests/gcov-19.c | 11 --------- > gcc/testsuite/gcc.misc-tests/gcov-pr114599.c | 25 ++++++++++++++++++++ > gcc/tree-inline.cc | 20 +++++++++++++++- > 3 files changed, 44 insertions(+), 12 deletions(-) > create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr114599.c ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-15 8:56 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-05 19:58 [PATCH 0/2] Condition coverage fixes Jørgen Kvalsvik 2024-04-05 19:58 ` [PATCH 1/2] Remove unecessary and broken MC/DC compile test Jørgen Kvalsvik 2024-04-15 8:56 ` Rainer Orth 2024-04-05 19:58 ` [PATCH 2/2] Copy condition->expr map when inlining [PR114599] Jørgen Kvalsvik 2024-04-06 5:50 ` [PATCH 0/2] Condition coverage fixes Richard Biener 2024-04-06 11:15 ` Jørgen Kvalsvik 2024-04-06 20:41 ` Jørgen Kvalsvik 2024-04-07 6:26 ` Richard Biener 2024-04-07 7:28 ` Jørgen Kvalsvik 2024-04-08 6:31 ` Sam James
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).