PR26197 (new_type_alias) fix

[Dorit Nuzman]

Ian Lance Taylor <iant@google.com> wrote on 08/08/2006 06:10:34 PM:

> Dorit Nuzman <DORIT@il.ibm.com> writes:
>
> > Index: tree-ssa-alias.c
> > ===================================================================
> > *** tree-ssa-alias.c   (revision 115685)
> > --- tree-ssa-alias.c   (working copy)
> > *************** static void create_global_var (void);
> > *** 114,119 ****
> > --- 114,120 ----
> >   static void maybe_create_global_var (struct alias_info *ai);
> >   static void group_aliases (struct alias_info *);
> >   static void set_pt_anything (tree ptr);
> > + static tree add_may_alias_for_new_tag (tree tag, tree var);
> >
> >   /* Global declarations.  */
>
> The current gcc convention is to define static functions before their
> use, where possible, and to not bother with an explicit declaration.
> That is, you should omit this declaration, and move the definition of
> add_may_alias_for_new_tag before new_type_alias.
>

ok, fixed

>
> > +   ref = get_ref_base_and_extent (expr, &offset, &size, &maxsize);
> > +   if (ref == NULL_TREE)
> > +     ref = var;
>
> Here you allow for the possibility that EXPR does not have a base and
> extent which can be determined.
>
> >         for (sv = svars; sv; sv = sv->next)
> >      {
> > !           bool exact;
> >
> > !           if (overlap_subvar (offset, maxsize, sv->var, &exact))
> > !             VEC_safe_push (tree, heap, overlaps, sv->var);
> > !         }
>
> Here you seem to assume that OFFSET and MAXSIZE will always be set
> correctly.  It seems to me that in the case where
> get_ref_base_and_extent returns NULL_TREE, then OFFSET and MAXSIZE
> will be such that overlap_subvar will always return false.  That seems
> like the opposite of what you want to occur in that case.  It seems to
> me you should fix that in the obvious way.  Please let me know if you
> disagree.
>

As Daniel pointed out, the way this function is used we always expect for
it to get valid arguments and therefore return a valid result. I'm
bootstrapping now a version with an assert that get_ref_base_and_extent
does not return NULL_TREE.

> > ! /* The following is based on code in add_stmt_operand to ensure that
the
> > !    same defs/uses/vdefs/vuses will be found after replacing a
reference
> > !    to var (or ARRAY_REF to var) with an INDIRECT_REF to ptr whose
value
> > !    is the address of var.  */
>
> This comment needs to explain the return value of
> add_may_alias_for_new_tag.  Also, I believe the reference should be to
> add_virtual_operand, not to add_stmt_operand.
>

This comment was there originally... I added a sentence about the return
value.

>
> Patch is approved with those changes.  Since I'm suggesting a
> functional change, please run another bootstrap, and please confirm
> that the PR is still fixed.

sure

> It wasn't clear to me whether you were
> ever able to recreate the problem; since this is an ICE, you should be
> able to recreate it with a cross-compiler, and you should be able to
> confirm that your patch fixes the problem.  Let us know if you need
> any help with that.
>

I was able to reproduce the ICE with Pinski's reduced testcase, and also
the ICE in the duplicate PR26948, and verify that they were fixed.

> Also, please add the test case to the testsuite.
>

sure. Here's the patch I'm bootstrapping now.

(See attached file: pr26197.aug9.fix.txt)

thanks,
dorit

> Thanks.
>
> :REVIEWMAIL:
>
> Ian
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pr26197.aug9.fix.txt
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20060809/7e1c4b02/attachment.txt>