diff mbox series

[v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]

Message ID 977b3846-f1fc-77a5-c1ee-367dd947ed44@gmail.com
State Accepted
Headers
Series [v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Xionghu Luo March 15, 2023, 10:07 a.m. UTC 
  On 2023/3/9 20:02, Richard Biener wrote:
> On Wed, 8 Mar 2023, Xionghu Luo wrote:
> 
>>
>>
>> On 2023/3/7 19:25, Richard Biener wrote:
>>>>> It would be nice to avoid creating blocks / preserving labels we'll
>>>>> immediately remove again.  For that we do need some analysis
>>>>> before creating basic-blocks that determines whether a label is
>>>>> possibly reached by a non-falltru edge.
>>>>>
>>>>
>>>> <bb 2> :
>>>> p = 0;
>>>> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>>>>
>>>> <bb 3> :
>>>> <L0>:           <= prev_stmt
>>>> <D.2748>:       <= stmt
>>>> p = p + 1;
>>>> n = n + -1;
>>>> if (n != 0) goto <D.2748>; else goto <D.2746>;
>>>>
>>>> Check if <L0> is a case label and <D.2748> is a goto target then return
>>>> true
>>>> in stmt_starts_bb_p to start a new basic block?  This would avoid creating
>>>> and
>>>> removing blocks, but cleanup_dead_labels has all bbs setup while
>>>> stmt_starts_bb_p
>>>> does't yet to iterate bbs/labels to establish label_for_bb[] map?
>>
>>> Yes.  I think we'd need something more pragmatic before make_blocks (),
>>> like re-computing TREE_USED of the label decls or computing a bitmap
>>> of targeted labels (targeted by goto, switch or any other means).
>>>
>>> I'll note that doing a cleanup_dead_labels () like optimization before
>>> we create blocks will help keeping LABEL_DECL_UID and thus
>>> label_to_block_map dense.  But it does look like a bit of
>>> an chicken-and-egg problem and the question is how effective the
>>> dead label removal is in practice.
>>
>> Tried to add function compute_target_labels(not sure whether the function
>> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
>> create/removing blocks now, but I still have several questions:
>>
>>   1. I used hash_set<tree> to save the target labels instead of bitmap, as
>> labels
>> are tree type value instead of block index so bitmap is not good for it since
>> we don't have LABEL_DECL_UID now?
> 
> We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> hash_set<tree> vs. bitmap is somewhat arbitrary here.  The real cost is
> the extra walk over all stmts.
> 
>>   2. Is the compute_target_labels still only for !optimize?  And if we compute
>> the target labels before create bbs, it is unnessary to guard the first
>> cleanup_dead_labels under !optimize now, because the switch-case-do-while
>> case already create new block for CASE_LABEL already.
> 
> OK.
> 
>>   3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
>> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
>> labels_eh?
> 
> I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> coverage already?
> 
>> PS1: The v3 patch will cause one test case fail:
>>
>> Number of regressions in total: 1
>>> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
>>> errors)
>>
>> due to this exausting case has labels from L0 to L100001, they won't be
>> optimized
>> to a simple if-else expression like before...
> 
> Hmm, that's somewhat unexpected.
> 
>>
>> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
>> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
>> so
>> just skip these label...
> 
> Please investigate, we might be missing a corner case here.
> 

I think the *previous fix* for labels “in the middle of block” is *incorrect*, it should
be handled in make_edges_bb when a basic block only has Label in it, just create a
fallthrough edge for it to avoid wrong cfg and unreachable trap generated?


@@ -853,6 +922,12 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index)
    bool fallthru = false;
    int ret = 0;

+  if (!optimize && !last)
+    {
+      make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
+      return 0;
+    }
+
    if (!last)
      return ret;


With the fix, the attached version could pass bootstrap and regression test on x86_64-linux-gnu.
From ec505cc7952707db805802af83dd82776a1d949f Mon Sep 17 00:00:00 2001
From: Xionghu Luo <xionghuluo@tencent.com>
Date: Tue, 28 Feb 2023 17:46:18 +0800
Subject: [PATCH v4]  gcov: Fix "do-while" structure in case statement leads to
 incorrect code coverage [PR93680]

v4: Address comments.
 4.1. Handle GIMPLE_GOTO and GIMPLE_ASM.
 4.2. Fix failure of limit-caselabels.c (labels on same line),
 pointer_array_1.f90 (unused labels) etc.

