On 04/04/2022 10:14, Sebastian Huber wrote:
> Hello Jørgen,
>
> having support for MC/DC coverage in GCC would be really nice. I tried out
> your
> latest patch on an arm cross-compiler with Newlib (inhibit_libc is defined).
> Could you please add the following fix to your patch:
>
> diff --git a/libgcc/libgcov-merge.c b/libgcc/libgcov-merge.c
> index 89741f637e1..9e3e8ee5657 100644
> --- a/libgcc/libgcov-merge.c
> +++ b/libgcc/libgcov-merge.c
> @@ -33,6 +33,11 @@ void __gcov_merge_add (gcov_type *counters __attribute__
> ((unused)),
> unsigned n_counters __attribute__ ((unused))) {}
> #endif
>
> +#ifdef L_gcov_merge_ior
> +void __gcov_merge_ior (gcov_type *counters __attribute__ ((unused)),
> + unsigned n_counters __attribute__ ((unused))) {}
> +#endif
> +
> #ifdef L_gcov_merge_topn
> void __gcov_merge_topn (gcov_type *counters __attribute__ ((unused)),
> unsigned n_counters __attribute__ ((unused))) {}
>
> It seems that support for the new GCOV_TAG_CONDS is missing in gcov-tool and
> gcov-dump, see "tag_table" in gcc/gcov-dump.c and libgcc/libgcov-util.c.
>
> On 21/03/2022 12:55, Jørgen Kvalsvik via Gcc-patches wrote:
> [...]
>> Like Wahlen et al this implementation uses bitsets to store conditions,
>> which gcov later interprets. This is very fast, but introduces an max
>> limit for the number of terms in a single boolean expression. This limit
>> is the number of bits in a gcov_unsigned_type (which is typedef'd to
>> uint64_t), so for most practical purposes this would be acceptable.
>> limitation can be relaxed with a more sophisticated way of storing and
>> updating bitsets (for example length-encoding).
>
> For multi-threaded applications using -fprofile-update=atomic is quite
> important. Unfortunately, not all 32-bit targets support 64-bit atomic
> operations in hardware. There is a target hook to select the size of
> gcov_type.
> Maybe a dedicated 64-bit type should be used for the bitfield using two 32-bit
> atomic OR if necessary.
I was not very clear here - I select operations (and limits) based on the width
of gcov_unsigned_type, which (judging from other snippets in tree-profile.cc)
can be 32 or 64-bit depending on platform. I assume gcov_type is 32 bit on the
platforms you allude to, in which case it should still work fine but fail on
expressions that would work on 64-bit systems.
If I am wrong here I suppose we should consider what you propose (concatenating
bitfields would also be a strategy for supporting >64 conditions), but in that
case I think the other counters are wrong.
I would appreciate it if someone would take a closer look at the code touching
the gcov type to see if I got it right, which also includes the atomic code.
Should something need fixing I will be happy to do it.
>
>>
>> In action it looks pretty similar to the branch coverage. The -g short
>> opt carries no significance, but was chosen because it was an available
>> option with the upper-case free too.
>>
>> gcov --conditions:
>>
>> 3: 17:void fn (int a, int b, int c, int d) {
>> 3: 18: if ((a && (b || c)) && d)
>> conditions covered 5/8
>> condition 1 not covered (false)
>> condition 2 not covered (true)
>> condition 2 not covered (false)
>> 1: 19: x = 1;
>> -: 20: else
>> 2: 21: x = 2;
>> 3: 22:}
>
> I have some trouble to understand the output. Would 8/8 mean that we have 100%
> MC/DC coverage? What does "not covered (false)" or "not covered (true)" mean?
>
Yes, 8/8 would mean that the expression is 100% covered (all conditions take on
both values and have independent effect on the outcome). "not covered" is a
report of missing coverage, that is "condition 1 not covered (false)" means
that bit N (N = 1, b in this case) has not taken on false yet, and to achieve
100% coverage you need a test case where b = false.
The wording is arbitrary, and I tried to strike a balance between density,
clarity, grepability and noise. I am open to other suggestions that would
improve this.
Unrelated to this, in typing up some notes on this I found a few minor and one
quite significant (really, the masking algorithm is broken) error in the
algorithm, which I am working on correcting. I will propose the new patch with
these fixes too once I have finished writing and testing it.
