Patchwork Avoid ivopts when loop contains vfork

login
register
mail settings
Submitter Bernd Schmidt
Date Feb. 10, 2011, 2:31 p.m.
Message ID <4D53F6B6.3020006@codesourcery.com>
Download mbox | patch
Permalink /patch/82609/
State New
Headers show

Comments

Bernd Schmidt - Feb. 10, 2011, 2:31 p.m.
When a loop contains vfork, certain loop optimizations can be incorrect.
This was seen with an unreleased port in the LTP testcase waitpid03,
with the relevant part of the code looking like this:

               while (++ikids < MAXUPRC) {
                        if ((pid[ikids] = FORK_OR_VFORK()) > 0) {

This was transformed into an autoinc, i.e. something like

               while (ikids < MAXUPRC - 1) {
                        if ((pid[ikids++] = FORK_OR_VFORK()) > 0) {

ikids did not get a hard register, which means that the copy on the
stack was incremented twice, once for each return from vfork.

In our tree, I fixed it with the following patch.  Note that, despite
the name, setjmp_call_p really tests ECF_RETURNS_TWICE, which is true
for vfork as well.

Bootstrapped and regression tested on i686-linux.  Ok?


Bernd
* tree-ssa-loop-ivopts.c (loop_body_includes_call): Change return
	value to integer.  Check whether the loop calls special functions.
	(tree_ssa_iv_optimize_loop): Return if we find it does.
Richard Guenther - Feb. 10, 2011, 4:26 p.m.
On Thu, Feb 10, 2011 at 3:31 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> When a loop contains vfork, certain loop optimizations can be incorrect.
> This was seen with an unreleased port in the LTP testcase waitpid03,
> with the relevant part of the code looking like this:
>
>               while (++ikids < MAXUPRC) {
>                        if ((pid[ikids] = FORK_OR_VFORK()) > 0) {
>
> This was transformed into an autoinc, i.e. something like
>
>               while (ikids < MAXUPRC - 1) {
>                        if ((pid[ikids++] = FORK_OR_VFORK()) > 0) {
>
> ikids did not get a hard register, which means that the copy on the
> stack was incremented twice, once for each return from vfork.

Err, but isn't the stack copied by fork?  It isn't obvious to me that
IVOPTs can't handle returning twice functions transparently (it has
to consider us going into both arms of the if anyways).

Richard.

> In our tree, I fixed it with the following patch.  Note that, despite
> the name, setjmp_call_p really tests ECF_RETURNS_TWICE, which is true
> for vfork as well.
>
> Bootstrapped and regression tested on i686-linux.  Ok?
>
>
> Bernd
>
>
Bernd Schmidt - Feb. 10, 2011, 4:43 p.m.
On 02/10/2011 05:26 PM, Richard Guenther wrote:
> Err, but isn't the stack copied by fork?

fork yes, vfork no - the latter does not duplicate any memory. You end
up with two processes using the same memory space for a brief period.

>  It isn't obvious to me that
> IVOPTs can't handle returning twice functions transparently (it has
> to consider us going into both arms of the if anyways).

Maybe we could handle it, but I feel there's no reason to try for
something that occurs extremely rarely. It should me more reliable to
just skip ivopts entirely.


Bernd
Michael Matz - Feb. 10, 2011, 4:46 p.m.
Hi,

On Thu, 10 Feb 2011, Richard Guenther wrote:

> On Thu, Feb 10, 2011 at 3:31 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> > When a loop contains vfork, certain loop optimizations can be incorrect.
> > This was seen with an unreleased port in the LTP testcase waitpid03,
> > with the relevant part of the code looking like this:
> >
> >               while (++ikids < MAXUPRC) {
> >                        if ((pid[ikids] = FORK_OR_VFORK()) > 0) {
> >
> > This was transformed into an autoinc, i.e. something like
> >
> >               while (ikids < MAXUPRC - 1) {
> >                        if ((pid[ikids++] = FORK_OR_VFORK()) > 0) {
> >
> > ikids did not get a hard register, which means that the copy on the
> > stack was incremented twice, once for each return from vfork.
> 
> Err, but isn't the stack copied by fork?

Yeah, but not by vfork.  Still the testcase is simply invalid, the _only_ 
things allowed after calling vfork is capturing the return value in a 
variable of type pid_t, calling exit, or calling exec*.  The above stores 
into a variable of type pid_t[] --> Invalid.

> It isn't obvious to me that IVOPTs can't handle returning twice 
> functions transparently (it has to consider us going into both arms of 
> the if anyways).

Right, it seems a sledge-hammer approach, and for a very general problem 
at that.  On OSes that implement vfork in the most restrictive sense (as 
in not separating the memory mappings), the compiler has to ensure that it 
doesn't store to any memory at all between the vfork/(exec|exit) sequence.  
For instance spills of the return value would be forbidden.  As long as we 
have those problems I don't see why we should change ivopts for an invalid 
testcase.


Ciao,
Michael.

Patch

Index: tree-ssa-loop-ivopts.c
===================================================================
--- tree-ssa-loop-ivopts.c	(revision 169892)
+++ tree-ssa-loop-ivopts.c	(working copy)
@@ -6325,9 +6325,12 @@  tree_ssa_iv_optimize_finalize (struct iv
   htab_delete (data->inv_expr_tab);
 }
 
-/* Returns true if the loop body BODY includes any function calls.  */
+/* Returns a nonzero value if the loop body BODY includes any function
+   calls.  For normal calls, we return a positive value; if there is
+   something special (like a call to vfork) that should prevent us from
+   optimizing the loop, we return a negative value.  */
 
-static bool
+static int
 loop_body_includes_call (basic_block *body, unsigned num_nodes)
 {
   gimple_stmt_iterator gsi;
@@ -6337,11 +6340,16 @@  loop_body_includes_call (basic_block *bo
     for (gsi = gsi_start_bb (body[i]); !gsi_end_p (gsi); gsi_next (&gsi))
       {
 	gimple stmt = gsi_stmt (gsi);
-	if (is_gimple_call (stmt)
-	    && !is_inexpensive_builtin (gimple_call_fndecl (stmt)))
-	  return true;
+	if (is_gimple_call (stmt))
+	  {
+	    tree callee_t = gimple_call_fndecl (stmt);
+	    if (setjmp_call_p (callee_t))
+	      return -1;
+	    if (!is_inexpensive_builtin (callee_t))
+	      return 1;
+	  }
       }
-  return false;
+  return 0;
 }
 
 /* Optimizes the LOOP.  Returns true if anything changed.  */
@@ -6353,6 +6361,7 @@  tree_ssa_iv_optimize_loop (struct ivopts
   struct iv_ca *iv_ca;
   edge exit;
   basic_block *body;
+  int t;
 
   gcc_assert (!data->niters);
   data->current_loop = loop;
@@ -6375,7 +6384,14 @@  tree_ssa_iv_optimize_loop (struct ivopts
     }
 
   body = get_loop_body (loop);
-  data->body_includes_call = loop_body_includes_call (body, loop->num_nodes);
+  t = loop_body_includes_call (body, loop->num_nodes);
+  if (t >= 0)
+    data->body_includes_call = t > 0;
+  else
+    {
+      free (body);
+      goto finish;
+    }
   renumber_gimple_stmt_uids_in_blocks (body, loop->num_nodes);
   free (body);