diff mbox

[PR61776] verify_flow_info failed: control flow in the middle of basic block with -fprofile-generate

Message ID CA+4CFy7a=CbTVSnpchKBqQjomw8Fc6kHWK2BkVDBVddyCRwO5A@mail.gmail.com
State New
Headers show

Commit Message

Wei Mi July 28, 2014, 6:08 a.m. UTC
> But fact is that it is _not_ necessary to split the block because there
> are no outgoing abnormal edges from it.
>
> The verifier failure is an artifact from using the same predicates during
> CFG building and CFG verifying (usually ok, but for this particular
> case it leads to this issue).
>
> So I don't think your patch is the proper way to address this issue
> (while it certainly works).
>
> Instead whether a call can make abnormal gotos should be recorded
> per-call and stored on the gimple-call.  Martin - this is exactly
> one of the cases your patch would address?
>

Thanks for the comment and thanks to Martin's patch. I try the patch.
It works well to address both pr60449 and pr61776 after some
extension. One extension is to replace GF_CALL_LEAF attribute using
GF_CALL_NO_ABNORMAL_GOTO. That is because not only dropping "leaf"
attribute in lto symbol merge could introduce the control flow
verification problem in pr60449, dropping "const/pure" attributes
could introduce the same problem too. It is unnecessary to introduce
per-call attributes for all these three: ECF_LEAF/ECF_CONST/ECF_PURE,
so GF_CALL_NO_ABNORMAL_GOTO is introduced to indicate that a call stmt
has no abnormal goto.

GF_CALL_NO_ABNORMAL_GOTO will be set according to gimple_call_flags()
once gimple call stmt is created, then updated in execute_fixup_cfg
and cleanup_tree_cfg.

I posted the extended patch here. I didn't add the noreturn part in
because it has no direct impact on pr60449 and pr61776. I can help
Martin to test and post that part as an independent patch later.

bootstrap and regression pass on x86_64-linux-gnu. Is it ok?

Thanks,
Wei.
ChangeLog:

2014-07-27  Martin Jambor  <mjambor@suse.cz>
	    Wei Mi  <wmi@google.com>

	PR ipa/60449
	PR middle-end/61776
	* tree-cfgcleanup.c (update_no_abnormal_goto_attr): New function.
	(cleanup_tree_cfg_1): Use update_no_abnormal_goto_attr.
	* gimple.c (gimple_call_initialize_no_abnormal_goto): New function.
	(gimple_build_call_1): Use gimple_call_initialize_no_abnormal_goto.
	(gimple_build_call_internal_1): Ditto.
	* gimple.h (enum gf_mask): Added GF_NO_ABNORMAL_GOTO.
	(gimple_call_set_no_abnormal_goto): New function.
	(gimple_call_no_abnormal_goto_p): Ditto.
	* tree-cfg.c (call_can_make_abnormal_goto):
	Use gimple_call_no_abnormal_goto_p.
	(execute_fixup_cfg): Use gimple_call_set_no_abnormal_goto.

2014-07-27  Martin Jambor  <mjambor@suse.cz>
	    Wei Mi  <wmi@google.com>

	PR ipa/60449
	PR middle-end/61776
	* testsuite/gcc.dg/pr61776.c: New test.
	* testsuite/gcc.dg/lto/pr60449_1.c: New test.
	* testsuite/gcc.dg/lto/pr60449_0.c: New test.

Comments

Wei Mi Aug. 12, 2014, 8:56 p.m. UTC | #1
Ping.

