Alias sets for VLAs and strict aliasing warnings (PR 41673)

Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Joseph S. Myers]

This patch fixes PR 41673, where a VLA being given alias set 0 caused
a bogus -Wstrict-aliasing warning from dereference of that array
converted to a pointer (which involves a conversion from
pointer-to-array to pointer-to-element that looks like a cast for the
purposes of these warnings).

I believe it is appropriate for both warnings and optimization for
VLAs to get the alias sets of their elements (rather than special
casing VLAs in the warning code, for example), so this patch makes it
possible for langhooks to override the default of alias set 0 for
types with structural comparison.

Bootstrapped with no regressions on x86_64-unknown-linux-gnu.  OK to
commit (the alias.c changes)?

2009-10-22  Joseph Myers  <joseph@codesourcery.com>

	PR c/41673
	* alias.c (get_alias_set): Call langhook before returning 0 for
	types with structural equality.
	* c-common.c (c_common_get_alias_set): Use alias set of element
	type for arrays with structural comparison.

testsuite:
2009-10-22  Joseph Myers  <joseph@codesourcery.com>

	PR c/41673
	* gcc.dg/Wstrict-aliasing-bogus-vla-1.c: New test.

Index: testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c
===================================================================
--- testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c	(revision 0)
+++ testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c	(revision 0)
@@ -0,0 +1,10 @@
+/* PR 41673: bogus -Wstrict-aliasing warning from VLA dereference.  */
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99 -O2 -Wall" } */
+
+int main(int argc, char *argv[])
+{
+    float x[argc];
+    float y[argc];
+    return 0 == __builtin_memcpy(y, x, argc * sizeof(*x));
+}
Index: alias.c
===================================================================
--- alias.c	(revision 153482)
+++ alias.c	(working copy)
@@ -691,7 +691,14 @@ get_alias_set (tree t)
      requires structural comparisons to identify compatible types
      use alias set zero.  */
   if (TYPE_STRUCTURAL_EQUALITY_P (t))
-    return 0;
+    {
+      /* Allow the language to specify another alias set for this
+	 type.  */
+      set = lang_hooks.get_alias_set (t);
+      if (set != -1)
+	return set;
+      return 0;
+    }
   t = TYPE_CANONICAL (t);
   /* Canonical types shouldn't form a tree nor should the canonical
      type require structural equality checks.  */
Index: c-common.c
===================================================================
--- c-common.c	(revision 153482)
+++ c-common.c	(working copy)
@@ -4183,6 +4183,15 @@ c_common_get_alias_set (tree t)
   tree u;
   PTR *slot;
 
