[PATCH] Fix PR42944

[PATCH] Fix PR42944

[Richard Guenther]

We're a bit overeager in assuming that malloc/calloc do not clobber
memory.  Because glibc appearantly chooses to set errno.

Fixed as follows, bootstrapped and tested on x86_64-unknown-linux-gnu.

The testcase will show whether there are any systems that have
a plain errno declaration.

Committed to trunk.

Richard.

2010-02-03  Richard Guenther  <rguenther@suse.de>

        PR tree-optimization/42944
        * tree-ssa-alias.c (ref_maybe_used_by_call_p_1): Handle
        calloc.
        (call_may_clobber_ref_p_1): Likewise.  Properly handle
        malloc and calloc clobbering errno.

        * gcc.dg/errno-1.c: New testcase.



Index: gcc/tree-ssa-alias.c
===================================================================
*** gcc/tree-ssa-alias.c	(revision 156463)
--- gcc/tree-ssa-alias.c	(working copy)
*************** ref_maybe_used_by_call_p_1 (gimple call,
*** 963,968 ****
--- 963,969 ----
  	/* The following builtins do not read from memory.  */
  	case BUILT_IN_FREE:
  	case BUILT_IN_MALLOC:
+ 	case BUILT_IN_CALLOC:
  	case BUILT_IN_MEMSET:
  	case BUILT_IN_FREXP:
  	case BUILT_IN_FREXPF:
*************** call_may_clobber_ref_p_1 (gimple call, a
*** 1190,1195 ****
--- 1191,1211 ----
  	/* Allocating memory does not have any side-effects apart from
  	   being the definition point for the pointer.  */
  	case BUILT_IN_MALLOC:
+ 	case BUILT_IN_CALLOC:
+ 	  /* Unix98 specifies that errno is set on allocation failure.
+ 	     Until we properly can track the errno location assume it
+ 	     is not a plain decl but anonymous storage in a different
+ 	     translation unit.  */
+ 	  if (flag_errno_math)
+ 	    {
+ 	      struct ptr_info_def *pi;
+ 	      if (DECL_P (base))
+ 		return false;
+ 	      if (INDIRECT_REF_P (base)
+ 		  && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME
+ 		  && (pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0))))
+ 		return pi->pt.anything || pi->pt.nonlocal;
+ 	    }
  	  return false;
  	/* Freeing memory kills the pointed-to memory.  More importantly
  	   the call has to serve as a barrier for moving loads and stores
Index: gcc/testsuite/gcc.dg/errno-1.c
===================================================================
*** gcc/testsuite/gcc.dg/errno-1.c	(revision 0)
--- gcc/testsuite/gcc.dg/errno-1.c	(revision 0)
***************
*** 0 ****
--- 1,17 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O2" } */
+ 
+ #include <errno.h>
+ #include <stdlib.h>
+ 
+ int main()
+ {
+   void *p;
+   errno = 0;
+   p = malloc (-1);
+   if (errno != 0)
+     do_not_optimize_away ();
+   return 0;
+ }
+ 
+ /* { dg-final { scan-assembler "do_not_optimize_away" } } */

Re: [PATCH] Fix PR42944

[Steve Ellcey]

> We're a bit overeager in assuming that malloc/calloc do not clobber
> memory.  Because glibc appearantly chooses to set errno.
> 
> Fixed as follows, bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> The testcase will show whether there are any systems that have
> a plain errno declaration.

And then what?  The test case is failing for me on my HP-UX systems.

Steve Ellcey
sje@cup.hp.com

Re: [PATCH] Fix PR42944

[Richard Guenther]

On Tue, 16 Feb 2010, Steve Ellcey wrote:

> > We're a bit overeager in assuming that malloc/calloc do not clobber
> > memory.  Because glibc appearantly chooses to set errno.
> > 
> > Fixed as follows, bootstrapped and tested on x86_64-unknown-linux-gnu.
> > 
> > The testcase will show whether there are any systems that have
> > a plain errno declaration.
> 
> And then what?  The test case is failing for me on my HP-UX systems.

Then we'll massage the test until everyone is happy.  Does the
following make it work for you?

Thanks,
Richard.

2010-02-17  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/42944
	* tree-ssa-alias.c (call_may_clobber_ref_p_1): Massage
	test for aliasing with errno.

Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c	(revision 156824)
+++ gcc/tree-ssa-alias.c	(working copy)
@@ -1260,14 +1260,16 @@ call_may_clobber_ref_p_1 (gimple call, a
 	     Until we properly can track the errno location assume it
 	     is not a plain decl but anonymous storage in a different
 	     translation unit.  */
-	  if (flag_errno_math)
+	  if (flag_errno_math
+	      && TREE_TYPE (base) == integer_type_node)
 	    {
 	      struct ptr_info_def *pi;
-	      if (DECL_P (base))
-		return false;
-	      if (INDIRECT_REF_P (base)
-		  && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME
-		  && (pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0))))
+	      if (DECL_P (base)
+		  && !TREE_STATIC (base))
+		return true;
+	      else if (INDIRECT_REF_P (base)
+		       && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME
+		       && (pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0))))
 		return pi->pt.anything || pi->pt.nonlocal;
 	    }
 	  return false;

Re: [PATCH] Fix PR42944

[Steve Ellcey]

On Wed, 2010-02-17 at 10:57 +0100, Richard Guenther wrote:
> On Tue, 16 Feb 2010, Steve Ellcey wrote:
> > 
> > And then what?  The test case is failing for me on my HP-UX systems.
> 
> Then we'll massage the test until everyone is happy.  Does the
> following make it work for you?
> 
> Thanks,
> Richard.

Yes, that patch fixed the problem.  I didn't do a complete bootstrap &
test, I will do that overnight and let you know the results but it did
fix gcc.dg/errno-1.c.