v3: Add compute_target_labels and call it in the front of make_blocks_1.
v2: Check whether two locus are on same line.

Start a new basic block if two labels have different location when
test-coverage.

Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
master?

gcc/ChangeLog:

	PR gcov/93680
	* tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
	target_labels.
	(compute_target_labels): New function.
	(make_blocks_1): Call compute_target_labels.
	(same_line_p): Return false if two locus are both
	UNKOWN_LOCATION.

gcc/testsuite/ChangeLog:

	PR gcov/93680
	* g++.dg/gcov/gcov-1.C: Correct counts.
	* gcc.misc-tests/gcov-4.c: Likewise.
	* gcc.misc-tests/gcov-pr85332.c: Likewise.
	* lib/gcov.exp: Also clean gcda if fail.
	* gcc.misc-tests/gcov-pr93680.c: New test.

Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
 gcc/tree-cfg.cc                             | 97 ++++++++++++++++++++-
 gcc/testsuite/g++.dg/gcov/gcov-1.C          |  2 +-
 gcc/testsuite/gcc.misc-tests/gcov-4.c       |  2 +-
 gcc/testsuite/gcc.misc-tests/gcov-pr85332.c |  2 +-
 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++
 gcc/testsuite/lib/gcov.exp                  |  4 +-
 6 files changed, 122 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
diff mbox series

Patch

diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..7b185200aaf 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -164,7 +164,7 @@  static edge gimple_redirect_edge_and_branch (edge, basic_block);
 static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
 
 /* Various helpers.  */
-static inline bool stmt_starts_bb_p (gimple *, gimple *);
+static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
 static int gimple_verify_flow_info (void);
 static void gimple_make_forwarder_block (edge);
 static gimple *first_non_label_stmt (basic_block);
@@ -521,6 +521,68 @@  gimple_call_initialize_ctrl_altering (gimple *stmt)
     gimple_call_set_ctrl_altering (stmt, false);
 }
 
+/* Compute target labels to save useful labels.  */
+static void
+compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
+{
+  gimple *stmt = NULL;
+  gimple_stmt_iterator j = gsi_start (seq);
+
+  while (!gsi_end_p (j))
+  {
+      stmt = gsi_stmt (j);
+
+      switch (gimple_code (stmt))
+      {
+	case GIMPLE_COND:
+	  {
+	    gcond *cstmt = as_a <gcond *> (stmt);
+	    tree true_label = gimple_cond_true_label (cstmt);
+	    tree false_label = gimple_cond_false_label (cstmt);
+	    target_labels->add (true_label);
+	    target_labels->add (false_label);
+	  }
+	  break;
+	case GIMPLE_SWITCH:
+	  {
+	    gswitch *gstmt = as_a <gswitch *> (stmt);
+	    size_t i, n = gimple_switch_num_labels (gstmt);
+	    tree elt, label;
+	    for (i = 0; i < n; i++)
+	    {
+	      elt = gimple_switch_label (gstmt, i);
+	      label = CASE_LABEL (elt);
+	      target_labels->add (label);
+	    }
+	  }
+	  break;
+	case GIMPLE_GOTO:
+	  if (!computed_goto_p (stmt))
+	    {
+	      tree dest = gimple_goto_dest (stmt);
+	      target_labels->add (dest);
+	    }
+	  break;
+	case GIMPLE_ASM:
+	  {
+	    gasm *asm_stmt = as_a <gasm *> (stmt);
+	    int i, n = gimple_asm_nlabels (asm_stmt);
+	    for (i = 0; i < n; ++i)
+	    {
+	      tree cons = gimple_asm_label_op (asm_stmt, i);
+	      target_labels->add (cons);
+	    }
+	  }
+	  break;
+
+	default:
+	  break;
+      }
+
+      gsi_next (&j);
+  }
+}
+
 
 /* Insert SEQ after BB and build a flowgraph.  */
 
@@ -532,6 +594,10 @@  make_blocks_1 (gimple_seq seq, basic_block bb)
   gimple *prev_stmt = NULL;
   bool start_new_block = true;
   bool first_stmt_of_seq = true;
