[RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile

How does this look as a potential fix for PR64703?  I haven't made
many forays into gimple code, so even though this patch passes
bootstrap and regression testing on powerpc64-linux it's quite
possible this is the wrong place to change.  If it does look to be OK,
then I'll fill out the targetm changes, include a testcase etc.

	PR target/64703
	* tree-ssa-alias.c (pt_solution_includes_base): New function,
	extracted from..
	(ref_maybe_used_by_call_p_1): ..here.  Delete dead code checking
	for NULL return from ao_ref_base.  Handle potential memory
	reference by indirect calls on targets using function descriptors.

On Sat, Jan 24, 2015 at 12:23 AM, Alan Modra <amodra@gmail.com> wrote:
> How does this look as a potential fix for PR64703?  I haven't made
> many forays into gimple code, so even though this patch passes
> bootstrap and regression testing on powerpc64-linux it's quite
> possible this is the wrong place to change.  If it does look to be OK,
> then I'll fill out the targetm changes, include a testcase etc.

It looks mostly ok, comments below.

>         PR target/64703
>         * tree-ssa-alias.c (pt_solution_includes_base): New function,
>         extracted from..
>         (ref_maybe_used_by_call_p_1): ..here.  Delete dead code checking
>         for NULL return from ao_ref_base.  Handle potential memory
>         reference by indirect calls on targets using function descriptors.
>
> Index: gcc/tree-ssa-alias.c
> ===================================================================
> --- gcc/tree-ssa-alias.c        (revision 220025)
> +++ gcc/tree-ssa-alias.c        (working copy)
> @@ -1532,6 +1532,23 @@ refs_output_dependent_p (tree store1, tree store2)
>    return refs_may_alias_p_1 (&r1, &r2, false);
>  }
>
> +static bool
> +pt_solution_includes_base (struct pt_solution *pt, tree base)

Needs a comment.

> +{
> +  if (DECL_P (base))
> +    return pt_solution_includes (pt, base);
> +
> +  if ((TREE_CODE (base) == MEM_REF
> +       || TREE_CODE (base) == TARGET_MEM_REF)
> +      && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
> +    {
> +      struct ptr_info_def *pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0));
> +      if (pi)
> +       return pt_solutions_intersect (pt, &pi->pt);
> +    }
> +  return true;
> +}
> +
>  /* If the call CALL may use the memory reference REF return true,
>     otherwise return false.  */
>
> @@ -1542,15 +1559,24 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
>    unsigned i;
>    int flags = gimple_call_flags (call);
>
> +  base = ao_ref_base (ref);

You dropped the

  if (!base)
    return true;

check - please put it back.

> +  callee = gimple_call_fn (call);
> +  if (callee && TREE_CODE (callee) == SSA_NAME

Do we never propagate the address of a function descriptor here?
That is, can we generate a testcase like

   descr fn;
   <setup fn>
   foo (&fn);

void foo (fn)
{
   (*fn) (a, b);
}

and then inline foo so the call becomes

  (*&fn) (a, b);

?  We'd then still implicitely read from 'fn'.  I also wonder whether
(and where!) we resolve such a descriptor reference to the actual
function on GIMPLE?

That is - don't you simply want to use

  if (targetm.function_descriptors
      && ptr_deref_mayalias_ref_p_1 (callee, ref))
    return true;

here?

Thanks,
Richard.

> +      /* && targetm.function_descriptors */)
> +    {
> +      /* Handle indirect call.  When a target defines the address of a
> +        function as that of a function descriptor, then dereferencing
> +        a function pointer implicitly references memory.  */
> +      struct ptr_info_def *pi = SSA_NAME_PTR_INFO (callee);
> +      if (pi && pt_solution_includes_base (&pi->pt, base))
> +       return true;
> +    }
> +
>    /* Const functions without a static chain do not implicitly use memory.  */
>    if (!gimple_call_chain (call)
>        && (flags & (ECF_CONST|ECF_NOVOPS)))
>      goto process_args;
>
> -  base = ao_ref_base (ref);
> -  if (!base)
> -    return true;
> -
>    /* A call that is not without side-effects might involve volatile
>       accesses and thus conflicts with all other volatile accesses.  */
>    if (ref->volatile_p)
> @@ -1564,7 +1590,7 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
>        && !is_global_var (base))
>      goto process_args;
>
> -  callee = gimple_call_fndecl (call);
> +  callee = gimple_call_addr_fndecl (callee);
>
>    /* Handle those builtin functions explicitly that do not act as
>       escape points.  See tree-ssa-structalias.c:find_func_aliases
> @@ -1803,23 +1829,7 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
>      }
>
>    /* Check if the base variable is call-used.  */
> -  if (DECL_P (base))
> -    {
> -      if (pt_solution_includes (gimple_call_use_set (call), base))
> -       return true;
> -    }
> -  else if ((TREE_CODE (base) == MEM_REF
> -           || TREE_CODE (base) == TARGET_MEM_REF)
> -          && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
> -    {
> -      struct ptr_info_def *pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0));
> -      if (!pi)
> -       return true;
> -
> -      if (pt_solutions_intersect (gimple_call_use_set (call), &pi->pt))
> -       return true;
> -    }
> -  else
> +  if (pt_solution_includes_base (gimple_call_use_set (call), base))
>      return true;
>
>    /* Inspect call arguments for passed-by-value aliases.  */
>
> --
> Alan Modra
> Australia Development Lab, IBM

