Re: [PATCH] Add condition coverage profiling

["Martin Liška" <mliska@suse.cz>]

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:

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

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

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

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

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

Thanks.

Cheers,
Martin