+  /* For VLAs, use the alias set of the element type rather than the
+     default of alias set 0 for types compared structurally.  */
+  if (TYPE_P (t) && TYPE_STRUCTURAL_EQUALITY_P (t))
+    {
+      if (TREE_CODE (t) == ARRAY_TYPE)
+	return get_alias_set (TREE_TYPE (t));
+      return -1;
+    }
+
   /* Permit type-punning when accessing a union, provided the access
      is directly through the union.  For example, this code does not
      permit taking the address of a union member and then storing

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Richard Guenther]

On Fri, Oct 23, 2009 at 4:27 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> This patch fixes PR 41673, where a VLA being given alias set 0 caused
> a bogus -Wstrict-aliasing warning from dereference of that array
> converted to a pointer (which involves a conversion from
> pointer-to-array to pointer-to-element that looks like a cast for the
> purposes of these warnings).
>
> I believe it is appropriate for both warnings and optimization for
> VLAs to get the alias sets of their elements (rather than special
> casing VLAs in the warning code, for example), so this patch makes it
> possible for langhooks to override the default of alias set 0 for
> types with structural comparison.
>
> Bootstrapped with no regressions on x86_64-unknown-linux-gnu.  OK to
> commit (the alias.c changes)?

The change is ok, but I wonder why the C FE sets
TYPE_STRUCTURAL_EQUALITY_P on VLAs.  As you can see
from alias.c the middle-end always uses the alias-set of the
element type for arrays (well, unless they are marked as not
aliased).

Thanks,
Richard.

> 2009-10-22  Joseph Myers  <joseph@codesourcery.com>
>
>        PR c/41673
>        * alias.c (get_alias_set): Call langhook before returning 0 for
>        types with structural equality.
>        * c-common.c (c_common_get_alias_set): Use alias set of element
>        type for arrays with structural comparison.
>
> testsuite:
> 2009-10-22  Joseph Myers  <joseph@codesourcery.com>
>
>        PR c/41673
>        * gcc.dg/Wstrict-aliasing-bogus-vla-1.c: New test.
>
> Index: testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c
> ===================================================================
> --- testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c     (revision 0)
> +++ testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c     (revision 0)
> @@ -0,0 +1,10 @@
> +/* PR 41673: bogus -Wstrict-aliasing warning from VLA dereference.  */
> +/* { dg-do compile } */
> +/* { dg-options "-std=gnu99 -O2 -Wall" } */
> +
> +int main(int argc, char *argv[])
> +{
> +    float x[argc];
> +    float y[argc];
> +    return 0 == __builtin_memcpy(y, x, argc * sizeof(*x));
> +}
> Index: alias.c
> ===================================================================
> --- alias.c     (revision 153482)
> +++ alias.c     (working copy)
> @@ -691,7 +691,14 @@ get_alias_set (tree t)
>      requires structural comparisons to identify compatible types
>      use alias set zero.  */
>   if (TYPE_STRUCTURAL_EQUALITY_P (t))
> -    return 0;
> +    {
> +      /* Allow the language to specify another alias set for this
> +        type.  */
> +      set = lang_hooks.get_alias_set (t);
> +      if (set != -1)
> +       return set;
> +      return 0;
> +    }
>   t = TYPE_CANONICAL (t);
>   /* Canonical types shouldn't form a tree nor should the canonical
>      type require structural equality checks.  */
> Index: c-common.c
> ===================================================================
> --- c-common.c  (revision 153482)
> +++ c-common.c  (working copy)
> @@ -4183,6 +4183,15 @@ c_common_get_alias_set (tree t)
>   tree u;
>   PTR *slot;
>
> +  /* For VLAs, use the alias set of the element type rather than the
> +     default of alias set 0 for types compared structurally.  */
> +  if (TYPE_P (t) && TYPE_STRUCTURAL_EQUALITY_P (t))
> +    {
> +      if (TREE_CODE (t) == ARRAY_TYPE)
> +       return get_alias_set (TREE_TYPE (t));
> +      return -1;
> +    }
> +
>   /* Permit type-punning when accessing a union, provided the access
>      is directly through the union.  For example, this code does not
>      permit taking the address of a union member and then storing
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>

Re: Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Joseph S. Myers]

On Fri, 23 Oct 2009, Richard Guenther wrote:

> The change is ok, but I wonder why the C FE sets
> TYPE_STRUCTURAL_EQUALITY_P on VLAs.  As you can see

It's not the front end setting it - tree.c:build_index_type sets it for 
the index type with nonconstant bounds, while tree.c:build_array_type sets 
it for arrays if it is set for either the element type or the index type.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Richard Guenther]

On Fri, Oct 23, 2009 at 2:21 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Fri, 23 Oct 2009, Richard Guenther wrote:
>
>> The change is ok, but I wonder why the C FE sets
>> TYPE_STRUCTURAL_EQUALITY_P on VLAs.  As you can see
>
> It's not the front end setting it - tree.c:build_index_type sets it for
> the index type with nonconstant bounds, while tree.c:build_array_type sets
> it for arrays if it is set for either the element type or the index type.

Ah, ok.  I wonder if it is better to handle the trivial cases like this
in alias.c and fall back to the structurally canonical type for
getting the alias set in case of TYPE_STRUCTURAL_EQUALITY_P
then.  Which would be for example type_for_mode () for all
integral types, the element type for array types and void *
for pointer types.

That might be a better approach anyway.

Thanks,
Richard.

> --
> Joseph S. Myers
> joseph@codesourcery.com
>

Re: Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Joseph S. Myers]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1214 bytes --]

On Fri, 23 Oct 2009, Richard Guenther wrote:

> On Fri, Oct 23, 2009 at 2:21 PM, Joseph S. Myers
> <joseph@codesourcery.com> wrote:
> > On Fri, 23 Oct 2009, Richard Guenther wrote:
> >
> >> The change is ok, but I wonder why the C FE sets
> >> TYPE_STRUCTURAL_EQUALITY_P on VLAs.  As you can see
> >
> > It's not the front end setting it - tree.c:build_index_type sets it for
> > the index type with nonconstant bounds, while tree.c:build_array_type sets
> > it for arrays if it is set for either the element type or the index type.
> 
> Ah, ok.  I wonder if it is better to handle the trivial cases like this
> in alias.c and fall back to the structurally canonical type for
> getting the alias set in case of TYPE_STRUCTURAL_EQUALITY_P
> then.  Which would be for example type_for_mode () for all
> integral types, the element type for array types and void *
> for pointer types.
> 
> That might be a better approach anyway.

I don't know why alias.c stops where it does for 
TYPE_STRUCTURAL_EQUALITY_P, but reverting the c-common.c change as part of 
changes to make this work in alias.c (that keep the testcase working) is 
preapproved.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Richard Guenther]

On Fri, Oct 23, 2009 at 2:40 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Fri, 23 Oct 2009, Richard Guenther wrote:
>
>> On Fri, Oct 23, 2009 at 2:21 PM, Joseph S. Myers
>> <joseph@codesourcery.com> wrote:
>> > On Fri, 23 Oct 2009, Richard Guenther wrote:
>> >
>> >> The change is ok, but I wonder why the C FE sets
>> >> TYPE_STRUCTURAL_EQUALITY_P on VLAs.  As you can see
>> >
>> > It's not the front end setting it - tree.c:build_index_type sets it for
>> > the index type with nonconstant bounds, while tree.c:build_array_type sets
>> > it for arrays if it is set for either the element type or the index type.
>>
>> Ah, ok.  I wonder if it is better to handle the trivial cases like this
>> in alias.c and fall back to the structurally canonical type for
>> getting the alias set in case of TYPE_STRUCTURAL_EQUALITY_P
>> then.  Which would be for example type_for_mode () for all
>> integral types, the element type for array types and void *
>> for pointer types.
>>
>> That might be a better approach anyway.
>
> I don't know why alias.c stops where it does for
> TYPE_STRUCTURAL_EQUALITY_P, but reverting the c-common.c change as part of
> changes to make this work in alias.c (that keep the testcase working) is
> preapproved.

Thanks.  I'll look into this.

Richard.

Re: Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Richard Guenther]

[-- Attachment #1: Type: text/plain, Size: 2580 bytes --]

On Fri, Oct 23, 2009 at 2:41 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Oct 23, 2009 at 2:40 PM, Joseph S. Myers
> <joseph@codesourcery.com> wrote:
>> On Fri, 23 Oct 2009, Richard Guenther wrote:
>>
>>> On Fri, Oct 23, 2009 at 2:21 PM, Joseph S. Myers
>>> <joseph@codesourcery.com> wrote:
>>> > On Fri, 23 Oct 2009, Richard Guenther wrote:
>>> >
>>> >> The change is ok, but I wonder why the C FE sets
>>> >> TYPE_STRUCTURAL_EQUALITY_P on VLAs.  As you can see
>>> >
>>> > It's not the front end setting it - tree.c:build_index_type sets it for
>>> > the index type with nonconstant bounds, while tree.c:build_array_type sets
>>> > it for arrays if it is set for either the element type or the index type.
>>>
>>> Ah, ok.  I wonder if it is better to handle the trivial cases like this
>>> in alias.c and fall back to the structurally canonical type for
>>> getting the alias set in case of TYPE_STRUCTURAL_EQUALITY_P
>>> then.  Which would be for example type_for_mode () for all
>>> integral types, the element type for array types and void *
>>> for pointer types.
>>>
>>> That might be a better approach anyway.
>>
>> I don't know why alias.c stops where it does for
>> TYPE_STRUCTURAL_EQUALITY_P, but reverting the c-common.c change as part of
>> changes to make this work in alias.c (that keep the testcase working) is
>> preapproved.
>
> Thanks.  I'll look into this.

Thus, I'm playing with the following.  It's assuming that
structural equality applies recursively, thus if it is set on
the array type then the element types also have to be
compared structurally.  At least I can't see how it could
be correct to for S[n] use the alias set of S if the array
type has TYPE_STRUCTURAL_EQUALITY_P set.  This
of course means that this patch will likely uncover
the original problem on a different testcase.

The patch defines structural equality based on mode
equivalence (excluding BLKmode and VOIDmode) and
uses component types where the non-structural path
does (in addition it also does so for complex types).

The patch assumes that TYPE_STRUCTURAL_EQUALITY_P
build a self-contained group, so there is no aliasing between
a type that has TYPE_STRUCTURAL_EQUALITY_P and
a type that has not set TYPE_STRUCTURAL_EQUALITY_P.
Otherwise a type with TYPE_STRUCTURAL_EQUALITY_P
would implicitly make all structural equivalent types subject
to structural comparison - which of course does not work
if we compute alias-sets incrementally and on-demand as we
do now.

Comments?

Thanks,
Richard.

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 4209 bytes --]

2009-10-23  Richard Guenther  <rguenther@suse.de>

	* alias.c (get_mode_alias_set): New function.
	(get_alias_set): Use it for structural canonicalization.
	* c-common.c (c_common_get_alias_set): Remove special-casing
	of array types with structural equality.

Index: gcc/alias.c
===================================================================
*** gcc/alias.c	(revision 153496)
--- gcc/alias.c	(working copy)
*************** component_uses_parent_alias_set (const_t
*** 572,577 ****
--- 572,604 ----
      }
  }
  
+ /* Return the alias set for MODE or -1 if it is not possible to compute.  */
+ 
+ static alias_set_type
+ get_mode_alias_set (enum machine_mode mode)
+ {
+   tree type;
+ 
+   /* We cannot deal with aggregates and incomplete types.  */
+   if (mode == BLKmode
+       || mode == VOIDmode)
+     return -1;
+ 
+   /* Ask the FE for the structural canonical type for this mode.
+      If there is none, give up.  */
+   type = lang_hooks.types.type_for_mode (mode, 0);
+   if (!type)
+     return -1;
+ 
+   gcc_assert (TYPE_MODE (type) == mode);
+ 
+   /* Avoid endless recursion.  */
+   if (TYPE_STRUCTURAL_EQUALITY_P (type))
+     return -1;
+ 
+   return get_alias_set (type);
+ }
+ 
  /* Return the alias set for the memory pointed to by T, which may be
     either a type or an expression.  Return -1 if there is nothing
     special about dereferencing T.  */