On Sun, Jul 27, 2014 at 11:08 PM, Wei Mi <wmi@google.com> wrote:
>> But fact is that it is _not_ necessary to split the block because there
>> are no outgoing abnormal edges from it.
>>
>> The verifier failure is an artifact from using the same predicates during
>> CFG building and CFG verifying (usually ok, but for this particular
>> case it leads to this issue).
>>
>> So I don't think your patch is the proper way to address this issue
>> (while it certainly works).
>>
>> Instead whether a call can make abnormal gotos should be recorded
>> per-call and stored on the gimple-call.  Martin - this is exactly
>> one of the cases your patch would address?
>>
>
> Thanks for the comment and thanks to Martin's patch. I try the patch.
> It works well to address both pr60449 and pr61776 after some
> extension. One extension is to replace GF_CALL_LEAF attribute using
> GF_CALL_NO_ABNORMAL_GOTO. That is because not only dropping "leaf"
> attribute in lto symbol merge could introduce the control flow
> verification problem in pr60449, dropping "const/pure" attributes
> could introduce the same problem too. It is unnecessary to introduce
> per-call attributes for all these three: ECF_LEAF/ECF_CONST/ECF_PURE,
> so GF_CALL_NO_ABNORMAL_GOTO is introduced to indicate that a call stmt
> has no abnormal goto.
>
> GF_CALL_NO_ABNORMAL_GOTO will be set according to gimple_call_flags()
> once gimple call stmt is created, then updated in execute_fixup_cfg
> and cleanup_tree_cfg.
>
> I posted the extended patch here. I didn't add the noreturn part in
> because it has no direct impact on pr60449 and pr61776. I can help
> Martin to test and post that part as an independent patch later.
>
> bootstrap and regression pass on x86_64-linux-gnu. Is it ok?
>
> Thanks,
> Wei.
Richard Biener Aug. 14, 2014, 2:31 p.m. UTC | #2
On Mon, Jul 28, 2014 at 8:08 AM, Wei Mi <wmi@google.com> wrote:
>> But fact is that it is _not_ necessary to split the block because there
>> are no outgoing abnormal edges from it.
>>
>> The verifier failure is an artifact from using the same predicates during
>> CFG building and CFG verifying (usually ok, but for this particular
>> case it leads to this issue).
>>
>> So I don't think your patch is the proper way to address this issue
>> (while it certainly works).
>>
>> Instead whether a call can make abnormal gotos should be recorded
>> per-call and stored on the gimple-call.  Martin - this is exactly
>> one of the cases your patch would address?
>>
>
> Thanks for the comment and thanks to Martin's patch. I try the patch.
> It works well to address both pr60449 and pr61776 after some
> extension. One extension is to replace GF_CALL_LEAF attribute using
> GF_CALL_NO_ABNORMAL_GOTO. That is because not only dropping "leaf"
> attribute in lto symbol merge could introduce the control flow
> verification problem in pr60449, dropping "const/pure" attributes
> could introduce the same problem too. It is unnecessary to introduce
> per-call attributes for all these three: ECF_LEAF/ECF_CONST/ECF_PURE,
> so GF_CALL_NO_ABNORMAL_GOTO is introduced to indicate that a call stmt
> has no abnormal goto.
>
> GF_CALL_NO_ABNORMAL_GOTO will be set according to gimple_call_flags()
> once gimple call stmt is created, then updated in execute_fixup_cfg
> and cleanup_tree_cfg.
>
> I posted the extended patch here. I didn't add the noreturn part in
> because it has no direct impact on pr60449 and pr61776. I can help
> Martin to test and post that part as an independent patch later.
>
> bootstrap and regression pass on x86_64-linux-gnu. Is it ok?