+  hash_set<tree> target_labels;
+
+  if (!optimize)
+    compute_target_labels (seq, &target_labels);
 
   while (!gsi_end_p (i))
     {
@@ -553,7 +619,7 @@  make_blocks_1 (gimple_seq seq, basic_block bb)
       /* If the statement starts a new basic block or if we have determined
 	 in a previous pass that we need to create a new block for STMT, do
 	 so now.  */
-      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
+      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt, &target_labels))
 	{
 	  if (!first_stmt_of_seq)
 	    gsi_split_seq_before (&i, &seq);
@@ -566,6 +632,9 @@  make_blocks_1 (gimple_seq seq, basic_block bb)
 	 codes.  */
       gimple_set_bb (stmt, bb);
 
+      if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
+	target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
+
       /* If STMT is a basic block terminator, set START_NEW_BLOCK for the
 	 next iteration.  */
       if (stmt_ends_bb_p (stmt))
@@ -853,6 +922,12 @@  make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index)
   bool fallthru = false;
   int ret = 0;
 
+  if (!optimize && !last)
+    {
+      make_edge (bb, bb->next_bb, EDGE_FALLTHRU);
+      return 0;
+    }
+
   if (!last)
     return ret;
 
@@ -1152,6 +1227,10 @@  same_line_p (location_t locus1, expanded_location *from, location_t locus2)
 {
   expanded_location to;
 
+  if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
+      && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
+    return false;
+
   if (locus1 == locus2)
     return true;
 
@@ -2832,7 +2911,8 @@  simple_goto_p (gimple *t)
    label.  */
 
 static inline bool
-stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
+stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
+		  hash_set<tree> *target_labels)
 {
   if (stmt == NULL)
     return false;
@@ -2860,6 +2940,17 @@  stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
 	      || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
 	    return true;
 
+	  location_t prev_locus = gimple_location (plabel);
+	  location_t locus = gimple_location (label_stmt);
+	  expanded_location locus_e = expand_location (locus);
+
+	  if (!optimize
+	      && target_labels->contains (gimple_label_label (label_stmt))
+	      && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
+		|| LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
+	      && !same_line_p (locus, &locus_e, prev_locus))
+	    return true;
+
 	  cfg_stats.num_merged_labels++;
 	  return false;
 	}
diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index ee383b480a8..01e7084fb03 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -263,7 +263,7 @@  test_switch (int i, int j)
       case 2:
         result = do_something (1024);
         break;
-      case 3:				/* count(3) */
+      case 3:				/* count(2) */
       case 4:
 					/* branch(67) */
         if (j == 2)			/* count(3) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index da7929ef7fc..792cda8cfce 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -122,7 +122,7 @@  top:
       }
     else
       {
-else_:				/* count(1) */
+else_:				/* count(2) */
 	j = do_something (j);	/* count(2) */
 	if (j)			/* count(2) */
 	  {
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
index 73e50b19fc7..b37e760910c 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
@@ -7,7 +7,7 @@  int doit(int sel, int n, void *p)
 
   switch (sel)
   {
-    case 0: /* count(3) */
+    case 0: /* count(1) */
       do {*p0 += *p0;} while (--n); /* count(3) */
       return *p0 == 0; /* count(1) */
 
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
new file mode 100644
index 00000000000..2fe340c4011
--- /dev/null
+++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
@@ -0,0 +1,24 @@ 
+/* { dg-options "-fprofile-arcs -ftest-coverage" } */
+/* { dg-do run { target native } } */
+
+int f(int s, int n)
+{
+  int p = 0;
+
+  switch (s)
+  {
+    case 0: /* count(1) */
+      do { p++; } while (--n); /* count(5) */
+      return p; /* count(1) */
+
+    case 1: /* count(1) */
+      do { p++; } while (--n); /* count(5) */
+      return p; /* count(1) */
+  }
+
+  return 0;
+}
+
+int main() { f(0, 5); f(1, 5); return 0; }
+
+/* { dg-final { run-gcov gcov-pr93680.c } } */
diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
index 80e74aeb220..07e1978d25d 100644
--- a/gcc/testsuite/lib/gcov.exp
+++ b/gcc/testsuite/lib/gcov.exp
@@ -424,9 +424,7 @@  proc run-gcov { args } {
     }
     if { $tfailed > 0 } {
 	fail "$testname gcov: $lfailed failures in line counts, $bfailed in branch percentages, $cfailed in return percentages, $ifailed in intermediate format"
-	if { $xfailed } {
-	    clean-gcov $testcase
-	}
+	clean-gcov $testcase
     } else {
 	pass "$testname gcov"
 	clean-gcov $testcase