public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
@ 2015-01-24  2:37 Alan Modra
  2015-01-26  9:56 ` Richard Biener
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Modra @ 2015-01-24  2:37 UTC (permalink / raw)
  To: gcc-patches

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.

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)
+{
+  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);
+  callee = gimple_call_fn (call);
+  if (callee && TREE_CODE (callee) == SSA_NAME
+      /* && 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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-24  2:37 [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile Alan Modra
@ 2015-01-26  9:56 ` Richard Biener
  2015-01-27 14:27   ` Alan Modra
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Biener @ 2015-01-26  9:56 UTC (permalink / raw)
  To: GCC Patches

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-26  9:56 ` Richard Biener
@ 2015-01-27 14:27   ` Alan Modra
  2015-01-29 15:06     ` Alan Modra
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Modra @ 2015-01-27 14:27 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches

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.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-27 14:27   ` Alan Modra
@ 2015-01-29 15:06     ` Alan Modra
  2015-01-29 16:30       ` Richard Biener
  2015-01-29 20:35       ` Segher Boessenkool
  0 siblings, 2 replies; 18+ messages in thread
From: Alan Modra @ 2015-01-29 15:06 UTC (permalink / raw)
  To: Richard Biener, GCC Patches

Here is the completed patch.  Bootstrapped and regression tested
powerpc64-linux.  Is this OK to apply?  If not now, then when gcc is
in stage1 again?

gcc/
	PR target/64703
	* target.def (has_function_descriptors): New hook.
	* doc/tm.texi.in: Add TARGET_HAS_FUNCTION_DESCRIPTORS.
	* doc/tc.texi: Regenerate.
	* tree-ssa-alias.c (pt_solution_includes_base): New function,
	extracted from..
	(ref_maybe_used_by_call_p_1): ..here.  Handle potential memory
	reference by indirect calls on targets using function descriptors.
	* config/rs6000/rs6000.c (TARGET_HAS_FUNCTION_DESCRIPTORS): Define.
	(rs6000_has_function_descriptors): New function.
gcc/testsuite/
	* gcc.target/powerpc/pr64703.c: New.

Index: gcc/target.def
===================================================================
--- gcc/target.def	(revision 220025)
+++ gcc/target.def	(working copy)
@@ -2821,6 +2821,15 @@ The default value of this hook is based on target'
  bool, (void),
  default_has_ifunc_p)
 
+/* True if target defines the address of a function as that of a
+   function descriptor.  */
+DEFHOOK
+(has_function_descriptors,
+ "True if target has function descriptors and defines the address\n\
+of a function as that of a function descriptor.",
+ bool, (void),
+ hook_bool_void_false)
+
 /* True if it is OK to do sibling call optimization for the specified
    call expression EXP.  DECL will be the called function, or NULL if
    this is an indirect call.  */
Index: gcc/doc/tm.texi.in
===================================================================
--- gcc/doc/tm.texi.in	(revision 220025)
+++ gcc/doc/tm.texi.in	(working copy)
@@ -8175,6 +8175,8 @@ and the associated definitions of those functions.
 
 @hook TARGET_HAS_IFUNC_P
 
+@hook TARGET_HAS_FUNCTION_DESCRIPTORS
+
 @hook TARGET_ATOMIC_ALIGN_FOR_MODE
 
 @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
Index: gcc/doc/tm.texi
===================================================================
--- gcc/doc/tm.texi	(revision 220025)
+++ gcc/doc/tm.texi	(working copy)
@@ -11510,6 +11510,11 @@ The support includes the assembler, linker and dyn
 The default value of this hook is based on target's libc.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_HAS_FUNCTION_DESCRIPTORS (void)
+True if target has function descriptors and defines the address
+of a function as that of a function descriptor.
+@end deftypefn
+
 @deftypefn {Target Hook} {unsigned int} TARGET_ATOMIC_ALIGN_FOR_MODE (machine_mode @var{mode})
 If defined, this function returns an appropriate alignment in bits for an atomic object of machine_mode @var{mode}.  If 0 is returned then the default alignment for the specified mode is used. 
 @end deftypefn
Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c	(revision 220025)
+++ gcc/tree-ssa-alias.c	(working copy)
@@ -1532,6 +1532,25 @@ refs_output_dependent_p (tree store1, tree store2)
   return refs_may_alias_p_1 (&r1, &r2, false);
 }
 
+/* Return true if the points-to solution *PT includes the object BASE.  */
+
+static bool
+pt_solution_includes_base (struct pt_solution *pt, tree base)
+{
+  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,6 +1561,22 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
   unsigned i;
   int flags = gimple_call_flags (call);
 
+  callee = gimple_call_fn (call);
+  if (callee && TREE_CODE (callee) == SSA_NAME
+      && targetm.has_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)
+	{
+	  base = ao_ref_base (ref);
+	  if (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)))
@@ -1564,7 +1599,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 +1838,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.  */
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 220025)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -1490,6 +1490,9 @@ static const struct attribute_spec rs6000_attribut
 #undef TARGET_ASM_CAN_OUTPUT_MI_THUNK
 #define TARGET_ASM_CAN_OUTPUT_MI_THUNK hook_bool_const_tree_hwi_hwi_const_tree_true
 
+#undef TARGET_HAS_FUNCTION_DESCRIPTORS
+#define TARGET_HAS_FUNCTION_DESCRIPTORS rs6000_has_function_descriptors
+
 #undef TARGET_FUNCTION_OK_FOR_SIBCALL
 #define TARGET_FUNCTION_OK_FOR_SIBCALL rs6000_function_ok_for_sibcall
 
@@ -22099,6 +22102,14 @@ rs6000_return_addr (int count, rtx frame)
   return get_hard_reg_initial_val (Pmode, LR_REGNO);
 }
 
+/* Return true if we use function descriptors.  */
+
+static bool
+rs6000_has_function_descriptors (void)
+{
+  return DEFAULT_ABI == ABI_AIX;
+}
+
 /* Say whether a function is a candidate for sibcall handling or not.  */
 
 static bool
