From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77409 invoked by alias); 11 Apr 2017 11:53:30 -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 71166 invoked by uid 89); 11 Apr 2017 11:53:25 -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=Disclaimer, tC 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; Tue, 11 Apr 2017 11:53:23 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 157CDAC3B; Tue, 11 Apr 2017 11:53:23 +0000 (UTC) Date: Tue, 11 Apr 2017 11:53:00 -0000 From: Richard Biener To: Jason Merrill cc: Bernd Edlinger , Jakub Jelinek , Jonathan Wakely , Florian Weimer , GCC Patches , Jeff Law , Jan Hubicka Subject: Re: [PATCH] Add a new type attribute always_alias (PR79671) In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="-1609908220-20583472-1491911603=:30715" X-SW-Source: 2017-04/txt/msg00517.txt.bz2 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1609908220-20583472-1491911603=:30715 Content-Type: text/plain; charset=ISO-8859-7 Content-Transfer-Encoding: 8BIT Content-length: 5853 On Tue, 11 Apr 2017, Richard Biener wrote: > On Mon, 10 Apr 2017, Jason Merrill wrote: > > > On Mon, Apr 10, 2017 at 11:30 AM, Richard Biener wrote: > > > On Mon, 10 Apr 2017, Jason Merrill wrote: > > >> On Mon, Apr 10, 2017 at 8:50 AM, Richard Biener wrote: > > >> > * tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE > > >> > for arrays of unsigned char or std::byte. > > >> > > >> I think it would be good to have a flag to select whether these > > >> semantics apply to any char variant and std::byte, only unsigned char > > >> and std::byte, or only std::byte. > > > > > > Any suggestion? Not sure we really need it (I'm hesitant to write > > > all the testcases to verify it actually works). > > > > Well, there's existing code that uses plain char (e.g. boost) that I > > want to support. If there's a significant optimization penalty for > > allowing that, we probably also want to be able to limit the impact. > > If there isn't much penalty, let's just support all char variants. > > I haven't assessed the penalty involved but it can't be worse than > the situation we had in GCC 6. So I think it's reasonable to support > all char variants for now. One could add some instrumenting to > alias_set_subset_of / alias_sets_conflict_p but it would only yield > an upper bound on the number of failed queries (TBAA is a quite early > out before PTA info is used for example). > > The following variant -- added missing > > Index: gcc/cp/tree.c > =================================================================== > --- gcc/cp/tree.c (revision 246832) > +++ gcc/cp/tree.c (working copy) > @@ -972,6 +979,7 @@ build_cplus_array_type (tree elt_type, t > as it will overwrite alignment etc. of all variants. */ > TYPE_SIZE (t) = TYPE_SIZE (m); > TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m); > + TYPE_TYPELESS_STORAGE (t) = TYPE_TYPELESS_STORAGE (m); > } > > TYPE_MAIN_VARIANT (t) = m; > > that caused LTO bootstrap to fail and removed the tree-ssa-structalias.c > change (committed separately) [LTO] bootstrapped and tested ok on > x86_64-unknown-linux-gnu. > > I've tested some template examples and they seem to work fine. > > Ok for trunk? > > Disclaimer: there might still be an issue with cross-language LTO > support, but it might at most result in TYPE_TYPELESS_STORAGE > getting lost. Trying to craft a testcase to verify my theory. Not too difficult in the end (see below). A fix (below) is to not treat arrays with differing TYPE_TYPELESS_STORAGE as compatible for the purpose of computing TYPE_CANONICAL (and thus recursively structures containing them). While they'd still alias each other (because currently a TYPE_TYPELESS_STORAGE member makes the whole thing effectively alias anything) this results in warnings in case such a type is used in the interface between C and C++ (it's also considered a ODR type). t.C:17:17: warning: type of ¡bar¢ does not match original declaration [-Wlto-type-mismatch] extern "C" void bar (X *); ^ t2.c:5:6: note: ¡bar¢ was previously declared here void bar (struct X *p) {} ^ if you add a struct X * parameter to bar(). So it's not the optimal solution here. Another fix would be to somehow make TYPE_TYPELESS_STORAGE "sticky" when merging canonical types but there's not precedent in doing this kind of thing and I'm not sure we'll get everything merged before computing alias sets. CCing Honza. Richard. 2017-04-11 Richard Biener PR middle-end/79671 * tree.c (gimple_canonical_types_compatible_p): Do not treat arrays with differing TYPE_TYPELESS_STORAGE as compatible. * g++.dg/lto/pr79671_0.C: New testcase. * g++.dg/lto/pr79671_1.c: Likewise. Index: gcc/tree.c =================================================================== *** gcc/tree.c (revision 246835) --- gcc/tree.c (working copy) *************** gimple_canonical_types_compatible_p (con *** 13709,13715 **** trust_type_canonical) || TYPE_STRING_FLAG (t1) != TYPE_STRING_FLAG (t2) || TYPE_REVERSE_STORAGE_ORDER (t1) != TYPE_REVERSE_STORAGE_ORDER (t2) ! || TYPE_NONALIASED_COMPONENT (t1) != TYPE_NONALIASED_COMPONENT (t2)) return false; else { --- 13709,13716 ---- trust_type_canonical) || TYPE_STRING_FLAG (t1) != TYPE_STRING_FLAG (t2) || TYPE_REVERSE_STORAGE_ORDER (t1) != TYPE_REVERSE_STORAGE_ORDER (t2) ! || TYPE_NONALIASED_COMPONENT (t1) != TYPE_NONALIASED_COMPONENT (t2) ! || TYPE_TYPELESS_STORAGE (t1) != TYPE_TYPELESS_STORAGE (t2)) return false; else { Index: gcc/testsuite/g++.dg/lto/pr79671_0.C =================================================================== *** gcc/testsuite/g++.dg/lto/pr79671_0.C (nonexistent) --- gcc/testsuite/g++.dg/lto/pr79671_0.C (working copy) *************** *** 0 **** --- 1,26 ---- + // { dg-lto-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 alignas (B), y alignas (B); + new (&x) B (0); + y = x; + B *q = reinterpret_cast (&y); + asm volatile ("" : "=r" (q) : "r" (q)); + return q->i; + } + extern "C" void bar (); + int main() + { + if (foo() != 0) + __builtin_abort (); + bar (); + return 0; + } Index: gcc/testsuite/g++.dg/lto/pr79671_1.c =================================================================== *** gcc/testsuite/g++.dg/lto/pr79671_1.c (nonexistent) --- gcc/testsuite/g++.dg/lto/pr79671_1.c (working copy) *************** *** 0 **** --- 1,5 ---- + struct X + { + unsigned char buf[sizeof (int)]; + }; + void bar () { struct X x; *(volatile char *)x.buf = 1; } ---1609908220-20583472-1491911603=:30715--