From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40297 invoked by alias); 10 Apr 2017 12:50:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 40265 invoked by uid 89); 10 Apr 2017 12:50:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=underway, pta, 3757, Indicated X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 10 Apr 2017 12:50:03 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id B6E45ACD5; Mon, 10 Apr 2017 12:50:02 +0000 (UTC) Date: Mon, 10 Apr 2017 12:50:00 -0000 From: Richard Biener To: Jason Merrill cc: Bernd Edlinger , Jakub Jelinek , Jonathan Wakely , Florian Weimer , GCC Patches , Jeff Law Subject: Re: [PATCH] Add a new type attribute always_alias (PR79671) In-Reply-To: Message-ID: References: <21E940B5-C8C4-4A86-8C15-49A86547DD87@suse.de> <20170405160333.GR4425@redhat.com> <20170405160849.GV17461@tucnak> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2017-04/txt/msg00446.txt.bz2 On Fri, 7 Apr 2017, Jason Merrill wrote: > On Fri, Apr 7, 2017 at 11:32 AM, Bernd Edlinger > wrote: > > On 04/07/17 17:10, Richard Biener wrote: > >> On April 7, 2017 3:37:30 PM GMT+02:00, Bernd Edlinger wrote: > >>> On 04/07/17 08:54, Richard Biener wrote: > >>>> On Thu, 6 Apr 2017, Bernd Edlinger wrote: > >>>>> I think get_alias_set(t) will return 0 for typeless_storage > >>>>> types, and therefore has_zero_child will be set anyway. > >>>>> I think both mean the same thing in the end, but it depends on > >>>>> what typeless_storage should actually mean, and we have > >>>>> not yet the same idea about it. > >>>> > >>>> But has_zero_child does not do what we like it to because otherwise > >>>> in the PR using the char[] array member would have worked! > >>>> > >>>> has_zero_child doesn't do that on purpose of course, but this means > >>>> returing alias-set zero for the typeless storage _member_ doesn't > >>>> suffice. > >>>> > >>> > >>> I see you have a certain idea how to solve the C++17 issue. > >>> And yes, I apologize, if I tried to pee on your tree :) > >> > >> We do have the need to support this part of the C++ standard. For other user code may_alias suffices and I see no reason to haste inventing sth new without a single convincing testcase. GCC/Language extensions should not be added without a good reason. > >> > >> I didn't propose to expose the type flag to users at all. > >> > >> Richard. > >> > > > > Well, actually you did: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79671#c100 > > > > But I won't argue if you prefer to do it as you like, > > after all TBAA is your area. So that is fine for me. > > > > Attached is the latest version of my patch, I fixed > > the issue with the type-flag conversion, and it is > > already fully functional. > > I agree that we don't want a new attribute. > > This should not be limited to C++17 mode; std::aligned_storage is in > C++11 and we might as well have the same behavior in C++98 mode. So here's my variant of a fix. I constrained the new flag TYPE_TYPELESS_STORAGE to arrays and thus hope my localized fix in build_cplus_array_type will suffice (if I have time I'll play with template instantiation). I've created a nice small testcase that exposes the issue but it needs a PTA fix to work (I'll commit the tree-ssa-structalias.c fix separately). Some more obfuscation might remove the requirement of it (don't use an asm but a noinline const function maybe). Bootstrap and regtest is underway on x86_64-unknown-linux-gnu. There may be some issues with LTO / cross-language support remaining, I need to think more about this (TYPE_CANONICAL merging plus alias.c punning down to TYPE_CANONICAL). Richard. 2017-04-10 Richard Biener PR c++/79671 cp/ * tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE for arrays of unsigned char or std::byte. * tree-core.h (tree_type_common): Add typeless_storage flag. * tree.h (TYPE_TYPELESS_STORAGE): New macro. * alias.c (alias_set_entry): Add has_typeless_storage member. (alias_set_subset_of): Handle it. (alias_sets_conflict_p): Likewise. (init_alias_set_entry): Initialize it. (get_alias_set): For ARRAY_TYPEs handle TYPE_TYPELESS_STORAGE. (record_alias_subset): Likewise. * lto-streamer-out.c (hash_tree): Hash TYPE_TYPELESS_STORAGE. * tree-streamer-in.c (unpack_ts_type_common_value_fields): Stream TYPE_TYPELESS_STORAGE. * tree-streamer-out.c (pack_ts_type_common_value_fields): Likewise. * tree-ssa-structalias.c (find_func_aliases): Properly handle asm inputs. lto/ * lto.c (compare_tree_sccs_1): Compare TYPE_TYPELESS_STORAGE. * g++.dg/torture/pr79671.C: New testcase. Index: gcc/alias.c =================================================================== --- gcc/alias.c (revision 246803) +++ gcc/alias.c (working copy) @@ -136,6 +136,9 @@ struct GTY(()) alias_set_entry { bool is_pointer; /* Nonzero if is_pointer or if one of childs have has_pointer set. */ bool has_pointer; + /* Nonzero if we have a child serving as typeless storage (or are + such storage ourselves). */ + bool has_typeless_storage; /* The children of the alias set. These are not just the immediate children, but, in fact, all descendants. So, if we have: @@ -419,7 +422,8 @@ alias_set_subset_of (alias_set_type set1 /* Check if set1 is a subset of set2. */ ase2 = get_alias_set_entry (set2); if (ase2 != 0 - && (ase2->has_zero_child + && (ase2->has_typeless_storage + || ase2->has_zero_child || (ase2->children && ase2->children->get (set1)))) return true; @@ -480,7 +484,8 @@ alias_sets_conflict_p (alias_set_type se /* See if the first alias set is a subset of the second. */ ase1 = get_alias_set_entry (set1); if (ase1 != 0 - && ase1->children && ase1->children->get (set2)) + && (ase1->has_typeless_storage + || (ase1->children && ase1->children->get (set2)))) { ++alias_stats.num_dag; return 1; @@ -489,7 +494,8 @@ alias_sets_conflict_p (alias_set_type se /* Now do the same, but with the alias sets reversed. */ ase2 = get_alias_set_entry (set2); if (ase2 != 0 - && ase2->children && ase2->children->get (set1)) + && (ase2->has_typeless_storage + || (ase2->children && ase2->children->get (set1)))) { ++alias_stats.num_dag; return 1; @@ -825,6 +831,7 @@ init_alias_set_entry (alias_set_type set ase->has_zero_child = false; ase->is_pointer = false; ase->has_pointer = false; + ase->has_typeless_storage = false; gcc_checking_assert (!get_alias_set_entry (set)); (*alias_sets)[set] = ase; return ase; @@ -955,6 +962,7 @@ get_alias_set (tree t) 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_TYPELESS_STORAGE (t) && (!TYPE_NONALIASED_COMPONENT (t) || TYPE_STRUCTURAL_EQUALITY_P (t))) set = get_alias_set (TREE_TYPE (t)); @@ -1094,6 +1102,15 @@ get_alias_set (tree t) TYPE_ALIAS_SET (t) = set; + if (TREE_CODE (t) == ARRAY_TYPE + && TYPE_TYPELESS_STORAGE (t)) + { + alias_set_entry *ase = get_alias_set_entry (set); + if (!ase) + ase = init_alias_set_entry (set); + ase->has_typeless_storage = true; + } + /* If this is an aggregate type or a complex type, we must record any component aliasing information. */ if (AGGREGATE_TYPE_P (t) || TREE_CODE (t) == COMPLEX_TYPE) @@ -1173,6 +1190,8 @@ record_alias_subset (alias_set_type supe superset_entry->has_zero_child = true; if (subset_entry->has_pointer) superset_entry->has_pointer = true; + if (subset_entry->has_typeless_storage) + superset_entry->has_typeless_storage = true; if (subset_entry->children) { Index: gcc/cp/tree.c =================================================================== --- gcc/cp/tree.c (revision 246803) +++ gcc/cp/tree.c (working copy) @@ -938,6 +938,11 @@ build_cplus_array_type (tree elt_type, t else { t = build_array_type (elt_type, index_type); + if (elt_type == unsigned_char_type_node + || (TREE_CODE (elt_type) == ENUMERAL_TYPE + && TYPE_CONTEXT (elt_type) == std_node + && !strcmp ("byte", TYPE_NAME_STRING (elt_type)))) + TYPE_TYPELESS_STORAGE (t) = 1; } /* Now check whether we already have this array variant. */ Index: gcc/lto/lto.c =================================================================== --- gcc/lto/lto.c (revision 246803) +++ gcc/lto/lto.c (working copy) @@ -1161,7 +1161,10 @@ compare_tree_sccs_1 (tree t1, tree t2, t compare_values (TYPE_FINAL_P); } else if (code == ARRAY_TYPE) - compare_values (TYPE_NONALIASED_COMPONENT); + { + compare_values (TYPE_NONALIASED_COMPONENT); + compare_values (TYPE_TYPELESS_STORAGE); + } compare_values (TYPE_PACKED); compare_values (TYPE_RESTRICT); compare_values (TYPE_USER_ALIGN); Index: gcc/lto-streamer-out.c =================================================================== --- gcc/lto-streamer-out.c (revision 246803) +++ gcc/lto-streamer-out.c (working copy) @@ -1142,7 +1142,10 @@ hash_tree (struct streamer_tree_cache_d hstate.add_flag (TYPE_FINAL_P (t)); } else if (code == ARRAY_TYPE) - hstate.add_flag (TYPE_NONALIASED_COMPONENT (t)); + { + hstate.add_flag (TYPE_NONALIASED_COMPONENT (t)); + hstate.add_flag (TYPE_TYPELESS_STORAGE (t)); + } hstate.commit_flag (); hstate.add_int (TYPE_PRECISION (t)); hstate.add_int (TYPE_ALIGN (t)); Index: gcc/testsuite/g++.dg/torture/pr79671.C =================================================================== --- gcc/testsuite/g++.dg/torture/pr79671.C (nonexistent) +++ gcc/testsuite/g++.dg/torture/pr79671.C (working copy) @@ -0,0 +1,25 @@ +// { dg-do run } + +void *operator new(__SIZE_TYPE__, void *p2) { return p2; } +struct B { B(int i_) : i(i_) {} int i; }; +struct X +{ + unsigned char buf[sizeof (B)]; +}; + +int __attribute__((noinline)) foo() +{ + X x, y; + new (&x) B (0); + y = x; + B *q = reinterpret_cast (&y); + asm volatile ("" : "=r" (q) : "r" (q)); + return q->i; +} + +int main() +{ + if (foo() != 0) + __builtin_abort (); + return 0; +} Index: gcc/tree-core.h =================================================================== --- gcc/tree-core.h (revision 246803) +++ gcc/tree-core.h (working copy) @@ -1511,7 +1511,9 @@ struct GTY(()) tree_type_common { so we need to store the value 32 (not 31, as we need the zero as well), hence six bits. */ unsigned align : 6; - unsigned spare : 25; + unsigned typeless_storage : 1; + unsigned spare : 24; + alias_set_type alias_set; tree pointer_to; tree reference_to; Index: gcc/tree-ssa-structalias.c =================================================================== --- gcc/tree-ssa-structalias.c (revision 246803) +++ gcc/tree-ssa-structalias.c (working copy) @@ -4944,14 +4944,14 @@ find_func_aliases (struct function *fn, make_escape_constraint (build_fold_addr_expr (op)); /* The asm may read global memory, so outputs may point to - any global memory. */ + any global or escaped memory. */ if (op) { auto_vec lhsc; struct constraint_expr rhsc, *lhsp; unsigned j; get_constraint_for (op, &lhsc); - rhsc.var = nonlocal_id; + rhsc.var = escaped_id; rhsc.offset = 0; rhsc.type = SCALAR; FOR_EACH_VEC_ELT (lhsc, j, lhsp) Index: gcc/tree-streamer-in.c =================================================================== --- gcc/tree-streamer-in.c (revision 246803) +++ gcc/tree-streamer-in.c (working copy) @@ -375,7 +375,10 @@ unpack_ts_type_common_value_fields (stru TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1); } else if (TREE_CODE (expr) == ARRAY_TYPE) - TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1); + { + TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1); + TYPE_TYPELESS_STORAGE (expr) = (unsigned) bp_unpack_value (bp, 1); + } TYPE_PRECISION (expr) = bp_unpack_var_len_unsigned (bp); SET_TYPE_ALIGN (expr, bp_unpack_var_len_unsigned (bp)); #ifdef ACCEL_COMPILER Index: gcc/tree-streamer-out.c =================================================================== --- gcc/tree-streamer-out.c (revision 246803) +++ gcc/tree-streamer-out.c (working copy) @@ -327,7 +327,10 @@ pack_ts_type_common_value_fields (struct bp_pack_value (bp, TYPE_FINAL_P (expr), 1); } else if (TREE_CODE (expr) == ARRAY_TYPE) - bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1); + { + bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1); + bp_pack_value (bp, TYPE_TYPELESS_STORAGE (expr), 1); + } bp_pack_var_len_unsigned (bp, TYPE_PRECISION (expr)); bp_pack_var_len_unsigned (bp, TYPE_ALIGN (expr)); } Index: gcc/tree.h =================================================================== --- gcc/tree.h (revision 246803) +++ gcc/tree.h (working copy) @@ -2035,6 +2035,9 @@ extern machine_mode element_mode (const_ #define TYPE_NONALIASED_COMPONENT(NODE) \ (ARRAY_TYPE_CHECK (NODE)->type_common.transparent_aggr_flag) +#define TYPE_TYPELESS_STORAGE(NODE) \ + (ARRAY_TYPE_CHECK (NODE)->type_common.typeless_storage) + /* Indicated that objects of this type should be laid out in as compact a way as possible. */ #define TYPE_PACKED(NODE) (TYPE_CHECK (NODE)->base.u.bits.packed_flag)