On 12/01/2016 07:32 AM, Richard Biener wrote:
On Thu, Dec 1, 2016 at 12:03 PM, Aldy Hernandez <al...@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
