Fix gnat.dg/lto20.adb XPASS

Message ID yddwozznlma.fsf@CeBiTec.Uni-Bielefeld.DE
State New
Headers show
Series
  • Fix gnat.dg/lto20.adb XPASS
Related show

Commit Message

Rainer Orth Jan. 30, 2018, 8:21 p.m.
This patch

2018-01-30  Jan Hubicka  <hubicka@ucw.cz>

       gcc:
       PR lto/83954
       * lto-symtab.c (warn_type_compatibility_p): Silence false positive
       for type match warning on arrays of pointers.

       gcc/testsuite:
       PR lto/83954
       * gcc.dg/lto/pr83954.h: New testcase.
       * gcc.dg/lto/pr83954_0.c: New testcase.
       * gcc.dg/lto/pr83954_1.c: New testcase.

(which I didn't see posted on gcc-patches yet) seems to have caused

+XPASS: gnat.dg/lto20.adb (test for excess errors)

(seen on i386-pc-solaris2.11 and sparc-sun-solaris2.11).  The original
warning was

/vol/gcc/src/hg/trunk/local/gcc/testsuite/gnat.dg/lto20_pkg.ads:7:13: warning: type of 'lto20_pkg__proc' does not match original declaration [-Wlto-type-mismatch]

so just removing the dg-excess-errors seems the right thing to do.
Tested with the appropriate runtest invocation on
sparc-sun-solaris2.11.  Ok for mainline?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2018-01-30  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR lto/83954
	* gnat.dg/lto20.adb: Remove dg-excess-errors.

Comments

Eric Botcazou Jan. 31, 2018, 8:01 a.m. | #1
> The original warning was

> 

> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gnat.dg/lto20_pkg.ads:7:13:

> warning: type of 'lto20_pkg__proc' does not match original declaration

> [-Wlto-type-mismatch]

> 

> so just removing the dg-excess-errors seems the right thing to do.

> Tested with the appropriate runtest invocation on

> sparc-sun-solaris2.11.  Ok for mainline?


Yes, thanks.

-- 
Eric Botcazou
Eric Botcazou Jan. 31, 2018, 11:02 a.m. | #2
> 2018-01-30  Jan Hubicka  <hubicka@ucw.cz>

> 

>        gcc:

>        PR lto/83954

>        * lto-symtab.c (warn_type_compatibility_p): Silence false positive

>        for type match warning on arrays of pointers.

> 

>        gcc/testsuite:

>        PR lto/83954

>        * gcc.dg/lto/pr83954.h: New testcase.

>        * gcc.dg/lto/pr83954_0.c: New testcase.

>        * gcc.dg/lto/pr83954_1.c: New testcase.


That being said, I'm not convinced that the patch is correct:

+         /* Alias sets of arrays are the same as alias sets of the inner
+            types.  */
+         while (TREE_CODE (t1) == ARRAY_TYPE && TREE_CODE (t2) == ARRAY_TYPE)
+           {
+             t1 = TREE_TYPE (t1);
+             t2 = TREE_TYPE (t2);
+           }

That's not always true, see get_alias_set:

  /* Unless the language specifies otherwise, treat array types the
     same as their components.  This avoids the asymmetry we get
     through recording the components.  Consider accessing a
     character(kind=1) through a reference to a character(kind=1)[1:1].
     Or consider if we want to assign integer(kind=4)[0:D.1387] and
     integer(kind=4)[4] the same alias set or not.
     Just be pragmatic here and make sure the array and its element
     type get the same alias set assigned.  */
  else if (TREE_CODE (t) == ARRAY_TYPE
	   && (!TYPE_NONALIASED_COMPONENT (t)
	       || TYPE_STRUCTURAL_EQUALITY_P (t)))
    set = get_alias_set (TREE_TYPE (t));

and gnat.dg/lto20.adb is very likely in the TYPE_NONALIASED_COMPONENT case.

-- 
Eric Botcazou
Richard Biener Jan. 31, 2018, 11:07 a.m. | #3
On Wed, Jan 31, 2018 at 12:02 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> 2018-01-30  Jan Hubicka  <hubicka@ucw.cz>

>>

>>        gcc:

>>        PR lto/83954

>>        * lto-symtab.c (warn_type_compatibility_p): Silence false positive

>>        for type match warning on arrays of pointers.

>>

>>        gcc/testsuite:

>>        PR lto/83954

>>        * gcc.dg/lto/pr83954.h: New testcase.

>>        * gcc.dg/lto/pr83954_0.c: New testcase.

>>        * gcc.dg/lto/pr83954_1.c: New testcase.

>

> That being said, I'm not convinced that the patch is correct:

>

> +         /* Alias sets of arrays are the same as alias sets of the inner

> +            types.  */

> +         while (TREE_CODE (t1) == ARRAY_TYPE && TREE_CODE (t2) == ARRAY_TYPE)

> +           {

> +             t1 = TREE_TYPE (t1);

> +             t2 = TREE_TYPE (t2);

> +           }

>

> That's not always true, see get_alias_set:

>

>   /* Unless the language specifies otherwise, treat array types the

>      same as their components.  This avoids the asymmetry we get

>      through recording the components.  Consider accessing a

>      character(kind=1) through a reference to a character(kind=1)[1:1].

>      Or consider if we want to assign integer(kind=4)[0:D.1387] and

>      integer(kind=4)[4] the same alias set or not.

>      Just be pragmatic here and make sure the array and its element

>      type get the same alias set assigned.  */

>   else if (TREE_CODE (t) == ARRAY_TYPE

>            && (!TYPE_NONALIASED_COMPONENT (t)

>                || TYPE_STRUCTURAL_EQUALITY_P (t)))

>     set = get_alias_set (TREE_TYPE (t));

>

> and gnat.dg/lto20.adb is very likely in the TYPE_NONALIASED_COMPONENT case.


Ah, right.  The patch oversimplifies this.  We need to do sth like
TREE_CODE (t1) == ARRAY_TYPE && ! TYPE_NONALIASED_COMPONENT (t1) && ...
right?

Richard.

> --

> Eric Botcazou
Jan Hubicka Jan. 31, 2018, 11:09 a.m. | #4
> > 2018-01-30  Jan Hubicka  <hubicka@ucw.cz>

> > 

> >        gcc:

> >        PR lto/83954

> >        * lto-symtab.c (warn_type_compatibility_p): Silence false positive

> >        for type match warning on arrays of pointers.

> > 

> >        gcc/testsuite:

> >        PR lto/83954

> >        * gcc.dg/lto/pr83954.h: New testcase.

> >        * gcc.dg/lto/pr83954_0.c: New testcase.

> >        * gcc.dg/lto/pr83954_1.c: New testcase.

> 

> That being said, I'm not convinced that the patch is correct:

> 

> +         /* Alias sets of arrays are the same as alias sets of the inner

> +            types.  */

> +         while (TREE_CODE (t1) == ARRAY_TYPE && TREE_CODE (t2) == ARRAY_TYPE)

> +           {

> +             t1 = TREE_TYPE (t1);

> +             t2 = TREE_TYPE (t2);

> +           }

> 

> That's not always true, see get_alias_set:

> 

>   /* Unless the language specifies otherwise, treat array types the

>      same as their components.  This avoids the asymmetry we get

>      through recording the components.  Consider accessing a

>      character(kind=1) through a reference to a character(kind=1)[1:1].

>      Or consider if we want to assign integer(kind=4)[0:D.1387] and

>      integer(kind=4)[4] the same alias set or not.

>      Just be pragmatic here and make sure the array and its element

>      type get the same alias set assigned.  */

>   else if (TREE_CODE (t) == ARRAY_TYPE

> 	   && (!TYPE_NONALIASED_COMPONENT (t)

> 	       || TYPE_STRUCTURAL_EQUALITY_P (t)))

>     set = get_alias_set (TREE_TYPE (t));

> 

> and gnat.dg/lto20.adb is very likely in the TYPE_NONALIASED_COMPONENT case.


OK, so you think the test above should check
 && (!TYPE_NONALIASED_COMPONENT (t1) || TYPE_STRUCTURAL_EQUALITY_P (t1))
 && (!TYPE_NONALIASED_COMPONENT (t2) || TYPE_STRUCTURAL_EQUALITY_P (t2))

Do we have wrong code issue with lto20.adb?

Honza
> 

> -- 

> Eric Botcazou
Eric Botcazou Jan. 31, 2018, 11:43 a.m. | #5
> OK, so you think the test above should check

>  && (!TYPE_NONALIASED_COMPONENT (t1) || TYPE_STRUCTURAL_EQUALITY_P (t1))

>  && (!TYPE_NONALIASED_COMPONENT (t2) || TYPE_STRUCTURAL_EQUALITY_P (t2))


I'm not sure about TYPE_STRUCTURAL_EQUALITY_P (does it matter in LTO mode?) 
but, yes, TYPE_NONALIASED_COMPONENT should be checked I think.

> Do we have wrong code issue with lto20.adb?


No, it's only about the warning.  As a matter of fact, I'm ready to clear 
TYPE_NONALIASED_COMPONENT in the Ada FE for this particular case (array of 
pointers) so that gnat.dg/lto20.adb still passes after the above change.

-- 
Eric Botcazou
Eric Botcazou Feb. 2, 2018, 11:39 a.m. | #6
> Ah, right.  The patch oversimplifies this.  We need to do sth like

> TREE_CODE (t1) == ARRAY_TYPE && ! TYPE_NONALIASED_COMPONENT (t1) && ...

> right?


Right.  Any objection to me applying this?


lto/
	PR lto/83954
	* lto-symtab.c (warn_type_compatibility_p): Do not recurse into the
	component type of array types with non-aliased component.
ada/
	* gcc-interface/decl.c (array_type_has_nonaliased_component): Return
	false if the component type is a pointer.

-- 
Eric Botcazou
Index: ada/gcc-interface/decl.c
===================================================================
--- ada/gcc-interface/decl.c	(revision 257325)
+++ ada/gcc-interface/decl.c	(working copy)
@@ -6113,6 +6113,11 @@ array_type_has_nonaliased_component (tre
       return TYPE_NONALIASED_COMPONENT (gnu_parent_type);
     }
 
+  /* Consider that an array of pointers has an aliased component, which is sort
+     of logical and helps with arrays of Taft Amendment types in LTO mode.  */
+  if (POINTER_TYPE_P (TREE_TYPE (gnu_type)))
+    return false;
+
   /* Otherwise, rely exclusively on properties of the element type.  */
   return type_for_nonaliased_component_p (TREE_TYPE (gnu_type));
 }