+static void
+update_no_abnormal_goto_attr (basic_block bb)
+{
+  gimple_stmt_iterator gsi;
+  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+    {

it should be enough to check these on last stmts of a basic block, no?

That you call update_no_abnormal_goto_attr from two places
before cleanup_tree_cfg_bb suggests you may want to perform
this change in cleanup_control_flow_bb which already looks
at the last stmt only?

Btw, I originally had this whole idea of moving flags to the gimple
stmt level because of the "interesting" way we handle the noreturn
attribute (calls to noreturn functions also end basic-blocks).

Thus would it be possible to turn all these into a single flag,
GF_CALL_CTRL_ALTERING?  That is, cover everything
that is_ctrl_altering_stmt covers?  I suggest we initialize it at
CFG build time and only ever clear it later.

Sorry for not thinking about this earlier (and for the slow review).

Thanks,
Richard.

> Thanks,
> Wei.
diff mbox

Patch

Index: tree-cfgcleanup.c
===================================================================
--- tree-cfgcleanup.c	(revision 212442)
+++ tree-cfgcleanup.c	(working copy)
@@ -621,6 +621,28 @@  split_bbs_on_noreturn_calls (void)
   return changed;
 }
 
+/* Update GF_NO_ABNORMAL_GOTO attribute for call stmts in BB according
+   to gimple_call_flags.  */
+
+static void
+update_no_abnormal_goto_attr (basic_block bb)
+{
+  gimple_stmt_iterator gsi;
+  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+    {
+      gimple stmt = gsi_stmt (gsi);
+
+      if (!is_gimple_call (stmt))
+	continue;
+
+      int flags = gimple_call_flags (stmt);
+      if ((flags & (ECF_CONST | ECF_PURE)
+           && !(flags & ECF_LOOPING_CONST_OR_PURE))
+	  || (flags & ECF_LEAF))
+	gimple_call_set_no_abnormal_goto (stmt, true);
+    }
+}
+
 /* Tries to cleanup cfg in basic block BB.  Returns true if anything
    changes.  */
 
@@ -672,7 +694,10 @@  cleanup_tree_cfg_1 (void)
     {
       bb = BASIC_BLOCK_FOR_FN (cfun, i);
       if (bb)
-	retval |= cleanup_tree_cfg_bb (bb);
+	{
+	  update_no_abnormal_goto_attr (bb);
+	  retval |= cleanup_tree_cfg_bb (bb);
+	}
     }
 
   /* Now process the altered blocks, as long as any are available.  */
@@ -687,6 +712,7 @@  cleanup_tree_cfg_1 (void)
       if (!bb)
 	continue;
 
+      update_no_abnormal_goto_attr (bb);
       retval |= cleanup_tree_cfg_bb (bb);
 
       /* Rerun split_bbs_on_noreturn_calls, in case we have altered any noreturn
Index: gimple.c
===================================================================
--- gimple.c	(revision 212442)
+++ gimple.c	(working copy)
@@ -186,6 +186,19 @@  gimple_build_return (tree retval)
   return s;
 }
 
+/* Set GF_NO_ABNORMAL_GOTO attribute according to gimple_call_flags(STMT).  */
+
+void
+gimple_call_initialize_no_abnormal_goto (gimple stmt)
+{
+  int flags = gimple_call_flags (stmt);
+
+  if ((flags & (ECF_CONST | ECF_PURE)
+       && !(flags & ECF_LOOPING_CONST_OR_PURE))
+      || (flags & ECF_LEAF))
+    gimple_call_set_no_abnormal_goto (stmt, true);
+}
+
 /* Reset alias information on call S.  */
 
 void
@@ -215,6 +228,7 @@  gimple_build_call_1 (tree fn, unsigned n
   gimple_set_op (s, 1, fn);
   gimple_call_set_fntype (s, TREE_TYPE (TREE_TYPE (fn)));
   gimple_call_reset_alias_info (s);
+  gimple_call_initialize_no_abnormal_goto (s);
   return s;
 }
 
@@ -290,6 +304,7 @@  gimple_build_call_internal_1 (enum inter
   s->subcode |= GF_CALL_INTERNAL;
   gimple_call_set_internal_fn (s, fn);
   gimple_call_reset_alias_info (s);
+  gimple_call_initialize_no_abnormal_goto (s);
   return s;
 }
 
Index: gimple.h
===================================================================
--- gimple.h	(revision 212442)
+++ gimple.h	(working copy)
@@ -90,6 +90,7 @@  enum gf_mask {
     GF_CALL_NOTHROW		= 1 << 4,
     GF_CALL_ALLOCA_FOR_VAR	= 1 << 5,
     GF_CALL_INTERNAL		= 1 << 6,
+    GF_CALL_NO_ABNORMAL_GOTO    = 1 << 7,
     GF_OMP_PARALLEL_COMBINED	= 1 << 0,
     GF_OMP_FOR_KIND_MASK	= (1 << 2) - 1,
     GF_OMP_FOR_KIND_FOR		= 0,
@@ -2449,7 +2450,28 @@  gimple_call_internal_p (const_gimple gs)
 }
 
 
-/* Return the target of internal call GS.  */
+/* If NO_ABNORMAL_GOTO_P is true mark GIMPLE_CALL S as not having any
+   abnormal goto effect. It doesn't exclude EH.  */
+static inline void
+gimple_call_set_no_abnormal_goto (gimple s, bool no_abnormal_goto_p)
+{
+  GIMPLE_CHECK (s, GIMPLE_CALL);
+  if (no_abnormal_goto_p)
+    s->subcode |= GF_CALL_NO_ABNORMAL_GOTO;
+  else
+    s->subcode &= ~GF_CALL_NO_ABNORMAL_GOTO;
+}
+
+/* Return true if call GS calls an func without abnormal goto. Such call
+   could be a stmt in the middle of a bb.  */
+
+static inline bool
+gimple_call_no_abnormal_goto_p (const_gimple gs)
+{
+  GIMPLE_CHECK (gs, GIMPLE_CALL);
+  return (gs->subcode & GF_CALL_NO_ABNORMAL_GOTO) != 0;
+}
+
 
 static inline enum internal_fn
 gimple_call_internal_fn (const_gimple gs)
Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 212442)
+++ tree-cfg.c	(working copy)
@@ -2318,15 +2318,11 @@  call_can_make_abnormal_goto (gimple t)
       && !cfun->calls_setjmp)
    return false;
 
-  /* Likewise if the call has no side effects.  */
-  if (!gimple_has_side_effects (t))
+  /* If the call has been marked as having no abnormal goto.  */
+  if (gimple_call_no_abnormal_goto_p (t))
     return false;
