Re: [PATCH] TYPE_TYPELESS_STORAGE handling fixes (PR c++/80534)

[Richard Biener <rguenther at suse dot de>]

On Thu, 27 Apr 2017, Jakub Jelinek wrote:

> Hi!
> 
> As mentioned in the PR, stor-layout.c (layout_type) can set
> TYPE_TYPELESS_STORAGE flag on ARRAY_TYPE that didn't have it before,
> which breaks ARRAY_TYPE hashing in build_array_type_1/type_hash_canon.
> 
> We have 2 cases, one is ARRAY_TYPE with non-AGGREGATE_TYPE_P element type
> (note: TYPE_TYPELESS_STORAGE macro is only allowed on AGGREGATE_TYPE_P
> types), here we want to use that flag in hashing and treat differently
> C++ FE created types with char/unsigned char/signed char/std::byte elements
> from middle-end created arrays or other FE created arrays for LTO.
> And the other case is ARRAY_TYPE with AGGREGATE_TYPE_P element type,
> here the flag is always just inherited (copied) from the element type,
> but it might not be finalized by the time first array of such a type
> is created (added into hash table).  The following patch fixes this
> by only doing hashing and comparison of this flag for ARRAY_TYPEs with
> non-AGGREGATE_TYPE_P elements, for other arrays it is expected that all
> array types with that element type will have the flag set eventually,
> but it might not be finalized immediately.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and 7.1?

Ok.

Thanks,
Richard.