On Mon, Jan 26, 2015 at 10:11:14AM +0100, Richard Biener wrote:
> On Sat, Jan 24, 2015 at 12:23 AM, Alan Modra <amodra@gmail.com> wrote:
> > How does this look as a potential fix for PR64703?  I haven't made
> > many forays into gimple code, so even though this patch passes
> > bootstrap and regression testing on powerpc64-linux it's quite
> > possible this is the wrong place to change.  If it does look to be OK,
> > then I'll fill out the targetm changes, include a testcase etc.
> 
> It looks mostly ok, comments below.

Thanks for looking!

> >         PR target/64703
> >         * tree-ssa-alias.c (pt_solution_includes_base): New function,
> >         extracted from..
> >         (ref_maybe_used_by_call_p_1): ..here.  Delete dead code checking
> >         for NULL return from ao_ref_base.  Handle potential memory
> >         reference by indirect calls on targets using function descriptors.
> >
> > Index: gcc/tree-ssa-alias.c
> > ===================================================================
> > --- gcc/tree-ssa-alias.c        (revision 220025)
> > +++ gcc/tree-ssa-alias.c        (working copy)
> > @@ -1532,6 +1532,23 @@ refs_output_dependent_p (tree store1, tree store2)
> >    return refs_may_alias_p_1 (&r1, &r2, false);
> >  }
> >
> > +static bool
> > +pt_solution_includes_base (struct pt_solution *pt, tree base)
> 
> Needs a comment.

Sure, that was part of "etc." above. ;)

> > +{
> > +  if (DECL_P (base))
> > +    return pt_solution_includes (pt, base);
> > +
> > +  if ((TREE_CODE (base) == MEM_REF
> > +       || TREE_CODE (base) == TARGET_MEM_REF)
> > +      && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME)
> > +    {
> > +      struct ptr_info_def *pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0));
> > +      if (pi)
> > +       return pt_solutions_intersect (pt, &pi->pt);
> > +    }
> > +  return true;
> > +}
> > +
> >  /* If the call CALL may use the memory reference REF return true,
> >     otherwise return false.  */
> >
> > @@ -1542,15 +1559,24 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
> >    unsigned i;
> >    int flags = gimple_call_flags (call);
> >
> > +  base = ao_ref_base (ref);
> 
> You dropped the
> 
>   if (!base)
>     return true;
> 
> check - please put it back.

Hmm, calls to ao_ref_base in tree-ssa-alias.c are a mixed bag.  Some
check the return for NULL, others don't.  All the checks for NULL are
dead code since ao_ref_base never returns NULL.  OK, I'll put it back
and leave cleanup for another day.

> 
> > +  callee = gimple_call_fn (call);
> > +  if (callee && TREE_CODE (callee) == SSA_NAME
> 
> Do we never propagate the address of a function descriptor here?
> That is, can we generate a testcase like
> 
>    descr fn;
>    <setup fn>
>    foo (&fn);
> 
> void foo (fn)
> {
>    (*fn) (a, b);
> }
> 
> and then inline foo so the call becomes
> 
>   (*&fn) (a, b);
> 
> ?  We'd then still implicitely read from 'fn'.  I also wonder whether
> (and where!) we resolve such a descriptor reference to the actual
> function on GIMPLE?

Well, if we did end up with a direct call then the value in 'fn' won't
matter, I think.  A direct call implies gcc knows the value of a
function pointer, and can thus use the value rather than the pointer.
In this case it implies gcc has looked through 'fn' to its
initialization, and seen that 'fn' is a function address.
ie. your <setup fn> above is something like
  fn = *(descr *) &some_function;
Now it appears that gcc isn't clever enough to do this, but if it did,
then surely the "value of the pointer" would be 'some_function', not
'fn'.

I can't see how we could end up with a direct call any other way.  As
far as I know, gcc doesn't have any knowledge that 'descr' has
anything to do with a function address unless that knowledge is
imparted by an initialization such as the above.  ie. The answer to
your "and where!" question is "nowhere".

> That is - don't you simply want to use
> 
>   if (targetm.function_descriptors
>       && ptr_deref_mayalias_ref_p_1 (callee, ref))
>     return true;
> 
> here?

No.  I don't want to do needless work on direct calls, particularly
since it appears that ptr_deref_may_alias_ref_p_1 does return true for
direct calls like memcpy.