Steve Ellcey
sje@cup.hp.com

Re: [PATCH] Fix PR42944

[Steve Ellcey]

> Yes, that patch fixed the problem.  I didn't do a complete bootstrap &
> test, I will do that overnight and let you know the results but it did
> fix gcc.dg/errno-1.c.
> 
> Steve Ellcey
> sje@cup.hp.com

I finished my nightly testing and everything looked good.  No regressions
and gcc.dg/errno-1.c passed on all my HP-UX boxes.

Steve Ellcey
sje@cup.hp.com

Re: [PATCH] Fix PR42944

[Richard Guenther]

On Thu, 18 Feb 2010, Steve Ellcey wrote:

> > Yes, that patch fixed the problem.  I didn't do a complete bootstrap &
> > test, I will do that overnight and let you know the results but it did
> > fix gcc.dg/errno-1.c.
> > 
> > Steve Ellcey
> > sje@cup.hp.com
> 
> I finished my nightly testing and everything looked good.  No regressions
> and gcc.dg/errno-1.c passed on all my HP-UX boxes.

Committed as rev. 156890.

Richard.

Re: [PATCH] Fix PR42944

[Ulrich Weigand]

Richard Guenther wrote:

> 2010-02-03  Richard Guenther  <rguenther@suse.de>
> 
>         PR tree-optimization/42944
>         * tree-ssa-alias.c (ref_maybe_used_by_call_p_1): Handle
>         calloc.
>         (call_may_clobber_ref_p_1): Likewise.  Properly handle
>         malloc and calloc clobbering errno.
> 
>         * gcc.dg/errno-1.c: New testcase.

This test fails on SPU, presumably because we're using newlib.
Specifically, errno is a macro that expands to something like:
   _impure_data._errno

It seems the code in call_may_clobber_ref_p_1 may not expect
a component reference here?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix PR42944

[Richard Guenther]

On Tue, Jan 4, 2011 at 6:33 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Richard Guenther wrote:
>
>> 2010-02-03  Richard Guenther  <rguenther@suse.de>
>>
>>         PR tree-optimization/42944
>>         * tree-ssa-alias.c (ref_maybe_used_by_call_p_1): Handle
>>         calloc.
>>         (call_may_clobber_ref_p_1): Likewise.  Properly handle
>>         malloc and calloc clobbering errno.
>>
>>         * gcc.dg/errno-1.c: New testcase.
>
> This test fails on SPU, presumably because we're using newlib.
> Specifically, errno is a macro that expands to something like:
>   _impure_data._errno
>
> It seems the code in call_may_clobber_ref_p_1 may not expect
> a component reference here?

Yes, we assume that errno is an integer object or accessed
via a pointer.  Basically we try to be able to disambiguate
at least _something_ across malloc/calloc, which is very
important for SPEC 2006 FP performance.  Something like
_impure_data._errno is a very bad choice for errno.

We do unfortunately have no way to conservatively track what
points to errno, so this guard is very important here.

Iff we want to fix this cornercase for 4.6 (for 4.7 somebody
may sit down and really think through how to generally keep
track of errno) then I suppose a target hook might be the
way to go.  For newlib we could say errno is always accessed
via a aggregate decl.  So the target hook would get the
'ref' variable passed and thus can access 'base' from

          if (flag_errno_math
              && TREE_TYPE (base) == integer_type_node)
            {
              struct ptr_info_def *pi;
              if (DECL_P (base)
                  && !TREE_STATIC (base))
                return true;
              else if ((INDIRECT_REF_P (base)
                        || TREE_CODE (base) == MEM_REF)
                       && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME
                       && (pi = SSA_NAME_PTR_INFO (TREE_OPERAND (base, 0))))
                return pi->pt.anything || pi->pt.nonlocal;
            }

and return true if errno might be accessed through it, with the above code
being the default implementation.  The hook could be called
ref_may_alias_errno_p () and we'd wrap that in a helper with the same
name in tree-ssa-alias.c.

I can cook up a patch sketch next week (can you open a bug?) but would
appreciate testing from your side (plus maybe the enable-for-all-newlib
targets stuff).

Thanks,
Richard.


> Bye,
> Ulrich
>
> --
>  Dr. Ulrich Weigand
>  GNU Toolchain for Linux on System z and Cell BE
>  Ulrich.Weigand@de.ibm.com
>

Re: [PATCH] Fix PR42944

[Ulrich Weigand]

Richard Guenther wrote:
> On Tue, Jan 4, 2011 at 6:33 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > This test fails on SPU, presumably because we're using newlib.
> > Specifically, errno is a macro that expands to something like:
> >   _impure_data._errno
> >
> > It seems the code in call_may_clobber_ref_p_1 may not expect
> > a component reference here?
> 
> Yes, we assume that errno is an integer object or accessed
> via a pointer.  Basically we try to be able to disambiguate
> at least _something_ across malloc/calloc, which is very
> important for SPEC 2006 FP performance.  Something like
> _impure_data._errno is a very bad choice for errno.

I see.  Unfortunately it doesn't seem feasible to change this
for the SPU newlib at this point as it's part of the ABI now ...

> I can cook up a patch sketch next week

Thanks for your help with this!

> (can you open a bug?)

Of course, this is now PR 47179.

> but would appreciate testing from your side

Certainly!

> (plus maybe the enable-for-all-newlib targets stuff).

Actually, it looks like this is SPU specific; on other platforms, newlib
does something like:

  #define errno (*__errno())
  extern int *__errno _PARAMS ((void));

The SPU code is an optimization to exploit the fact that we're always
single-threaded.  (It still doesn't use just a global because all the
rest of the newlib code assumes the errno value is part of a _reent
structure, even for single-threaded systems.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com