Index: gcc/testsuite/gcc.target/powerpc/pr64703.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr64703.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr64703.c	(working copy)
@@ -0,0 +1,36 @@
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
+/* { dg-options "-O2 -mabi=elfv1" } */
+/* { dg-final { scan-assembler "std .\*,112\\(1\\)" } } */
+/* { dg-final { scan-assembler "std .\*,120\\(1\\)" } } */
+/* { dg-final { scan-assembler "std .\*,128\\(1\\)" } } */
+/* { dg-final { scan-assembler "addi .\*,1,112" } } */
+
+/* Testcase taken from glibc, powerpc64 dl-machine.h.  */
+
+typedef struct {
+  unsigned long fd_func;
+  unsigned long fd_toc;
+  unsigned long fd_aux;
+} Elf64_FuncDesc;
+
+extern unsigned long dl_hwcap;
+
+unsigned long
+resolve_ifunc (unsigned long value, unsigned long adjust)
+{
+  Elf64_FuncDesc opd;
+
+  if (adjust)
+    {
+      Elf64_FuncDesc *func = (Elf64_FuncDesc *) value;
+      opd.fd_func = func->fd_func + adjust;
+      opd.fd_toc = func->fd_toc + adjust;
+      opd.fd_aux = func->fd_aux;
+      value = (unsigned long) &opd;
+    }
+#if 0
+  __asm__ ("#%0" : : "r" (value));
+#endif
+  return ((unsigned long (*) (unsigned long)) value) (dl_hwcap);
+}

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 15:06     ` Alan Modra
@ 2015-01-29 16:30       ` Richard Biener
  2015-01-29 16:31         ` Richard Biener
  2015-01-29 20:35       ` Segher Boessenkool
  1 sibling, 1 reply; 18+ messages in thread
From: Richard Biener @ 2015-01-29 16:30 UTC (permalink / raw)
  To: Richard Biener, GCC Patches

On Thu, Jan 29, 2015 at 3:14 PM, Alan Modra <amodra@gmail.com> wrote:
> Here is the completed patch.  Bootstrapped and regression tested
> powerpc64-linux.  Is this OK to apply?  If not now, then when gcc is
> in stage1 again?

It's ok to apply as it is a wrong-code fix.  It would also be ok to backport
if needed.

Did you check whether other targets have function descriptors (seem
to remember the Itanic here at least)?

The middle-end changes are ok, I defer to David for the rs6000 changes.

I am also curious of the .029t.ealias dump from

gcc -O -fdump-tree-ealias-details-alias

on (the nonsensical)

void x(void);
int y;
int main()
{
  void *p = x;
  p+=y;
  return *(int *)p;
}

Thanks,
Richard.