*************** get_alias_set (tree t)
*** 687,705 ****
       variant.  */
    t = TYPE_MAIN_VARIANT (t);
  
!   /* Always use the canonical type as well.  If this is a type that
!      requires structural comparisons to identify compatible types
!      use alias set zero.  */
    if (TYPE_STRUCTURAL_EQUALITY_P (t))
      {
        /* Allow the language to specify another alias set for this
  	 type.  */
        set = lang_hooks.get_alias_set (t);
        if (set != -1)
  	return set;
        return 0;
      }
    t = TYPE_CANONICAL (t);
    /* Canonical types shouldn't form a tree nor should the canonical
       type require structural equality checks.  */
    gcc_assert (!TYPE_STRUCTURAL_EQUALITY_P (t) && TYPE_CANONICAL (t) == t);
--- 714,750 ----
       variant.  */
    t = TYPE_MAIN_VARIANT (t);
  
!   /* If this is a type that requires structural comparisons to identify
!      compatible types canonicalize to a structural equivalent type or
!      use alias set zero as a fallback.  */
    if (TYPE_STRUCTURAL_EQUALITY_P (t))
      {
+       tree inner = t;
+ 
+       /* Structural equivalence on simple aggregate types is based on
+ 	 the mode of their ultimate contained element type.  */
+       while (TREE_CODE (inner) == ARRAY_TYPE)
+ 	inner = TREE_TYPE (inner);
+       if (TREE_CODE (inner) == VECTOR_TYPE
+ 	  || TREE_CODE (inner) == COMPLEX_TYPE)
+ 	inner = TREE_TYPE (inner);
+       set = get_mode_alias_set (TYPE_MODE (inner));
+       if (set != -1)
+ 	return set;
+ 
        /* Allow the language to specify another alias set for this
  	 type.  */
        set = lang_hooks.get_alias_set (t);
        if (set != -1)
  	return set;
+ 
+       /* Fall back to alias-set zero.  */
        return 0;
      }
