| Message ID | yddwozznlma.fsf@CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
| Series | Fix gnat.dg/lto20.adb XPASS | expand |
> 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.
> 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.
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
> > 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
> 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.
> 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.
> > 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);
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;
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