Re: [PATCH 0/2] Condition coverage fixes

["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

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