| Message ID | 4D53F6B6.3020006@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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.
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);
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.