Re: [PATCH v10 1/2] Add condition coverage (MC/DC)

[Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE>]

Hi Jørgen,

> On 04/04/2024 14:10, Jan Hubicka wrote:
>>> gcc/ChangeLog:
>>>
>>> 	* builtins.cc (expand_builtin_fork_or_exec): Check
>>> 	  condition_coverage_flag.
>>> 	* collect2.cc (main): Add -fno-condition-coverage to OBSTACK.
>>> 	* common.opt: Add new options -fcondition-coverage and
>>> 	  -Wcoverage-too-many-conditions.
>>> 	* doc/gcov.texi: Add --conditions documentation.
>>> 	* doc/invoke.texi: Add -fcondition-coverage documentation.
>>> 	* function.cc (free_after_compilation): Free cond_uids.
>>> 	* function.h (struct function): Add cond_uids.
>>> 	* gcc.cc: Link gcov on -fcondition-coverage.
>>> 	* gcov-counter.def (GCOV_COUNTER_CONDS): New.
>>> 	* gcov-dump.cc (tag_conditions): New.
>>> 	* gcov-io.h (GCOV_TAG_CONDS): New.
>>> 	(GCOV_TAG_CONDS_LENGTH): New.
>>> 	(GCOV_TAG_CONDS_NUM): New.
>>> 	* gcov.cc (class condition_info): New.
>>> 	(condition_info::condition_info): New.
>>> 	(condition_info::popcount): New.
>>> 	(struct coverage_info): New.
>>> 	(add_condition_counts): New.
>>> 	(output_conditions): New.
>>> 	(print_usage): Add -g, --conditions.
>>> 	(process_args): Likewise.
>>> 	(output_intermediate_json_line): Output conditions.
>>> 	(read_graph_file): Read condition counters.
>>> 	(read_count_file): Likewise.
>>> 	(file_summary): Print conditions.
>>> 	(accumulate_line_info): Accumulate conditions.
>>> 	(output_line_details): Print conditions.
>>> 	* gimplify.cc (next_cond_uid): New.
>>> 	(reset_cond_uid): New.
>>> 	(shortcut_cond_r): Set condition discriminator.
>>> 	(tag_shortcut_cond): New.
>>> 	(gimple_associate_condition_with_expr): New.
>>> 	(shortcut_cond_expr): Set condition discriminator.
>>> 	(gimplify_cond_expr): Likewise.
>>> 	(gimplify_function_tree): Call reset_cond_uid.
>>> 	* ipa-inline.cc (can_early_inline_edge_p): Check
>>> 	  condition_coverage_flag.
>>> 	* ipa-split.cc (pass_split_functions::gate): Likewise.
>>> 	* passes.cc (finish_optimization_passes): Likewise.
>>> 	* profile.cc (struct condcov): New declaration.
>>> 	(cov_length): Likewise.
>>> 	(cov_blocks): Likewise.
>>> 	(cov_masks): Likewise.
>>> 	(cov_maps): Likewise.
>>> 	(cov_free): Likewise.
>>> 	(instrument_decisions): New.
>>> 	(read_thunk_profile): Control output to file.
>>> 	(branch_prob): Call find_conditions, instrument_decisions.
>>> 	(init_branch_prob): Add total_num_conds.
>>> 	(end_branch_prob): Likewise.
>>> 	* tree-core.h (struct tree_exp): Add condition_uid.
>>> 	* tree-profile.cc (struct conds_ctx): New.
>>> 	(CONDITIONS_MAX_TERMS): New.
>>> 	(EDGE_CONDITION): New.
>>> 	(topological_cmp): New.
>>> 	(index_of): New.
>>> 	(single_p): New.
>>> 	(single_edge): New.
>>> 	(contract_edge_up): New.
>>> 	(struct outcomes): New.
>>> 	(conditional_succs): New.
>>> 	(condition_index): New.
>>> 	(condition_uid): New.
>>> 	(masking_vectors): New.
>>> 	(emit_assign): New.
>>> 	(emit_bitwise_op): New.
>>> 	(make_top_index_visit): New.
>>> 	(make_top_index): New.
>>> 	(paths_between): New.
>>> 	(struct condcov): New.
>>> 	(cov_length): New.
>>> 	(cov_blocks): New.
>>> 	(cov_masks): New.
>>> 	(cov_maps): New.
>>> 	(cov_free): New.
>>> 	(find_conditions): New.
>>> 	(struct counters): New.
>>> 	(find_counters): New.
>>> 	(resolve_counter): New.
>>> 	(resolve_counters): New.
>>> 	(instrument_decisions): New.
>>> 	(tree_profiling): Check condition_coverage_flag.
>>> 	(pass_ipa_tree_profile::gate): Likewise.
>>> 	* tree.h (SET_EXPR_UID): New.
>>> 	(EXPR_COND_UID): New.
>>>
>>> libgcc/ChangeLog:
>>>
>>> 	* libgcov-merge.c (__gcov_merge_ior): New.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* lib/gcov.exp: Add condition coverage test function.
>>> 	* g++.dg/gcov/gcov-18.C: New test.
>>> 	* gcc.misc-tests/gcov-19.c: New test.
>>> 	* gcc.misc-tests/gcov-20.c: New test.
>>> 	* gcc.misc-tests/gcov-21.c: New test.
>>> 	* gcc.misc-tests/gcov-22.c: New test.
>>> 	* gcc.misc-tests/gcov-23.c: New test.
>>> ---
>>>   gcc/builtins.cc                        |    2 +-
>>>   gcc/collect2.cc                        |    7 +-
>>>   gcc/common.opt                         |    9 +
>>>   gcc/doc/gcov.texi                      |   38 +
>>>   gcc/doc/invoke.texi                    |   21 +
>>>   gcc/function.cc                        |    1 +
>>>   gcc/function.h                         |    4 +
>>>   gcc/gcc.cc                             |    4 +-
>>>   gcc/gcov-counter.def                   |    3 +
>>>   gcc/gcov-dump.cc                       |   24 +
>>>   gcc/gcov-io.h                          |    3 +
>>>   gcc/gcov.cc                            |  209 ++-
>>>   gcc/gimplify.cc                        |  123 +-
>>>   gcc/ipa-inline.cc                      |    2 +-
>>>   gcc/ipa-split.cc                       |    2 +-
>>>   gcc/passes.cc                          |    3 +-
>>>   gcc/profile.cc                         |   76 +-
>>>   gcc/testsuite/g++.dg/gcov/gcov-18.C    |  282 ++++
>>>   gcc/testsuite/gcc.misc-tests/gcov-19.c | 1737 ++++++++++++++++++++++++
>>>   gcc/testsuite/gcc.misc-tests/gcov-20.c |   22 +
>>>   gcc/testsuite/gcc.misc-tests/gcov-21.c |   16 +
>>>   gcc/testsuite/gcc.misc-tests/gcov-22.c |  103 ++
>>>   gcc/testsuite/gcc.misc-tests/gcov-23.c |  361 +++++
>>>   gcc/testsuite/lib/gcov.exp             |  259 +++-
>>>   gcc/tree-core.h                        |    3 +
>>>   gcc/tree-profile.cc                    | 1058 ++++++++++++++-
>>>   gcc/tree.h                             |    4 +
>>>   libgcc/libgcov-merge.c                 |    5 +
>>>   28 files changed, 4339 insertions(+), 42 deletions(-)
>>>   create mode 100644 gcc/testsuite/g++.dg/gcov/gcov-18.C
>>>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-19.c
>>>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-20.c
>>>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-21.c
>>>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-22.c
>>>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-23.c
>>>
>>> ---
>>> Changes since v9:
>>>
>>> * function->cond_uid is no longer in ggc memory
>>> * function->cond_uid and condition annotation is only done when coverage
>>>    is requested
>>> * A few new test cases, notably interactions with C++ constexpr
>>> * Updated comments based on review feedback
>>> ---
>>>
>> 
>>> diff --git a/gcc/ipa-inline.cc b/gcc/ipa-inline.cc
>>> index dc120e6da5a..9502a21c741 100644
>>> --- a/gcc/ipa-inline.cc
>>> +++ b/gcc/ipa-inline.cc
>>> @@ -682,7 +682,7 @@ can_early_inline_edge_p (struct cgraph_edge *e)
>>>       }
>>>     gcc_assert (gimple_in_ssa_p (DECL_STRUCT_FUNCTION (e->caller->decl))
>>>   	      && gimple_in_ssa_p (DECL_STRUCT_FUNCTION (callee->decl)));
>>> -  if (profile_arc_flag
>>> +  if ((profile_arc_flag || condition_coverage_flag)
>>>         && ((lookup_attribute ("no_profile_instrument_function",
>>>   			    DECL_ATTRIBUTES (caller->decl)) == NULL_TREE)
>>>   	  != (lookup_attribute ("no_profile_instrument_function",
>> tree-inline should also copy the cond tags, since always_inline
>> functions are inlined even at -O0.  But I think this can be done
>> incrementally.
>> Patch is OK.
>> Thanks a lot and sorry for taking so long on this one.
>> Honza
>
> I guess you mean that since tree-inlining happen after gimplification (does
> it?) that the basic condition -> condition expression association should
> also fold in the now-inlined conditions?
>
> Thanks for the review - I'll do a final test run and install the patch.

this patch fails badly on Solaris (and probably all non-Linux targets):

+FAIL: gcc.misc-tests/gcov-19.c (test for excess errors)
+UNRESOLVED: gcc.misc-tests/gcov-19.c compilation failed to produce executable
+FAIL: gcc.misc-tests/gcov-19.c gcov: 0 failures in line counts, 0 in branch percentages, 796 in condition/decision, 0 in return percentages, 0 in intermediate format
+FAIL: gcc.misc-tests/gcov-19.c line 1006: unexpected summary - expected 1/24, got 0/24

and almost 1000 more lines.

gcov-19.exe doesn't link due to use of a Linux implementation detail:

Undefined                       first referenced
 symbol                             in file
__sigsetjmp                         /var/tmp//cc.8e7Qc.o
ld: fatal: symbol referencing errors
collect2: error: ld returned 1 exit status

Either switch the testcase to use sigsetjmp or restrict it to Linux.
Besides, emitting many hundreds of FAILs if the testcase doesn't compile
is very bad style.

Please fix.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University