Re: [PATCH] Add condition coverage profiling

["Jørgen Kvalsvik" <jorgen.kvalsvik@woven-planet.global>]

On 18/10/2022 02:17, Hans-Peter Nilsson wrote:
> On Wed, 12 Oct 2022, Jørgen Kvalsvik via Gcc-patches wrote:
>> This patch adds support in gcc+gcov for modified condition/decision
>> coverage (MC/DC) with the -fprofile-conditions flag.
> 
> I'd love improvements in this area.
> 
> But this is a serious concern:
> 
>> gcov --conditions:
>>
>>         3:   17:void fn (int a, int b, int c, int d) {
>>         3:   18:    if ((a && (b || c)) && d)
>> condition outcomes covered 3/8
>> condition  0 not covered (true false)
>> condition  1 not covered (true)
>> condition  2 not covered (true)
>> condition  3 not covered (true)
>>         1:   19:        x = 1;
>>         -:   20:    else
>>         2:   21:        x = 2;
>>         3:   22:}
> 
> Is this the suggested output from gcov?
> 
> Sorry, but this is too hard to read; I can't read this.  What 
> does it mean?  What's 0 and what's 1 and which are the 8 
> conditions?  (Why not 16 or more; which are redundant?)  Or to 
> wit, a glance, which parts of (a && (b || c)) && d are actually 
> covered?

Hello,

Thanks for the feedback. I've modeled the output after the existing branch
coverage, but as you noticed conditions are slightly different because the
interesting output is what's _not_ taken. Like with branches, "conditions %d"
are the indices of the terms in the expression. The values in parenthesis are
the outcomes not found to be independent. Anything not listed is covered, guided
by the summary (conditions output covered n/m).

> 
> There has got to be a better *intuitively* understandable 
> presentation format than this. If you please forgive the errors 
> in not matching the partal expressions like in your proposal and 
> focus on the presentation format, I'd suggest something like, 
> for a one-time run with a=true, b=false, c=true, d=false:
> 
> "With:
>          3:   18:    if ((a && (b || c)) && d)
> 0:                       ^^^^^^^^^^^^^^^
> 1:                                          ^
> 2:                        ^
> 3:                             ^^^^^^^^
> 4:                              ^
> 5:                                   ^
> condition  0 not covered (false)
> condition  1 not covered (true)
> condition  2 not covered (false)
> condition  3 not covered (false)
> condition  4 not covered (true)
> condition  5 not covered (false)"
> (etc)
> 
> Possibly with each partial expression repeated above its 
> underscoring for readability, because of the increasing distance 
> between the underscoring and referred source.
> 
> Actually, a separate indexed table like that isn't the best 
> choice either.  Perhaps better quoting the source:
> 
> "condition (a && (b || c)) false not covered
> condition d false not covered
> condition (b || c) false not covered
> condition b true not covered
> condition c false not covered"
> 
> Or, just underscoring as instead of quoting the source:
> "        3:   18:    if ((a && (b || c)) && d)
> 
> In condition:            ^^^^^^^^^^^^^^^
> false not covered"
> (etc)
> 
> It is possible I completely misunderstand your proposal, but 
> there has to be something from the above to pick.  I'd hate to 
> see this go down because of usability problems.  Hope this was 
> constructive.
> 
> brgds, H-P

I agree, all of these are good suggestions to better outputs. The problem is
that information needed to create this output is, as far as I know, not
available when gcov runs as it is not recorded. When the instrumentation for the
condition coverage itself is determined this information *is* available (basic
blocks with locus) so it should be possible to improve both the output of
condition coverage and maybe even the branch coverage too.

Thanks,
Jørgen