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

["Jørgen Kvalsvik" <j@lambda.is>]

On 15/04/2024 10:53, Rainer Orth wrote:
> 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.
> 
> the new gcc.misc-tests/gcov-22.c test loops on SPARC (both Solaris and
> Linux).  I've filed PR gcov-profile/114720 for this, but couldn't find
> any bugzilla account of yours to Cc:
> 
> 	Rainer
> 
> 

Rainer,

Could you please try this patch? I don't have a sparc nor non-glibc 
build (and getting a virtual one up will take a while). I suppose the 
problem is that after longjmp the return address is to the call to 
setdest(), not jump() (like is assumed), which creates the loop. If so, 
just guarding the longjmp should be fine, the point of the test is to 
make sure that both branches can be taken and recorded when the cond is 
a setjmp. If it works I will document it and post the patch.

Thanks,
Jørgen

diff --git a/gcc/testsuite/gcc.misc-tests/gcov-22.c 
b/gcc/testsuite/gcc.misc-tests/gcov-22.c
index 641791a7223..1e413f31850 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-22.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-22.c
@@ -87,7 +87,12 @@ setdest ()
  void
  jump ()
  {
-    longjmp (dest, 1);
+    static int called_once = 0;
+    if (!called_once) /* conditions(suppress) */
+    {
+       called_once = 1;
+       longjmp (dest, 1);
+    }
  }

  int