Re: [PATCH] Add condition coverage profiling

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

On 07/04/2022 14:04, Martin Liška wrote:
> On 3/28/22 16:40, Jørgen Kvalsvik via Gcc-patches wrote:
>> ... And with another tiny change that fixes Martin's while (1); case.
> 
> Hello.
> 
> Back to this ;) Thank you for the updated version of the patch. I have a couple
> of comments/requests:

Sorry for the late response, this email was eaten by spam filtering for some reason.

> 1) I noticed the following cannot be linked:
> 
> $ cat errors.C
> char trim_filename_name;
> int r;
> 
> void trim_filename() {
>   if (trim_filename_name)
>     r = 123;
>   while (trim_filename_name)
>     ;
> }
> 
> int
> main() {}
> 
> $ g++ errors.C -fprofile-conditions -O2
> mold: error: undefined symbol: /tmp/ccmZANB5.o: __gcov8._Z13trim_filenamev
> collect2: error: ld returned 1 exit status

I was able to reproduce this, and I think I have it fixed (that is, I cannot
reproduce it anymore). I looks like the counters were allocated, but never used
in the absence of an output file. I changed it so that the writes are guarded
behind an output check and the counter instrumentation is unconditional which
makes it go away.

The coverage data dependencies are more interleaved (at least currently) than
all the other counters, which makes the flow awkward. I am not convinced a
proper fix is actually worth it, or if anything it would be an separate next step.


> Btw. how much have you tested the patch set so far? Have you tried building
> something bigger
> with -fprofile-conditions enabled?
> 

My own test suite plus zlib, and it looks like linux is compiling fine. I
haven't rigorously studied the _output_ in these projects yet, and it is very
time consuming without a benchmark.

> 2) As noticed by Sebastian, please support the new tag in gcov-dump:
> 
> $ gcov-dump -l a.gcno
> ...
> a.gcno:    01450000:  28:LINES
> a.gcno:                  block 7:`a.c':11
> a.gcno:    01470000:   8:UNKNOWN
> 

Will do.

> 3) Then I have some top-level formatting comments:
> 
> a) please re-run check_GNU_style.py, I still see:
> === ERROR type #1: blocks of 8 spaces should be replaced with tabs (35 error(s))
> ===
> ...
> 
> b) write comments for each newly added function (and separate it by one empty
> line from
> the function definition)
> 
> c) do not create visual alignment, we don't use it:
> 
> +       cond->set ("count",   new json::integer_number (count));
> +       cond->set ("covered", new json::integer_number (covered));
> 
> and similar examples
> 
> d) finish multiple comments after a dot on the same line:
> 
> +    /* Number of expressions found - this value is the number of entries in the
> +       gcov output and the parameter to gcov_counter_alloc ().
> +       */
> 
> should be ... gcov_counter_alloc ().  */
> 
> e) for long lines with a function call like:
> 
> +           const int n = find_conditions_masked_by
> +               (block, expr, flags + k, path, CONDITIONS_MAX_TERMS);
> 
> do rather
> const int n
>   = find_conditions_masked_by (block, expr,
>                                next_arg, ...);
> 
> f) for function params:
> 
> +int
> +collect_reachable_conditionals (
> +    basic_block pre,
> +    basic_block post,
> +    basic_block *out,
> +    int maxsize,
> +    sbitmap expr)
> 
> use rather:
> 
> int
> collect_reachable_conditionals (basic_block pre,
>                                 second_arg,...
> 

Consider it done for the next revision.

> In the next round, I'm going to take a look at the CFG algorithm that identifies
> and instruments the sub-expressions.

Thank you.