> 2017-04-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/80534
> 	* tree.c (type_cache_hasher::equal): Only compare
> 	TYPE_TYPELESS_STORAGE flag on non-aggregate element types.
> 	(build_array_type_1): Only hash TYPE_TYPELESS_STORAGE flag on
> 	non-aggregate element types.
> 	* tree.h (TYPE_TYPELESS_STORAGE): Fix comment typo, add more details
> 	about the flag on ARRAY_TYPEs in the comment, formatting fix.
> c-family/
> 	* c-common.c (complete_array_type): Only hash TYPE_TYPELESS_STORAGE
> 	flag on non-aggregate element types.
> testsuite/
> 	* g++.dg/other/pr80534-1.C: New test.
> 	* g++.dg/other/pr80534-2.C: New test.
> 
> --- gcc/tree.c.jj	2017-04-27 09:11:09.000000000 +0200
> +++ gcc/tree.c	2017-04-27 11:48:08.177168696 +0200
> @@ -7073,9 +7073,16 @@ type_cache_hasher::equal (type_hash *a,
>          break;
>        return 0;
>      case ARRAY_TYPE:
> -      return (TYPE_TYPELESS_STORAGE (a->type)
> -	      == TYPE_TYPELESS_STORAGE (b->type)
> -	      && TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type));
> +      /* Don't compare TYPE_TYPELESS_STORAGE flag on aggregates,
> +	 where the flag should be inherited from the element type
> +	 and can change after ARRAY_TYPEs are created; on non-aggregates
> +	 compare it and hash it, scalars will never have that flag set
> +	 and we need to differentiate between arrays created by different
> +	 front-ends or middle-end created arrays.  */
> +      return (TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type)
> +	      && (AGGREGATE_TYPE_P (TREE_TYPE (a->type))
> +		  || (TYPE_TYPELESS_STORAGE (a->type)
> +		      == TYPE_TYPELESS_STORAGE (b->type))));
>  
>      case RECORD_TYPE:
>      case UNION_TYPE:
> @@ -8386,7 +8393,8 @@ build_array_type_1 (tree elt_type, tree
>        hstate.add_object (TYPE_HASH (elt_type));
>        if (index_type)
>  	hstate.add_object (TYPE_HASH (index_type));
> -      hstate.add_flag (typeless_storage);
> +      if (!AGGREGATE_TYPE_P (elt_type))
> +	hstate.add_flag (TYPE_TYPELESS_STORAGE (t));
>        t = type_hash_canon (hstate.end (), t);
>      }
>  
> --- gcc/tree.h.jj	2017-04-27 09:11:09.000000000 +0200
> +++ gcc/tree.h	2017-04-27 12:19:32.886969545 +0200
> @@ -2037,10 +2037,16 @@ extern machine_mode element_mode (const_
>  
>  /* For an ARRAY_TYPE, a RECORD_TYPE, a UNION_TYPE or a QUAL_UNION_TYPE
>     whether the array is typeless storage or the type contains a member
> -   with this flag set.  Such types are excempt from type-based alias
> -   analysis.  */
> +   with this flag set.  Such types are exempt from type-based alias
> +   analysis.  For ARRAY_TYPEs with AGGREGATE_TYPE_P element types
> +   the flag should be inherited from the element type, can change
> +   when type is finalized and because of that should not be used in
> +   type hashing.  For ARRAY_TYPEs with non-AGGREGATE_TYPE_P element types
> +   the flag should not be changed after the array is created and should
> +   be used in type hashing.  */
>  #define TYPE_TYPELESS_STORAGE(NODE) \
> -  (TREE_CHECK4 (NODE, RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE, ARRAY_TYPE)->type_common.typeless_storage)
> +  (TREE_CHECK4 (NODE, RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE, \
> +		ARRAY_TYPE)->type_common.typeless_storage)
>  
>  /* Indicated that objects of this type should be laid out in as
>     compact a way as possible.  */
> --- gcc/c-family/c-common.c.jj	2017-04-25 15:51:08.000000000 +0200
> +++ gcc/c-family/c-common.c	2017-04-27 11:47:39.089543361 +0200
> @@ -6371,7 +6371,8 @@ complete_array_type (tree *ptype, tree i
>    inchash::hash hstate;
>    hstate.add_object (TYPE_HASH (unqual_elt));
>    hstate.add_object (TYPE_HASH (TYPE_DOMAIN (main_type)));
> -  hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type));
> +  if (!AGGREGATE_TYPE_P (unqual_elt))
> +    hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type));
>    main_type = type_hash_canon (hstate.end (), main_type);
>  
>    /* Fix the canonical type.  */
> --- gcc/testsuite/g++.dg/other/pr80534-1.C.jj	2017-04-27 11:51:53.278269268 +0200
> +++ gcc/testsuite/g++.dg/other/pr80534-1.C	2017-04-27 11:53:03.733361767 +0200
> @@ -0,0 +1,21 @@
> +// PR c++/80534
> +// { dg-do compile { target c++11 } }
> +// { dg-options "" }
> +
> +template <int> struct A {
> +  struct type {
> +    char __data[0];
> +  };
> +};
> +template <typename _Tp, typename = _Tp> struct B;
> +template <typename _Tp, typename _Dp> struct B<_Tp[], _Dp> {
> +  _Tp _M_t;
> +  using pointer = int;
> +  void m_fn1() {}
> +};
> +struct C {
> +  using Storage = A<0>::type;
> +  using StorageUniquePointer = B<Storage[]>;
> +  void m_fn2() { storageUniquePointer_.m_fn1(); }
> +  StorageUniquePointer storageUniquePointer_;
> +};
> --- gcc/testsuite/g++.dg/other/pr80534-2.C.jj	2017-04-27 11:51:58.419203049 +0200
> +++ gcc/testsuite/g++.dg/other/pr80534-2.C	2017-04-27 11:53:12.952243022 +0200
> @@ -0,0 +1,27 @@
> +// PR c++/80534
> +// { dg-do compile { target c++11 } }
> +// { dg-options "" }
> +
> +template <int, int> struct aligned_storage {
> +  struct type {
> +    char __data[0];
> +  };
> +};
> +struct A {};
> +template <typename _Tp, typename = _Tp> struct unique_ptr;
> +template <typename _Tp, typename _Dp> struct unique_ptr<_Tp[], _Dp> {
> +  int _M_t;
> +  void get() { _M_t; }
> +};
> +struct B {
> +  using Association = A;
> +  using Storage = aligned_storage<sizeof(Association), alignof(Association)>::type;
> +  using StorageUniquePointer = unique_ptr<Storage[]>;
> +  void getAssociationsBegin() { storageUniquePointer_.get(); }
> +  StorageUniquePointer storageUniquePointer_;
> +};
> +struct C {};
> +using MainThreadStaticSignalsReceiver = C;
> +aligned_storage<sizeof(MainThreadStaticSignalsReceiver),
> +                alignof(MainThreadStaticSignalsReceiver)>::type
> +    mainThreadStaticSignalsReceiverStorage;
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)