Index: lto/lto-symtab.c
===================================================================
--- lto/lto-symtab.c	(revision 257325)
+++ lto/lto-symtab.c	(working copy)
@@ -288,9 +288,12 @@ warn_type_compatibility_p (tree prevaili
 	{
           tree t1 = type, t2 = prevailing_type;
 
-	  /* Alias sets of arrays are the same as alias sets of the inner
-	     types.  */
-	  while (TREE_CODE (t1) == ARRAY_TYPE && TREE_CODE (t2) == ARRAY_TYPE)
+	  /* Alias sets of arrays with aliased components are the same as alias
+	     sets of the inner types.  */
+	  while (TREE_CODE (t1) == ARRAY_TYPE
+		 && !TYPE_NONALIASED_COMPONENT (t1)
+		 && TREE_CODE (t2) == ARRAY_TYPE
+		 && !TYPE_NONALIASED_COMPONENT (t2))
 	    {
 	      t1 = TREE_TYPE (t1);
 	      t2 = TREE_TYPE (t2);
Jan Hubicka Feb. 2, 2018, 11:41 a.m. | #7
> > Ah, right.  The patch oversimplifies this.  We need to do sth like

> > TREE_CODE (t1) == ARRAY_TYPE && ! TYPE_NONALIASED_COMPONENT (t1) && ...

> > right?

> 

> Right.  Any objection to me applying this?


Not form my side - lto-symtab change makes sense to me.
With LTO all array types have canonical type so indeed we can drop the check
for structural equality used by alias.c

Honza
> 

> 

> lto/

> 	PR lto/83954

> 	* lto-symtab.c (warn_type_compatibility_p): Do not recurse into the

> 	component type of array types with non-aliased component.

> ada/

> 	* gcc-interface/decl.c (array_type_has_nonaliased_component): Return

> 	false if the component type is a pointer.

> 

> -- 

> Eric Botcazou


> Index: ada/gcc-interface/decl.c

> ===================================================================

> --- ada/gcc-interface/decl.c	(revision 257325)

> +++ ada/gcc-interface/decl.c	(working copy)

> @@ -6113,6 +6113,11 @@ array_type_has_nonaliased_component (tre

>        return TYPE_NONALIASED_COMPONENT (gnu_parent_type);

>      }

>  

> +  /* Consider that an array of pointers has an aliased component, which is sort

> +     of logical and helps with arrays of Taft Amendment types in LTO mode.  */

> +  if (POINTER_TYPE_P (TREE_TYPE (gnu_type)))

> +    return false;

> +

>    /* Otherwise, rely exclusively on properties of the element type.  */

>    return type_for_nonaliased_component_p (TREE_TYPE (gnu_type));

>  }

> Index: lto/lto-symtab.c

> ===================================================================

> --- lto/lto-symtab.c	(revision 257325)

> +++ lto/lto-symtab.c	(working copy)

> @@ -288,9 +288,12 @@ warn_type_compatibility_p (tree prevaili

>  	{

>            tree t1 = type, t2 = prevailing_type;

>  

> -	  /* Alias sets of arrays are the same as alias sets of the inner

> -	     types.  */

> -	  while (TREE_CODE (t1) == ARRAY_TYPE && TREE_CODE (t2) == ARRAY_TYPE)

> +	  /* Alias sets of arrays with aliased components are the same as alias

> +	     sets of the inner types.  */

> +	  while (TREE_CODE (t1) == ARRAY_TYPE

> +		 && !TYPE_NONALIASED_COMPONENT (t1)

> +		 && TREE_CODE (t2) == ARRAY_TYPE

> +		 && !TYPE_NONALIASED_COMPONENT (t2))

>  	    {

>  	      t1 = TREE_TYPE (t1);

>  	      t2 = TREE_TYPE (t2);

Patch

diff --git a/gcc/testsuite/gnat.dg/lto20.adb b/gcc/testsuite/gnat.dg/lto20.adb
--- a/gcc/testsuite/gnat.dg/lto20.adb
+++ b/gcc/testsuite/gnat.dg/lto20.adb
@@ -1,6 +1,5 @@ 
 -- { dg-do run }
 -- { dg-options "-flto" { target lto } }
--- { dg-excess-errors "does not match original declaration" }
 
 with Lto20_Pkg;