+ 
+   /* Always use the canonical type as well.  */
    t = TYPE_CANONICAL (t);
+ 
    /* Canonical types shouldn't form a tree nor should the canonical
       type require structural equality checks.  */
    gcc_assert (!TYPE_STRUCTURAL_EQUALITY_P (t) && TYPE_CANONICAL (t) == t);
Index: gcc/c-common.c
===================================================================
*** gcc/c-common.c	(revision 153496)
--- gcc/c-common.c	(working copy)
*************** c_common_get_alias_set (tree t)
*** 4183,4197 ****
    tree u;
    PTR *slot;
  
-   /* For VLAs, use the alias set of the element type rather than the
-      default of alias set 0 for types compared structurally.  */
-   if (TYPE_P (t) && TYPE_STRUCTURAL_EQUALITY_P (t))
-     {
-       if (TREE_CODE (t) == ARRAY_TYPE)
- 	return get_alias_set (TREE_TYPE (t));
-       return -1;
-     }
- 
    /* Permit type-punning when accessing a union, provided the access
       is directly through the union.  For example, this code does not
       permit taking the address of a union member and then storing
--- 4183,4188 ----

Re: Alias sets for VLAs and strict aliasing warnings (PR 41673)

[Richard Guenther]

On Fri, Oct 23, 2009 at 4:02 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Oct 23, 2009 at 2:41 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, Oct 23, 2009 at 2:40 PM, Joseph S. Myers
>> <joseph@codesourcery.com> wrote:
>>> On Fri, 23 Oct 2009, Richard Guenther wrote:
>>>
>>>> On Fri, Oct 23, 2009 at 2:21 PM, Joseph S. Myers
>>>> <joseph@codesourcery.com> wrote:
>>>> > On Fri, 23 Oct 2009, Richard Guenther wrote:
>>>> >
>>>> >> The change is ok, but I wonder why the C FE sets
>>>> >> TYPE_STRUCTURAL_EQUALITY_P on VLAs.  As you can see
>>>> >
>>>> > It's not the front end setting it - tree.c:build_index_type sets it for
>>>> > the index type with nonconstant bounds, while tree.c:build_array_type sets
>>>> > it for arrays if it is set for either the element type or the index type.
>>>>
>>>> Ah, ok.  I wonder if it is better to handle the trivial cases like this
>>>> in alias.c and fall back to the structurally canonical type for
>>>> getting the alias set in case of TYPE_STRUCTURAL_EQUALITY_P
>>>> then.  Which would be for example type_for_mode () for all
>>>> integral types, the element type for array types and void *
>>>> for pointer types.
>>>>
>>>> That might be a better approach anyway.
>>>
>>> I don't know why alias.c stops where it does for
>>> TYPE_STRUCTURAL_EQUALITY_P, but reverting the c-common.c change as part of
>>> changes to make this work in alias.c (that keep the testcase working) is
>>> preapproved.
>>
>> Thanks.  I'll look into this.
>
> Thus, I'm playing with the following.  It's assuming that
> structural equality applies recursively, thus if it is set on
> the array type then the element types also have to be
> compared structurally.  At least I can't see how it could
> be correct to for S[n] use the alias set of S if the array
> type has TYPE_STRUCTURAL_EQUALITY_P set.  This
> of course means that this patch will likely uncover
> the original problem on a different testcase.
>
> The patch defines structural equality based on mode
> equivalence (excluding BLKmode and VOIDmode) and
> uses component types where the non-structural path
> does (in addition it also does so for complex types).
>
> The patch assumes that TYPE_STRUCTURAL_EQUALITY_P
> build a self-contained group, so there is no aliasing between
> a type that has TYPE_STRUCTURAL_EQUALITY_P and
> a type that has not set TYPE_STRUCTURAL_EQUALITY_P.
> Otherwise a type with TYPE_STRUCTURAL_EQUALITY_P
> would implicitly make all structural equivalent types subject
> to structural comparison - which of course does not work
> if we compute alias-sets incrementally and on-demand as we
> do now.
>
> Comments?

Thinking about this some more I think we should properly fixup
after the FEs finished (for example during gimplification) so
that we can assert that TYPE_STRUCTURAL_EQUALITY_P is
false in get_alias_set (well, for every type part of a memory reference).

Only the FEs know how equivalence works, and the middle-end
should treat the canonical type as the type defining the alias-set.

We could implement similar type-merging during gimplification
like we do with LTO but relying on the types_compatible langhook
and a new hash_type langhook, computing canonical types of
TYPE_STRUCTURAL_EQUALITY_P on-the-fly.  I'm pretty sure
that my idea of structural equivalence types and non-s-e types
not overlapping isn't what frontends implement, so to avoid
visiting all types another langhook specifying whether a type
may be subject to be the canonical type for types that have
TYPE_STRUCTURAL_EQUALITY_P set could optimize this.

Now one issue is that the middle-end sets TYPE_STRUCTURAL_EQUALITY_P
in the generic type generation routines.  That's of course a very
bad design decision at the time this concept was introduced.  But
maybe we can still get away with that.

I'll give the above a quick implementation re-using the LTO hash
function, but I don't see an easy way out here (without pessimizing
T_S_E_P types by falling back to alias-set zero too often).

Do FE maintainers think that they can do better in merging their
T_S_E_P types properly?  Remember you now have a suitable
point after the TU is finished.

Thanks,
Richard.