> gcc/
>         PR target/64703
>         * target.def (has_function_descriptors): New hook.
>         * doc/tm.texi.in: Add TARGET_HAS_FUNCTION_DESCRIPTORS.
>         * doc/tc.texi: Regenerate.
>         * tree-ssa-alias.c (pt_solution_includes_base): New function,
>         extracted from..
>         (ref_maybe_used_by_call_p_1): ..here.  Handle potential memory
>         reference by indirect calls on targets using function descriptors.
>         * config/rs6000/rs6000.c (TARGET_HAS_FUNCTION_DESCRIPTORS): Define.
>         (rs6000_has_function_descriptors): New function.
> gcc/testsuite/
>         * gcc.target/powerpc/pr64703.c: New.
>
> Index: gcc/target.def
> ===================================================================
> --- gcc/target.def      (revision 220025)
> +++ gcc/target.def      (working copy)
> @@ -2821,6 +2821,15 @@ The default value of this hook is based on target'
>   bool, (void),
>   default_has_ifunc_p)
>
> +/* True if target defines the address of a function as that of a
> +   function descriptor.  */
> +DEFHOOK
> +(has_function_descriptors,
> + "True if target has function descriptors and defines the address\n\
> +of a function as that of a function descriptor.",
> + bool, (void),
> + hook_bool_void_false)
> +
>  /* True if it is OK to do sibling call optimization for the specified
>     call expression EXP.  DECL will be the called function, or NULL if
>     this is an indirect call.  */
> Index: gcc/doc/tm.texi.in
> ===================================================================
> --- gcc/doc/tm.texi.in  (revision 220025)
> +++ gcc/doc/tm.texi.in  (working copy)
> @@ -8175,6 +8175,8 @@ and the associated definitions of those functions.
>
>  @hook TARGET_HAS_IFUNC_P
>
> +@hook TARGET_HAS_FUNCTION_DESCRIPTORS
> +
>  @hook TARGET_ATOMIC_ALIGN_FOR_MODE
>
>  @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
> Index: gcc/doc/tm.texi
> ===================================================================
> --- gcc/doc/tm.texi     (revision 220025)
> +++ gcc/doc/tm.texi     (working copy)
> @@ -11510,6 +11510,11 @@ The support includes the assembler, linker and dyn
>  The default value of this hook is based on target's libc.
>  @end deftypefn
>
> +@deftypefn {Target Hook} bool TARGET_HAS_FUNCTION_DESCRIPTORS (void)
> +True if target has function descriptors and defines the address
> +of a function as that of a function descriptor.
> +@end deftypefn
> +
>  @deftypefn {Target Hook} {unsigned int} TARGET_ATOMIC_ALIGN_FOR_MODE (machine_mode @var{mode})
>  If defined, this function returns an appropriate alignment in bits for an atomic object of machine_mode @var{mode}.  If 0 is returned then the default alignment for the specified mode is used.
>  @end deftypefn
> Index: gcc/tree-ssa-alias.c
> ===================================================================
> --- gcc/tree-ssa-alias.c        (revision 220025)
> +++ gcc/tree-ssa-alias.c        (working copy)
> @@ -1532,6 +1532,25 @@ refs_output_dependent_p (tree store1, tree store2)
>    return refs_may_alias_p_1 (&r1, &r2, false);
>  }
>
> +/* Return true if the points-to solution *PT includes the object BASE.  */
> +
> +static bool
> +pt_solution_includes_base (struct pt_solution *pt, tree base)
> +{
> +  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,6 +1561,22 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
>    unsigned i;
>    int flags = gimple_call_flags (call);
>
> +  callee = gimple_call_fn (call);
> +  if (callee && TREE_CODE (callee) == SSA_NAME
> +      && targetm.has_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)
> +       {
> +         base = ao_ref_base (ref);
> +         if (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)))
> @@ -1564,7 +1599,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 +1838,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.  */
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 220025)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -1490,6 +1490,9 @@ static const struct attribute_spec rs6000_attribut
>  #undef TARGET_ASM_CAN_OUTPUT_MI_THUNK
>  #define TARGET_ASM_CAN_OUTPUT_MI_THUNK hook_bool_const_tree_hwi_hwi_const_tree_true
>
> +#undef TARGET_HAS_FUNCTION_DESCRIPTORS
> +#define TARGET_HAS_FUNCTION_DESCRIPTORS rs6000_has_function_descriptors
> +
>  #undef TARGET_FUNCTION_OK_FOR_SIBCALL
>  #define TARGET_FUNCTION_OK_FOR_SIBCALL rs6000_function_ok_for_sibcall
>
> @@ -22099,6 +22102,14 @@ rs6000_return_addr (int count, rtx frame)
>    return get_hard_reg_initial_val (Pmode, LR_REGNO);
>  }
>
> +/* Return true if we use function descriptors.  */
> +
> +static bool
> +rs6000_has_function_descriptors (void)
> +{
> +  return DEFAULT_ABI == ABI_AIX;
> +}
> +
>  /* Say whether a function is a candidate for sibcall handling or not.  */
>
>  static bool
> Index: gcc/testsuite/gcc.target/powerpc/pr64703.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/pr64703.c  (revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/pr64703.c  (working copy)
> @@ -0,0 +1,36 @@
> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
> +/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
> +/* { dg-options "-O2 -mabi=elfv1" } */
> +/* { dg-final { scan-assembler "std .\*,112\\(1\\)" } } */
> +/* { dg-final { scan-assembler "std .\*,120\\(1\\)" } } */
> +/* { dg-final { scan-assembler "std .\*,128\\(1\\)" } } */
> +/* { dg-final { scan-assembler "addi .\*,1,112" } } */
> +
> +/* Testcase taken from glibc, powerpc64 dl-machine.h.  */
> +
> +typedef struct {
> +  unsigned long fd_func;
> +  unsigned long fd_toc;
> +  unsigned long fd_aux;
> +} Elf64_FuncDesc;
> +
> +extern unsigned long dl_hwcap;
> +
> +unsigned long
> +resolve_ifunc (unsigned long value, unsigned long adjust)
> +{
> +  Elf64_FuncDesc opd;
> +
> +  if (adjust)
> +    {
> +      Elf64_FuncDesc *func = (Elf64_FuncDesc *) value;
> +      opd.fd_func = func->fd_func + adjust;
> +      opd.fd_toc = func->fd_toc + adjust;
> +      opd.fd_aux = func->fd_aux;
> +      value = (unsigned long) &opd;
> +    }
> +#if 0
> +  __asm__ ("#%0" : : "r" (value));
> +#endif
> +  return ((unsigned long (*) (unsigned long)) value) (dl_hwcap);
> +}
>
> --
> Alan Modra
> Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 16:30       ` Richard Biener
@ 2015-01-29 16:31         ` Richard Biener
  2015-01-29 16:32           ` Richard Biener
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Biener @ 2015-01-29 16:31 UTC (permalink / raw)
  To: Richard Biener, GCC Patches

On Thu, Jan 29, 2015 at 4:05 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jan 29, 2015 at 3:14 PM, Alan Modra <amodra@gmail.com> wrote:
>> Here is the completed patch.  Bootstrapped and regression tested
>> powerpc64-linux.  Is this OK to apply?  If not now, then when gcc is
>> in stage1 again?
>
> It's ok to apply as it is a wrong-code fix.  It would also be ok to backport
> if needed.
>
> Did you check whether other targets have function descriptors (seem
> to remember the Itanic here at least)?
>
> The middle-end changes are ok, I defer to David for the rs6000 changes.
>
> I am also curious of the .029t.ealias dump from
>
> gcc -O -fdump-tree-ealias-details-alias
>
> on (the nonsensical)
>
> void x(void);
> int y;
> int main()
> {
>   void *p = x;
>   p+=y;
>   return *(int *)p;
> }

Just checked myself with the host gcc 4.8 on gcc111.  It looks like
function descriptors are not exposed in GIMPLE:

  void * p;
  sizetype y.1;
  int y.0;
  int _6;

  <bb 2>:
  y.0_3 = y;
  y.1_4 = (sizetype) y.0_3;
  # PT =
  p_5 = x + y.1_4;
  _6 = MEM[(int *)p_5];
  return _6;

thus when the function address-taking happens in the same function
as the call there will be no aliasing as it points to nothing (points-to
doesn't track function decls).  And if it's flowing in from the outside
you get "all globals".

This means that you still will be able to create a testcase that is
miscompiled with exposing the address-taking to points-to analysis.
And it means that indirect calls to const functions are severely
pessimized (not that it matters?) as they effectively become pure calls.

Richard.

> Thanks,
> Richard.
>
>> gcc/
>>         PR target/64703
>>         * target.def (has_function_descriptors): New hook.
>>         * doc/tm.texi.in: Add TARGET_HAS_FUNCTION_DESCRIPTORS.
>>         * doc/tc.texi: Regenerate.
>>         * tree-ssa-alias.c (pt_solution_includes_base): New function,
>>         extracted from..
>>         (ref_maybe_used_by_call_p_1): ..here.  Handle potential memory
>>         reference by indirect calls on targets using function descriptors.
>>         * config/rs6000/rs6000.c (TARGET_HAS_FUNCTION_DESCRIPTORS): Define.
>>         (rs6000_has_function_descriptors): New function.
>> gcc/testsuite/
>>         * gcc.target/powerpc/pr64703.c: New.
>>
>> Index: gcc/target.def
>> ===================================================================
>> --- gcc/target.def      (revision 220025)
>> +++ gcc/target.def      (working copy)
>> @@ -2821,6 +2821,15 @@ The default value of this hook is based on target'
>>   bool, (void),
>>   default_has_ifunc_p)
>>
>> +/* True if target defines the address of a function as that of a
>> +   function descriptor.  */
>> +DEFHOOK
>> +(has_function_descriptors,
>> + "True if target has function descriptors and defines the address\n\
>> +of a function as that of a function descriptor.",
>> + bool, (void),
>> + hook_bool_void_false)
>> +
>>  /* True if it is OK to do sibling call optimization for the specified
>>     call expression EXP.  DECL will be the called function, or NULL if
>>     this is an indirect call.  */
>> Index: gcc/doc/tm.texi.in
>> ===================================================================
>> --- gcc/doc/tm.texi.in  (revision 220025)
>> +++ gcc/doc/tm.texi.in  (working copy)
>> @@ -8175,6 +8175,8 @@ and the associated definitions of those functions.
>>
>>  @hook TARGET_HAS_IFUNC_P
>>
>> +@hook TARGET_HAS_FUNCTION_DESCRIPTORS
>> +
>>  @hook TARGET_ATOMIC_ALIGN_FOR_MODE
>>
>>  @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
>> Index: gcc/doc/tm.texi
>> ===================================================================
>> --- gcc/doc/tm.texi     (revision 220025)
>> +++ gcc/doc/tm.texi     (working copy)
>> @@ -11510,6 +11510,11 @@ The support includes the assembler, linker and dyn
>>  The default value of this hook is based on target's libc.
>>  @end deftypefn
>>
>> +@deftypefn {Target Hook} bool TARGET_HAS_FUNCTION_DESCRIPTORS (void)
>> +True if target has function descriptors and defines the address
>> +of a function as that of a function descriptor.
>> +@end deftypefn
>> +
>>  @deftypefn {Target Hook} {unsigned int} TARGET_ATOMIC_ALIGN_FOR_MODE (machine_mode @var{mode})
>>  If defined, this function returns an appropriate alignment in bits for an atomic object of machine_mode @var{mode}.  If 0 is returned then the default alignment for the specified mode is used.
>>  @end deftypefn
>> Index: gcc/tree-ssa-alias.c
>> ===================================================================
>> --- gcc/tree-ssa-alias.c        (revision 220025)
>> +++ gcc/tree-ssa-alias.c        (working copy)
>> @@ -1532,6 +1532,25 @@ refs_output_dependent_p (tree store1, tree store2)
>>    return refs_may_alias_p_1 (&r1, &r2, false);
>>  }
>>
>> +/* Return true if the points-to solution *PT includes the object BASE.  */
>> +
>> +static bool
>> +pt_solution_includes_base (struct pt_solution *pt, tree base)
>> +{
>> +  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,6 +1561,22 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
>>    unsigned i;
>>    int flags = gimple_call_flags (call);
>>
>> +  callee = gimple_call_fn (call);
>> +  if (callee && TREE_CODE (callee) == SSA_NAME
>> +      && targetm.has_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)
>> +       {
>> +         base = ao_ref_base (ref);
>> +         if (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)))
>> @@ -1564,7 +1599,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 +1838,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.  */
>> Index: gcc/config/rs6000/rs6000.c
>> ===================================================================
>> --- gcc/config/rs6000/rs6000.c  (revision 220025)
>> +++ gcc/config/rs6000/rs6000.c  (working copy)
>> @@ -1490,6 +1490,9 @@ static const struct attribute_spec rs6000_attribut
>>  #undef TARGET_ASM_CAN_OUTPUT_MI_THUNK
>>  #define TARGET_ASM_CAN_OUTPUT_MI_THUNK hook_bool_const_tree_hwi_hwi_const_tree_true
>>
>> +#undef TARGET_HAS_FUNCTION_DESCRIPTORS
>> +#define TARGET_HAS_FUNCTION_DESCRIPTORS rs6000_has_function_descriptors
>> +
>>  #undef TARGET_FUNCTION_OK_FOR_SIBCALL
>>  #define TARGET_FUNCTION_OK_FOR_SIBCALL rs6000_function_ok_for_sibcall
>>
>> @@ -22099,6 +22102,14 @@ rs6000_return_addr (int count, rtx frame)
>>    return get_hard_reg_initial_val (Pmode, LR_REGNO);
>>  }
>>
>> +/* Return true if we use function descriptors.  */
>> +
>> +static bool
>> +rs6000_has_function_descriptors (void)
>> +{
>> +  return DEFAULT_ABI == ABI_AIX;
>> +}
>> +
>>  /* Say whether a function is a candidate for sibcall handling or not.  */
>>
>>  static bool
>> Index: gcc/testsuite/gcc.target/powerpc/pr64703.c
>> ===================================================================
>> --- gcc/testsuite/gcc.target/powerpc/pr64703.c  (revision 0)
>> +++ gcc/testsuite/gcc.target/powerpc/pr64703.c  (working copy)
>> @@ -0,0 +1,36 @@
>> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
>> +/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
>> +/* { dg-options "-O2 -mabi=elfv1" } */
>> +/* { dg-final { scan-assembler "std .\*,112\\(1\\)" } } */
>> +/* { dg-final { scan-assembler "std .\*,120\\(1\\)" } } */
>> +/* { dg-final { scan-assembler "std .\*,128\\(1\\)" } } */
>> +/* { dg-final { scan-assembler "addi .\*,1,112" } } */
>> +
>> +/* Testcase taken from glibc, powerpc64 dl-machine.h.  */
>> +
>> +typedef struct {
>> +  unsigned long fd_func;
>> +  unsigned long fd_toc;
>> +  unsigned long fd_aux;
>> +} Elf64_FuncDesc;
>> +
>> +extern unsigned long dl_hwcap;
>> +
>> +unsigned long
>> +resolve_ifunc (unsigned long value, unsigned long adjust)
>> +{
>> +  Elf64_FuncDesc opd;
>> +
>> +  if (adjust)
>> +    {
>> +      Elf64_FuncDesc *func = (Elf64_FuncDesc *) value;
>> +      opd.fd_func = func->fd_func + adjust;
>> +      opd.fd_toc = func->fd_toc + adjust;
>> +      opd.fd_aux = func->fd_aux;
>> +      value = (unsigned long) &opd;
>> +    }
>> +#if 0
>> +  __asm__ ("#%0" : : "r" (value));
>> +#endif
>> +  return ((unsigned long (*) (unsigned long)) value) (dl_hwcap);
>> +}
>>
>> --
>> Alan Modra
>> Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 16:31         ` Richard Biener
@ 2015-01-29 16:32           ` Richard Biener
  2015-01-29 17:48             ` Jakub Jelinek
  2015-01-30  6:11             ` Alan Modra
  0 siblings, 2 replies; 18+ messages in thread
From: Richard Biener @ 2015-01-29 16:32 UTC (permalink / raw)
  To: Richard Biener, GCC Patches

On Thu, Jan 29, 2015 at 4:12 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jan 29, 2015 at 4:05 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jan 29, 2015 at 3:14 PM, Alan Modra <amodra@gmail.com> wrote:
>>> Here is the completed patch.  Bootstrapped and regression tested
>>> powerpc64-linux.  Is this OK to apply?  If not now, then when gcc is
>>> in stage1 again?
>>
>> It's ok to apply as it is a wrong-code fix.  It would also be ok to backport
>> if needed.
>>
>> Did you check whether other targets have function descriptors (seem
>> to remember the Itanic here at least)?
>>
>> The middle-end changes are ok, I defer to David for the rs6000 changes.
>>
>> I am also curious of the .029t.ealias dump from
>>
>> gcc -O -fdump-tree-ealias-details-alias
>>
>> on (the nonsensical)
>>
>> void x(void);
>> int y;
>> int main()
>> {
>>   void *p = x;
>>   p+=y;
>>   return *(int *)p;
>> }
>
> Just checked myself with the host gcc 4.8 on gcc111.  It looks like
> function descriptors are not exposed in GIMPLE:
>
>   void * p;
>   sizetype y.1;
>   int y.0;
>   int _6;
>
>   <bb 2>:
>   y.0_3 = y;
>   y.1_4 = (sizetype) y.0_3;
>   # PT =
>   p_5 = x + y.1_4;
>   _6 = MEM[(int *)p_5];
>   return _6;
>
> thus when the function address-taking happens in the same function
> as the call there will be no aliasing as it points to nothing (points-to
> doesn't track function decls).  And if it's flowing in from the outside
> you get "all globals".
>
> This means that you still will be able to create a testcase that is
> miscompiled with exposing the address-taking to points-to analysis.
> And it means that indirect calls to const functions are severely
> pessimized (not that it matters?) as they effectively become pure calls.

Btw, with gimple_call_fntype available I was hoping to eventually get
rid of the required conversion between ptr and function ptr:

  /* We need to take special care recursing to pointed-to types.  */
  else if (POINTER_TYPE_P (inner_type)
           && POINTER_TYPE_P (outer_type))
    {
      /* Do not lose casts to function pointer types.  */
      if ((TREE_CODE (TREE_TYPE (outer_type)) == FUNCTION_TYPE
           || TREE_CODE (TREE_TYPE (outer_type)) == METHOD_TYPE)
          && !(TREE_CODE (TREE_TYPE (inner_type)) == FUNCTION_TYPE
               || TREE_CODE (TREE_TYPE (inner_type)) == METHOD_TYPE))
        return false;

      /* We do not care for const qualification of the pointed-to types
         as const qualification has no semantic value to the middle-end.  */

      /* Otherwise pointers/references are equivalent.  */
      return true;

which means call in the testcase in the PR would become

 (&opd) (dl_hwcap);

just as I mentioned in my initial review (you can try removing the above
check to see that).

I wonder if we have targets where a conversion between type T and
a function pointer generates actual code.

Richard.

> Richard.
>
>> Thanks,
>> Richard.
>>
>>> gcc/
>>>         PR target/64703
>>>         * target.def (has_function_descriptors): New hook.
>>>         * doc/tm.texi.in: Add TARGET_HAS_FUNCTION_DESCRIPTORS.
>>>         * doc/tc.texi: Regenerate.
>>>         * tree-ssa-alias.c (pt_solution_includes_base): New function,
>>>         extracted from..
>>>         (ref_maybe_used_by_call_p_1): ..here.  Handle potential memory
>>>         reference by indirect calls on targets using function descriptors.
>>>         * config/rs6000/rs6000.c (TARGET_HAS_FUNCTION_DESCRIPTORS): Define.
>>>         (rs6000_has_function_descriptors): New function.
>>> gcc/testsuite/
>>>         * gcc.target/powerpc/pr64703.c: New.
>>>
>>> Index: gcc/target.def
>>> ===================================================================
>>> --- gcc/target.def      (revision 220025)
>>> +++ gcc/target.def      (working copy)
>>> @@ -2821,6 +2821,15 @@ The default value of this hook is based on target'
>>>   bool, (void),
>>>   default_has_ifunc_p)
>>>
>>> +/* True if target defines the address of a function as that of a
>>> +   function descriptor.  */
>>> +DEFHOOK
>>> +(has_function_descriptors,
>>> + "True if target has function descriptors and defines the address\n\
>>> +of a function as that of a function descriptor.",
>>> + bool, (void),
>>> + hook_bool_void_false)
>>> +
>>>  /* True if it is OK to do sibling call optimization for the specified
>>>     call expression EXP.  DECL will be the called function, or NULL if
>>>     this is an indirect call.  */
>>> Index: gcc/doc/tm.texi.in
>>> ===================================================================
>>> --- gcc/doc/tm.texi.in  (revision 220025)
>>> +++ gcc/doc/tm.texi.in  (working copy)
>>> @@ -8175,6 +8175,8 @@ and the associated definitions of those functions.
>>>
>>>  @hook TARGET_HAS_IFUNC_P
>>>
>>> +@hook TARGET_HAS_FUNCTION_DESCRIPTORS
>>> +
>>>  @hook TARGET_ATOMIC_ALIGN_FOR_MODE
>>>
>>>  @hook TARGET_ATOMIC_ASSIGN_EXPAND_FENV
>>> Index: gcc/doc/tm.texi
>>> ===================================================================
>>> --- gcc/doc/tm.texi     (revision 220025)
>>> +++ gcc/doc/tm.texi     (working copy)
>>> @@ -11510,6 +11510,11 @@ The support includes the assembler, linker and dyn
>>>  The default value of this hook is based on target's libc.
>>>  @end deftypefn
>>>
>>> +@deftypefn {Target Hook} bool TARGET_HAS_FUNCTION_DESCRIPTORS (void)
>>> +True if target has function descriptors and defines the address
>>> +of a function as that of a function descriptor.
>>> +@end deftypefn
>>> +
>>>  @deftypefn {Target Hook} {unsigned int} TARGET_ATOMIC_ALIGN_FOR_MODE (machine_mode @var{mode})
>>>  If defined, this function returns an appropriate alignment in bits for an atomic object of machine_mode @var{mode}.  If 0 is returned then the default alignment for the specified mode is used.
>>>  @end deftypefn
>>> Index: gcc/tree-ssa-alias.c
>>> ===================================================================
>>> --- gcc/tree-ssa-alias.c        (revision 220025)
>>> +++ gcc/tree-ssa-alias.c        (working copy)
>>> @@ -1532,6 +1532,25 @@ refs_output_dependent_p (tree store1, tree store2)
>>>    return refs_may_alias_p_1 (&r1, &r2, false);
>>>  }
>>>
>>> +/* Return true if the points-to solution *PT includes the object BASE.  */
>>> +
>>> +static bool
>>> +pt_solution_includes_base (struct pt_solution *pt, tree base)
>>> +{
>>> +  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,6 +1561,22 @@ ref_maybe_used_by_call_p_1 (gcall *call, ao_ref *r
>>>    unsigned i;
>>>    int flags = gimple_call_flags (call);
>>>
>>> +  callee = gimple_call_fn (call);
>>> +  if (callee && TREE_CODE (callee) == SSA_NAME
>>> +      && targetm.has_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)
>>> +       {
>>> +         base = ao_ref_base (ref);
>>> +         if (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)))
>>> @@ -1564,7 +1599,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 +1838,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.  */
>>> Index: gcc/config/rs6000/rs6000.c
>>> ===================================================================
>>> --- gcc/config/rs6000/rs6000.c  (revision 220025)
>>> +++ gcc/config/rs6000/rs6000.c  (working copy)
>>> @@ -1490,6 +1490,9 @@ static const struct attribute_spec rs6000_attribut
>>>  #undef TARGET_ASM_CAN_OUTPUT_MI_THUNK
>>>  #define TARGET_ASM_CAN_OUTPUT_MI_THUNK hook_bool_const_tree_hwi_hwi_const_tree_true
>>>
>>> +#undef TARGET_HAS_FUNCTION_DESCRIPTORS
>>> +#define TARGET_HAS_FUNCTION_DESCRIPTORS rs6000_has_function_descriptors
>>> +
>>>  #undef TARGET_FUNCTION_OK_FOR_SIBCALL
>>>  #define TARGET_FUNCTION_OK_FOR_SIBCALL rs6000_function_ok_for_sibcall
>>>
>>> @@ -22099,6 +22102,14 @@ rs6000_return_addr (int count, rtx frame)
>>>    return get_hard_reg_initial_val (Pmode, LR_REGNO);
>>>  }
>>>
>>> +/* Return true if we use function descriptors.  */
>>> +
>>> +static bool
>>> +rs6000_has_function_descriptors (void)
>>> +{
>>> +  return DEFAULT_ABI == ABI_AIX;
>>> +}
>>> +
>>>  /* Say whether a function is a candidate for sibcall handling or not.  */
>>>
>>>  static bool
>>> Index: gcc/testsuite/gcc.target/powerpc/pr64703.c
>>> ===================================================================
>>> --- gcc/testsuite/gcc.target/powerpc/pr64703.c  (revision 0)
>>> +++ gcc/testsuite/gcc.target/powerpc/pr64703.c  (working copy)
>>> @@ -0,0 +1,36 @@
>>> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
>>> +/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
>>> +/* { dg-options "-O2 -mabi=elfv1" } */
>>> +/* { dg-final { scan-assembler "std .\*,112\\(1\\)" } } */
>>> +/* { dg-final { scan-assembler "std .\*,120\\(1\\)" } } */
>>> +/* { dg-final { scan-assembler "std .\*,128\\(1\\)" } } */
>>> +/* { dg-final { scan-assembler "addi .\*,1,112" } } */
>>> +
>>> +/* Testcase taken from glibc, powerpc64 dl-machine.h.  */
>>> +
>>> +typedef struct {
>>> +  unsigned long fd_func;
>>> +  unsigned long fd_toc;
>>> +  unsigned long fd_aux;
>>> +} Elf64_FuncDesc;
>>> +
>>> +extern unsigned long dl_hwcap;
>>> +
>>> +unsigned long
>>> +resolve_ifunc (unsigned long value, unsigned long adjust)
>>> +{
>>> +  Elf64_FuncDesc opd;
>>> +
>>> +  if (adjust)
>>> +    {
>>> +      Elf64_FuncDesc *func = (Elf64_FuncDesc *) value;
>>> +      opd.fd_func = func->fd_func + adjust;
>>> +      opd.fd_toc = func->fd_toc + adjust;
>>> +      opd.fd_aux = func->fd_aux;
>>> +      value = (unsigned long) &opd;
>>> +    }
>>> +#if 0
>>> +  __asm__ ("#%0" : : "r" (value));
>>> +#endif
>>> +  return ((unsigned long (*) (unsigned long)) value) (dl_hwcap);
>>> +}
>>>
>>> --
>>> Alan Modra
>>> Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 16:32           ` Richard Biener
@ 2015-01-29 17:48             ` Jakub Jelinek
  2015-01-30  6:00               ` Alan Modra
  2015-01-30  6:11             ` Alan Modra
  1 sibling, 1 reply; 18+ messages in thread
From: Jakub Jelinek @ 2015-01-29 17:48 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches

On Thu, Jan 29, 2015 at 04:21:15PM +0100, Richard Biener wrote:
> >>>         PR target/64703
> >>>         * target.def (has_function_descriptors): New hook.
> >>>         * doc/tm.texi.in: Add TARGET_HAS_FUNCTION_DESCRIPTORS.
> >>>         * doc/tc.texi: Regenerate.
> >>>         * tree-ssa-alias.c (pt_solution_includes_base): New function,
> >>>         extracted from..
> >>>         (ref_maybe_used_by_call_p_1): ..here.  Handle potential memory
> >>>         reference by indirect calls on targets using function descriptors.
> >>>         * config/rs6000/rs6000.c (TARGET_HAS_FUNCTION_DESCRIPTORS): Define.
> >>>         (rs6000_has_function_descriptors): New function.
> >>> gcc/testsuite/
> >>>         * gcc.target/powerpc/pr64703.c: New.

Won't the patch pessimize say const method calls through vtable?
Though, say on:
struct A { virtual __attribute__((const)) int foo (int) const; };
int a, b;
A c;

int
foo (A *p)
{
  a = 1;
  b = 2;
  int c = p->foo (3);
  a = 3;
  b = 4;
  return c;
}
we don't DSE the first two a and b stores anyway.

	Jakub

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 15:06     ` Alan Modra
  2015-01-29 16:30       ` Richard Biener
@ 2015-01-29 20:35       ` Segher Boessenkool
  2015-01-29 20:36         ` Jakub Jelinek
  1 sibling, 1 reply; 18+ messages in thread
From: Segher Boessenkool @ 2015-01-29 20:35 UTC (permalink / raw)
  To: Richard Biener, GCC Patches

On Fri, Jan 30, 2015 at 12:44:37AM +1030, Alan Modra wrote:
> Index: gcc/testsuite/gcc.target/powerpc/pr64703.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/pr64703.c	(revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/pr64703.c	(working copy)
> @@ -0,0 +1,36 @@
> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
> +/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
> +/* { dg-options "-O2 -mabi=elfv1" } */
> +/* { dg-final { scan-assembler "std .\*,112\\(1\\)" } } */
> +/* { dg-final { scan-assembler "std .\*,120\\(1\\)" } } */
> +/* { dg-final { scan-assembler "std .\*,128\\(1\\)" } } */
> +/* { dg-final { scan-assembler "addi .\*,1,112" } } */

. matches newline; use  [^,]*,  instead?


Segher

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 20:35       ` Segher Boessenkool
@ 2015-01-29 20:36         ` Jakub Jelinek
  2015-01-29 20:44           ` Segher Boessenkool
  0 siblings, 1 reply; 18+ messages in thread
From: Jakub Jelinek @ 2015-01-29 20:36 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Richard Biener, GCC Patches

On Thu, Jan 29, 2015 at 01:55:22PM -0600, Segher Boessenkool wrote:
> On Fri, Jan 30, 2015 at 12:44:37AM +1030, Alan Modra wrote:
> > Index: gcc/testsuite/gcc.target/powerpc/pr64703.c
> > ===================================================================
> > --- gcc/testsuite/gcc.target/powerpc/pr64703.c	(revision 0)
> > +++ gcc/testsuite/gcc.target/powerpc/pr64703.c	(working copy)
> > @@ -0,0 +1,36 @@
> > +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
> > +/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
> > +/* { dg-options "-O2 -mabi=elfv1" } */
> > +/* { dg-final { scan-assembler "std .\*,112\\(1\\)" } } */
> > +/* { dg-final { scan-assembler "std .\*,120\\(1\\)" } } */
> > +/* { dg-final { scan-assembler "std .\*,128\\(1\\)" } } */
> > +/* { dg-final { scan-assembler "addi .\*,1,112" } } */
> 
> . matches newline; use  [^,]*,  instead?

\[^,\]* or \[^,]* , otherwise dejagnu/tcl eats it.

	Jakub

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 20:36         ` Jakub Jelinek
@ 2015-01-29 20:44           ` Segher Boessenkool
  0 siblings, 0 replies; 18+ messages in thread
From: Segher Boessenkool @ 2015-01-29 20:44 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Richard Biener, GCC Patches

On Thu, Jan 29, 2015 at 09:01:31PM +0100, Jakub Jelinek wrote:
> > . matches newline; use  [^,]*,  instead?
> 
> \[^,\]* or \[^,]* , otherwise dejagnu/tcl eats it.

Right; I did mention to use {} instead of "" in a previous mail :-)


