| Message ID | be4a21f7-998e-9922-e220-a27e1eca03aa@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 1, 2016 at 12:03 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > This looks like a latent problem in the -Wmaybe-uninitialized code unrelated > to my work. > > The problem here is a sequence of simplify_preds() followed by > normalize_preds() that I added, but is based on prior art all over the file: > > + simplify_preds (&uninit_preds, NULL, false); > + uninit_preds = normalize_preds (uninit_preds, NULL, false); > > The problem is that in this particular testcase we trigger simplify_preds_4 > which makes a copy of a chain, frees the chain, and then tries to use the > chain later (in normalize_preds). The normalize_preds() function tries to > free again the chain and we blow up: > > This is the main problem in simplify_preds_4: > > /* Now clean up the chain. */ > if (simplified) > { > for (i = 0; i < n; i++) > { > if ((*preds)[i].is_empty ()) > continue; > s_preds.safe_push ((*preds)[i]); > // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > // Makes a copy of the pred_chain. > } > > destroy_predicate_vecs (preds); > // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > // free() all the pred_chain's. > > (*preds) = s_preds; > // ^^^^^^^^^^^^^^^^^^^^^^ > // Wait a minute, we still keep a copy of the pred_chains. > s_preds = vNULL; > } > > I have no idea how this worked even before my patch. Perhaps we never had a > simplify_preds() followed by a normalize_preds() where the simplification > involved a call to simplify_preds_4. > > Interestingly enough, simplify_preds_2() has the exact same code, but with > the fix I am proposing. So this seems like an oversight. Also, the fact > that the simplification in simplify_preds_2 is more common than the > simplification performed in simplify_preds_4 would suggest that > simplify_preds_4 was uncommon enough and probably wasn't been used in a > simplify_preds/normalize_preds combo. > > Anyways... I've made some other cleanups to the code, but the main gist of > the entire patch is: > > - destroy_predicate_vecs (preds); > + preds->release (); > > That is, release preds, but don't free the associated memory with the > pred_chain's therein. > > This patch is on top of my pending patch here: > > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02900.html > > Tested on x86-64 Linux. > > OK for trunk? Ok. Richard. > Aldy
On 12/01/2016 07:32 AM, Richard Biener wrote: > On Thu, Dec 1, 2016 at 12:03 PM, Aldy Hernandez <aldyh@redhat.com> wrote: >> This looks like a latent problem in the -Wmaybe-uninitialized code unrelated >> to my work. >> >> The problem here is a sequence of simplify_preds() followed by >> normalize_preds() that I added, but is based on prior art all over the file: >> >> + simplify_preds (&uninit_preds, NULL, false); >> + uninit_preds = normalize_preds (uninit_preds, NULL, false); >> >> The problem is that in this particular testcase we trigger simplify_preds_4 >> which makes a copy of a chain, frees the chain, and then tries to use the >> chain later (in normalize_preds). The normalize_preds() function tries to >> free again the chain and we blow up: >> >> This is the main problem in simplify_preds_4: >> >> /* Now clean up the chain. */ >> if (simplified) >> { >> for (i = 0; i < n; i++) >> { >> if ((*preds)[i].is_empty ()) >> continue; >> s_preds.safe_push ((*preds)[i]); >> // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> // Makes a copy of the pred_chain. >> } >> >> destroy_predicate_vecs (preds); >> // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> // free() all the pred_chain's. >> >> (*preds) = s_preds; >> // ^^^^^^^^^^^^^^^^^^^^^^ >> // Wait a minute, we still keep a copy of the pred_chains. >> s_preds = vNULL; >> } >> >> I have no idea how this worked even before my patch. Perhaps we never had a >> simplify_preds() followed by a normalize_preds() where the simplification >> involved a call to simplify_preds_4. >> >> Interestingly enough, simplify_preds_2() has the exact same code, but with >> the fix I am proposing. So this seems like an oversight. Also, the fact >> that the simplification in simplify_preds_2 is more common than the >> simplification performed in simplify_preds_4 would suggest that >> simplify_preds_4 was uncommon enough and probably wasn't been used in a >> simplify_preds/normalize_preds combo. >> >> Anyways... I've made some other cleanups to the code, but the main gist of >> the entire patch is: >> >> - destroy_predicate_vecs (preds); >> + preds->release (); >> >> That is, release preds, but don't free the associated memory with the >> pred_chain's therein. >> >> This patch is on top of my pending patch here: >> >> https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02900.html >> >> Tested on x86-64 Linux. >> >> OK for trunk? > > Ok. Thank you Richard. Since this is dependent on the other patch I will wait until said patch is approved to commit. Aldy
diff --git a/gcc/testsuite/gcc.dg/uninit-pr78548.c b/gcc/testsuite/gcc.dg/uninit-pr78548.c new file mode 100644 index 0000000..12e06dd --- /dev/null +++ b/gcc/testsuite/gcc.dg/uninit-pr78548.c @@ -0,0 +1,24 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -w -O2" } */ + +char a; +int b; +unsigned c, d; +short e; +int main_f; +int main ( ) { +L0: + if ( e ) goto L1; + b = c & d || a; + if ( !c ) printf ( "", ( long long ) main_f ); + if ( d || !c ) { + printf ( "%llu\n", ( long long ) main ); + goto L2; + } + unsigned g = b; +L1: + b = g; +L2: + if ( b ) goto L0; + return 0; +} diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c index a648995..b4892c7 100644 --- a/gcc/tree-ssa-uninit.c +++ b/gcc/tree-ssa-uninit.c @@ -1774,7 +1774,7 @@ simplify_preds_4 (pred_chain_union *preds) s_preds.safe_push ((*preds)[i]); } - destroy_predicate_vecs (preds); + preds->release (); (*preds) = s_preds; s_preds = vNULL; } @@ -2211,10 +2211,9 @@ uninit_uses_cannot_happen (gphi *phi, unsigned uninit_opnds, /* Look for the control dependencies of all the uninitialized operands and build guard predicates describing them. */ - unsigned i; - pred_chain_union uninit_preds[max_phi_args]; - memset (uninit_preds, 0, sizeof (pred_chain_union) * phi_args); - for (i = 0; i < phi_args; ++i) + pred_chain_union uninit_preds; + bool ret = true; + for (unsigned i = 0; i < phi_args; ++i) { if (!MASK_TEST_BIT (uninit_opnds, i)) continue; @@ -2226,26 +2225,32 @@ uninit_uses_cannot_happen (gphi *phi, unsigned uninit_opnds, int num_calls = 0; /* Build the control dependency chain for uninit operand `i'... */ + uninit_preds = vNULL; if (!compute_control_dep_chain (find_dom (e->src), e->src, dep_chains, &num_chains, &cur_chain, &num_calls)) - return false; + { + ret = false; + break; + } /* ...and convert it into a set of predicates. */ convert_control_dep_chain_into_preds (dep_chains, num_chains, - &uninit_preds[i]); + &uninit_preds); for (size_t j = 0; j < num_chains; ++j) dep_chains[j].release (); - simplify_preds (&uninit_preds[i], NULL, false); - uninit_preds[i] - = normalize_preds (uninit_preds[i], NULL, false); + simplify_preds (&uninit_preds, NULL, false); + uninit_preds = normalize_preds (uninit_preds, NULL, false); /* Can the guard for this uninitialized operand be invalidated by the PHI use? */ - if (!can_chain_union_be_invalidated_p (uninit_preds[i], - phi_use_guards[0])) - return false; + if (!can_chain_union_be_invalidated_p (uninit_preds, phi_use_guards[0])) + { + ret = false; + break; + } } - return true; + destroy_predicate_vecs (&uninit_preds); + return ret; } /* Computes the predicates that guard the use and checks
This looks like a latent problem in the -Wmaybe-uninitialized code unrelated to my work. The problem here is a sequence of simplify_preds() followed by normalize_preds() that I added, but is based on prior art all over the file: + simplify_preds (&uninit_preds, NULL, false); + uninit_preds = normalize_preds (uninit_preds, NULL, false); The problem is that in this particular testcase we trigger simplify_preds_4 which makes a copy of a chain, frees the chain, and then tries to use the chain later (in normalize_preds). The normalize_preds() function tries to free again the chain and we blow up: This is the main problem in simplify_preds_4: /* Now clean up the chain. */ if (simplified) { for (i = 0; i < n; i++) { if ((*preds)[i].is_empty ()) continue; s_preds.safe_push ((*preds)[i]); // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ // Makes a copy of the pred_chain. } destroy_predicate_vecs (preds); // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ // free() all the pred_chain's. (*preds) = s_preds; // ^^^^^^^^^^^^^^^^^^^^^^ // Wait a minute, we still keep a copy of the pred_chains. s_preds = vNULL; } I have no idea how this worked even before my patch. Perhaps we never had a simplify_preds() followed by a normalize_preds() where the simplification involved a call to simplify_preds_4. Interestingly enough, simplify_preds_2() has the exact same code, but with the fix I am proposing. So this seems like an oversight. Also, the fact that the simplification in simplify_preds_2 is more common than the simplification performed in simplify_preds_4 would suggest that simplify_preds_4 was uncommon enough and probably wasn't been used in a simplify_preds/normalize_preds combo. Anyways... I've made some other cleanups to the code, but the main gist of the entire patch is: - destroy_predicate_vecs (preds); + preds->release (); That is, release preds, but don't free the associated memory with the pred_chain's therein. This patch is on top of my pending patch here: https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02900.html Tested on x86-64 Linux. OK for trunk? Aldy commit ec4443b8dcf89465cb8d9735a3e0a27b181c975c Author: Aldy Hernandez <aldyh@redhat.com> Date: Thu Dec 1 04:53:38 2016 -0500 PR middle-end/78548 * tree-ssa-uninit.c (simplify_preds_4): Call release() instead of destroy_predicate_vecs. (uninit_uses_cannot_happen): Make uninit_preds a scalar.