-
-  /* Likewise if the called function is leaf.  */
-  if (gimple_call_flags (t) & ECF_LEAF)
-    return false;
-
-  return true;
+  else
+    return true;
 }
 
 
@@ -8485,6 +8481,11 @@  execute_fixup_cfg (void)
 		    }
 		}
 
+	      if ((flags & (ECF_CONST | ECF_PURE)
+		   && !(flags & ECF_LOOPING_CONST_OR_PURE))
+		  || (flags & ECF_LEAF))
+		gimple_call_set_no_abnormal_goto (stmt, true);
+
 	      if (flags & ECF_NORETURN
 		  && fixup_noreturn_call (stmt))
 		todo |= TODO_cleanup_cfg;
Index: testsuite/gcc.dg/pr61776.c
===================================================================
--- testsuite/gcc.dg/pr61776.c	(revision 0)
+++ testsuite/gcc.dg/pr61776.c	(revision 0)
@@ -0,0 +1,27 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fprofile-generate" } */
+
+#include <setjmp.h>
+
+int cond1, cond2;
+
+int goo() __attribute__((noinline));
+
+int goo() {
+ if (cond1)
+   return 1;
+ else
+   return 2;
+}
+
+jmp_buf env;
+int foo() {
+ int a;
+
+ setjmp(env);
+ if (cond2)
+   a = goo();
+ else
+   a = 3;
+ return a;
+}
Index: testsuite/gcc.dg/lto/pr60449_1.c
===================================================================
--- testsuite/gcc.dg/lto/pr60449_1.c	(revision 0)
+++ testsuite/gcc.dg/lto/pr60449_1.c	(revision 0)
@@ -0,0 +1,76 @@ 
+extern int printf (const char *__restrict __format, ...);
+typedef long int __time_t;
+typedef long int __suseconds_t;
+struct timeval
+  {
+    __time_t tv_sec;
+    __suseconds_t tv_usec;
+  };
+struct timezone
+  {
+    int tz_minuteswest;
+    int tz_dsttime;
+  };
+typedef struct timezone *__restrict __timezone_ptr_t;
+extern int gettimeofday (struct timeval *__restrict __tv,
+    __timezone_ptr_t __tz) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__nonnull__ (1)));
+
+typedef long int __jmp_buf[8];
+typedef struct
+  {
+    unsigned long int __val[(1024 / (8 * sizeof (unsigned long int)))];
+  } __sigset_t;
+struct __jmp_buf_tag
+  {
+    __jmp_buf __jmpbuf;
+    int __mask_was_saved;
+    __sigset_t __saved_mask;
+  };
+typedef struct __jmp_buf_tag jmp_buf[1];
+
+extern int setjmp (jmp_buf __env) __attribute__ ((__nothrow__));
+extern void longjmp (struct __jmp_buf_tag __env[1], int __val)
+     __attribute__ ((__nothrow__)) __attribute__ ((__noreturn__));
+
+extern int bar (void);
+
+int __attribute__ ((noinline, noclone))
+get_input (void)
+{
+  return 0;
+}
+
+static jmp_buf buf;
+
+int foo (void)
+{
+  if (get_input ())
+    longjmp(buf, 1);
+  return 0;
+}
+
+volatile int z;
+
+
+int main (void)
+{
+  struct timeval tv;
+  struct timezone tz;
+
+  bar();
+  if (setjmp (buf))
+    return 1;
+
+  if (!get_input ())
+    {
+      gettimeofday (&tv, &tz);
+      z = 0;
+      printf ("This is from main %i\n", tz.tz_dsttime);
+    }
+
+  foo ();
+  bar ();
+  bar ();
+
+  return 0;
+}
Index: testsuite/gcc.dg/lto/pr60449_0.c
===================================================================
--- testsuite/gcc.dg/lto/pr60449_0.c	(revision 0)
+++ testsuite/gcc.dg/lto/pr60449_0.c	(revision 0)
@@ -0,0 +1,30 @@ 
+/* { dg-lto-do link } */
+
+extern int printf (const char *__restrict __format, ...);
+typedef long int __time_t;
+typedef long int __suseconds_t;
+
+struct timeval
+  {
+    __time_t tv_sec;
+    __suseconds_t tv_usec;
+  };
+
+struct timezone
+  {
+    int tz_minuteswest;
+    int tz_dsttime;
+  };
+typedef struct timezone *__restrict __timezone_ptr_t;
+
+extern int gettimeofday (struct timeval *__restrict __tv, __timezone_ptr_t __tz);
+
+int bar (void)
+{
+  struct timeval tv;
+  struct timezone tz;
+
+  gettimeofday (&tv, &tz);
+  printf ("This is from bar %i\n", tz.tz_dsttime);
+  return 5;
+}