Segher

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 17:48             ` Jakub Jelinek
@ 2015-01-30  6:00               ` Alan Modra
  0 siblings, 0 replies; 18+ messages in thread
From: Alan Modra @ 2015-01-30  6:00 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Richard Biener, GCC Patches

On Thu, Jan 29, 2015 at 04:51:47PM +0100, Jakub Jelinek wrote:
> Won't the patch pessimize say const method calls through vtable?

I was worried about accidental pessimization too, so ran a full gcc
build with the patch and compared against one with s/ABI_AIX/ABI_NONE/
in rs6000_has_function_descriptors.

$ for z in `find . -name testsuite -prune -o -name \*.o -print`; do cmp -s $z ~/build/gcc64-current.save/$z || echo $z >> files; done

That showed up a whole lot of libjava differences which on inspection
proved to be .debug_str ordering.  Ick.  OK, let's look at just code
differences.  (BTW, testsuite pruned because "make check" was still
running.)

$ for z in `cat files`; do objdump -d $z | tail -n +5 > dump; objdump -d ~/build/gcc64-current.save/$z | tail -n +5 > dump2; cmp -s dump dump2 || echo $z >> files2; done
$ cat files2
./gcc/rs6000.o
./prev-gcc/rs6000.o
./stage1-gcc/rs6000.o

So the patch didn't make a real mess..

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-29 16:32           ` Richard Biener
  2015-01-29 17:48             ` Jakub Jelinek
@ 2015-01-30  6:11             ` Alan Modra
  2015-01-30  8:52               ` Alan Modra
  1 sibling, 1 reply; 18+ messages in thread
From: Alan Modra @ 2015-01-30  6:11 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches

On Thu, Jan 29, 2015 at 04:21:15PM +0100, Richard Biener wrote:
> On Thu, Jan 29, 2015 at 4:12 PM, Richard Biener
> >> Did you check whether other targets have function descriptors (seem
> >> to remember the Itanic here at least)?

Yes, ia64 and hppa64 use function descriptors.  I don't know of any
others.

> > thus when the function address-taking happens in the same function
> > as the call there will be no aliasing as it points to nothing (points-to
> > doesn't track function decls).  And if it's flowing in from the outside
> > you get "all globals".
> >
> > This means that you still will be able to create a testcase that is
> > miscompiled with exposing the address-taking to points-to analysis.

I'm sorry, I don't see how to.  (I'm not disagreeing, just ignorant.)
One thing to consider is that a function address is potentially
pointing at read-only memory.  eg. -z relro on current ld.bfd makes
powerpc64 .opd read-only.  So people can't twiddle a "real" function
descriptor.  The only case that matter for ppc64 are where people
build their own function descriptor, as is done in glibc.

> > And it means that indirect calls to const functions are severely
> > pessimized (not that it matters?) as they effectively become pure calls.

That might be a problem but see my other email comparing gcc object
files.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-30  6:11             ` Alan Modra
@ 2015-01-30  8:52               ` Alan Modra
  2015-01-30 10:23                 ` Richard Biener
  0 siblings, 1 reply; 18+ messages in thread
From: Alan Modra @ 2015-01-30  8:52 UTC (permalink / raw)
  To: Richard Biener, GCC Patches

On Fri, Jan 30, 2015 at 01:19:51PM +1030, Alan Modra wrote:
> On Thu, Jan 29, 2015 at 04:21:15PM +0100, Richard Biener wrote:
> > > This means that you still will be able to create a testcase that is
> > > miscompiled with exposing the address-taking to points-to analysis.
> 
> I'm sorry, I don't see how to.  (I'm not disagreeing, just ignorant.)

Sigh.  Yes, I can make such a testcase.

typedef struct {
  unsigned long fd_func;
  unsigned long fd_toc;
  unsigned long fd_aux;
} Elf64_FuncDesc;

int a, b;

int
foo (__attribute__((const)) int (*f) (int), long adjust)
{
  Elf64_FuncDesc opd;

  a = 1;
  b = 2;
  if (adjust)
    {
      opd = *(Elf64_FuncDesc *) f;
      opd.fd_func += adjust;
      f = (int (*) (int)) &opd;
    }
  int c = f (3);
  a = 3;
  b = 4;
  return c;
}

This time we lose in .032t.cddce1.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-30  8:52               ` Alan Modra
@ 2015-01-30 10:23                 ` Richard Biener
  2015-01-30 11:00                   ` Jakub Jelinek
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Biener @ 2015-01-30 10:23 UTC (permalink / raw)
  To: Richard Biener, GCC Patches

On Fri, Jan 30, 2015 at 6:12 AM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Jan 30, 2015 at 01:19:51PM +1030, Alan Modra wrote:
>> On Thu, Jan 29, 2015 at 04:21:15PM +0100, Richard Biener wrote:
>> > > This means that you still will be able to create a testcase that is
>> > > miscompiled with exposing the address-taking to points-to analysis.
>>
>> I'm sorry, I don't see how to.  (I'm not disagreeing, just ignorant.)
>
> Sigh.  Yes, I can make such a testcase.
>
> typedef struct {
>   unsigned long fd_func;
>   unsigned long fd_toc;
>   unsigned long fd_aux;
> } Elf64_FuncDesc;
>
> int a, b;
>
> int
> foo (__attribute__((const)) int (*f) (int), long adjust)
> {
>   Elf64_FuncDesc opd;
>
>   a = 1;
>   b = 2;
>   if (adjust)
>     {
>       opd = *(Elf64_FuncDesc *) f;
>       opd.fd_func += adjust;
>       f = (int (*) (int)) &opd;
>     }
>   int c = f (3);
>   a = 3;
>   b = 4;
>   return c;
> }
>
> This time we lose in .032t.cddce1.

Ok - without digging into why the above would fail with your patch
(don't see that - the use in the function call can't be &opdd) - let's
take a step back and decide whether we want to allow user-created
function descriptors.  And if we do that if we should rather expose
this in a more sensible way to GCC, like with using a (target) builtin.
Say, force you to do

  int (*f) (int) = __builtin_fdesc (opd.fd_func, opd.fd_toc, opd.fd_aux);
  return f (3);

which would allow GCC to even optimize the call to a direct one
if it (or the target) can fold reads from the __builtin_fdesc argument
to a function decl.  Similar builtins could allow you to inspect
a function descriptor.  That way the actual memory operations
would be hidden from the middle-end.

For glibc you have to workaround the GCC "bug" anyway and I
think people building function descriptors themselves is a very
rare thing...  (I don't understand why a function descriptor is not
readily availabe here anyway...)

Thanks,
Richard.

> --
> Alan Modra
> Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-30 10:23                 ` Richard Biener
@ 2015-01-30 11:00                   ` Jakub Jelinek
  2015-02-02  9:29                     ` Alan Modra
  0 siblings, 1 reply; 18+ messages in thread
From: Jakub Jelinek @ 2015-01-30 11:00 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches

On Fri, Jan 30, 2015 at 10:12:35AM +0100, Richard Biener wrote:
> Ok - without digging into why the above would fail with your patch
> (don't see that - the use in the function call can't be &opdd) - let's
> take a step back and decide whether we want to allow user-created
> function descriptors.  And if we do that if we should rather expose
> this in a more sensible way to GCC, like with using a (target) builtin.
> Say, force you to do
> 
>   int (*f) (int) = __builtin_fdesc (opd.fd_func, opd.fd_toc, opd.fd_aux);
>   return f (3);
> 
> which would allow GCC to even optimize the call to a direct one
> if it (or the target) can fold reads from the __builtin_fdesc argument
> to a function decl.  Similar builtins could allow you to inspect
> a function descriptor.  That way the actual memory operations
> would be hidden from the middle-end.

That would be my preference too.  Constructing the calls this way is so rare
that pessimizing 99.9% of code out there that doesn't ever need this is
IMHO undesirable.

	Jakub

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
  2015-01-30 11:00                   ` Jakub Jelinek
@ 2015-02-02  9:29                     ` Alan Modra
  0 siblings, 0 replies; 18+ messages in thread
From: Alan Modra @ 2015-02-02  9:29 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Richard Biener, GCC Patches

On Fri, Jan 30, 2015 at 10:33:06AM +0100, Jakub Jelinek wrote:
> On Fri, Jan 30, 2015 at 10:12:35AM +0100, Richard Biener wrote:
> > Ok - without digging into why the above would fail with your patch
> > (don't see that - the use in the function call can't be &opdd) - let's
> > take a step back and decide whether we want to allow user-created
> > function descriptors.  And if we do that if we should rather expose
> > this in a more sensible way to GCC, like with using a (target) builtin.
> > Say, force you to do
> > 
> >   int (*f) (int) = __builtin_fdesc (opd.fd_func, opd.fd_toc, opd.fd_aux);
> >   return f (3);
> > 
> > which would allow GCC to even optimize the call to a direct one
> > if it (or the target) can fold reads from the __builtin_fdesc argument
> > to a function decl.  Similar builtins could allow you to inspect
> > a function descriptor.  That way the actual memory operations
> > would be hidden from the middle-end.
> 
> That would be my preference too.  Constructing the calls this way is so rare
> that pessimizing 99.9% of code out there that doesn't ever need this is
> IMHO undesirable.

I had a look at what it would take to fix the const function testcase,
and rapidly came to the conclusion that it is not something I should
attempt.  I'd need at least a week, probably more, to be confident of
making a proper fix.  Right now, I see FUD in comments and read Fear
Uncertainty and Doubt, rather than Function Use-Def chains.  ;-)
Besides, I hear your comments about pessimizing code.

The exercise also made me view the patch I submitted as a half-baked
hack.  :-(  Well, maybe not quite that bad, but enough to have
misgivings about applying the patch.  I suspect that future changes to
tree optimization passes will break glibc again (at least, versions
of glibc that don't have the asm fix), so it's probably better to not
apply an incomplete fix.  We're in a really grey area of the C
standard when it comes to the glibc code.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile
@ 2015-01-29 16:35 David Edelsohn
  0 siblings, 0 replies; 18+ messages in thread
From: David Edelsohn @ 2015-01-29 16:35 UTC (permalink / raw)
  To: Alan Modra; +Cc: GCC Patches, Richard Biener

gcc/
PR target/64703
* target.def (has_function_descriptors): New hook.
* doc/tm.texi.in: Add TARGET_HAS_FUNCTION_DESCRIPTORS.
* doc/tc.texi: Regenerate.
* tree-ssa-alias.c (pt_solution_includes_base): New function,
extracted from..
(ref_maybe_used_by_call_p_1): ..here.  Handle potential memory
reference by indirect calls on targets using function descriptors.
* config/rs6000/rs6000.c (TARGET_HAS_FUNCTION_DESCRIPTORS): Define.
(rs6000_has_function_descriptors): New function.
gcc/testsuite/
* gcc.target/powerpc/pr64703.c: New.

The rs6000 parts of the patch are okay.

Thanks for fixing this,
David

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2015-02-02  9:29 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-24  2:37 [RFC] PR64703, glibc sysdeps/powerpc/powerpc64/dl-machine.h miscompile Alan Modra
2015-01-26  9:56 ` Richard Biener
2015-01-27 14:27   ` Alan Modra
2015-01-29 15:06     ` Alan Modra
2015-01-29 16:30       ` Richard Biener
2015-01-29 16:31         ` Richard Biener
2015-01-29 16:32           ` Richard Biener
2015-01-29 17:48             ` Jakub Jelinek
2015-01-30  6:00               ` Alan Modra
2015-01-30  6:11             ` Alan Modra
2015-01-30  8:52               ` Alan Modra
2015-01-30 10:23                 ` Richard Biener
2015-01-30 11:00                   ` Jakub Jelinek
2015-02-02  9:29                     ` Alan Modra
2015-01-29 20:35       ` Segher Boessenkool
2015-01-29 20:36         ` Jakub Jelinek
2015-01-29 20:44           ` Segher Boessenkool
2015-01-29 16:35 David Edelsohn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).