From patchwork Wed Mar 25 16:22:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 454613 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 34FF61400A0 for ; Thu, 26 Mar 2015 03:23:43 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=H7srNu9Q; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=I9pDEAGreR0CIBGQm 1luLjxFPjg37/LlXMU3rc7VaWkTqii6NHDD10MTfYQYRLwwzY6TYl6qAiQPw8qkY XdYCLIqSD3x8OeCoaTRO1siwBJIOEBFUehKOTvAcq9FIN5FQp8vQxtR+NQAD3irr 7V7uRyR/b4hgedJRFvl+KggXGA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=3unaSkrUPcfSnqGb8zndkUD kjpI=; b=H7srNu9Q1AwnVZBSz23aw+M8FkxlVviBYr78M+dY5yUcD9TW3vEmynA 4Vd/U+2q13h7f0JPx/n4X2iE1ymgn/Z2o1xPHan7f6nCxZy+ddt7B4EKAJv2hlwX fCkV+jEZx/7ene/ueKJl3KwOFDdTd+bgUEnZd+7GPx1Bgi2Njyy4= Received: (qmail 6490 invoked by alias); 25 Mar 2015 16:22:52 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 6413 invoked by uid 89); 25 Mar 2015 16:22:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 25 Mar 2015 16:22:48 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2PGMk4Z027101 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 25 Mar 2015 12:22:46 -0400 Received: from localhost (ovpn-116-96.ams2.redhat.com [10.36.116.96]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2PGMjmJ004903; Wed, 25 Mar 2015 12:22:46 -0400 Date: Wed, 25 Mar 2015 16:22:45 +0000 From: Jonathan Wakely To: Richard Henderson Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org, Andrew MacLeod Subject: Re: [libstdc++/65033] Give alignment info to libatomic Message-ID: <20150325162244.GF9755@redhat.com> References: <54DD19B7.6060401@redhat.com> <20150218121512.GI3360@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150218121512.GI3360@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) On 18/02/15 12:15 +0000, Jonathan Wakely wrote: >On 12/02/15 13:23 -0800, Richard Henderson wrote: >>When we fixed PR54005, making sure that atomic_is_lock_free returns the same >>value for all objects of a given type, we probably should have changed the >>interface so that we would pass size and alignment rather than size and object >>pointer. >> >>Instead, we decided that passing null for the object pointer would be >>sufficient. But as this PR shows, we really do need to take alignment into >>account. >> >>The following patch constructs a fake object pointer that is maximally >>misaligned. This allows the interface to both the builtin and to libatomic to >>remain unchanged. Which probably makes this back-portable to maintenance >>releases as well. > >Am I right in thinking that another option would be to ensure that >std::atomic<> objects are always suitably aligned? Would that make >std::atomic<> slightly more compatible with a C11 atomic_int, where >the _Atomic qualifier affects alignment? > >https://gcc.gnu.org/PR62259 suggests we might need to enforce >alignment on std::atomic anyway, or am I barking up the wrong tree? > I've convinced myself that Richard's patch is correct in all cases, but I think we also want this patch, to fix PR62259 and PR65147. For the generic std::atomic (i.e. not the integral or pointer specializations) we should increase the alignment of atomic types that have the same size as one of the standard integral types. This should be consistent with what the C front end does for _Atomic, based on what Joseph told me on IRC: jwakely: _Atomic aligns 1/2/4/8/16-byte types the same as integer types of that size. (Which may not be alignment = size, depending on the architecture.) Ideally we'd use an attribute like Andrew describes at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259#c4 but that's not going to happen for GCC 5, so this just looks for an integral type of the same size and uses its alignment. Tested x86_64-linux, powerpc64le-linux. I'll wait for RM approval for this and Richard's patch (which is OK from a libstdc++ perspective). commit bdcba837b42bbef3143ea59a0194bd3ef435dfcb Author: Jonathan Wakely Date: Wed Sep 3 15:39:53 2014 +0100 PR libstdc++/62259 PR libstdc++/65147 * include/std/atomic (atomic): Increase alignment for types with the same size as one of the integral types. * testsuite/29_atomics/atomic/60695.cc: Adjust dg-error line number. * testsuite/29_atomics/atomic/62259.cc: New. diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic index cc4b5f1..5f02fe8 100644 --- a/libstdc++-v3/include/std/atomic +++ b/libstdc++-v3/include/std/atomic @@ -165,7 +165,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct atomic { private: - _Tp _M_i; + // Align 1/2/4/8/16-byte types the same as integer types of that size. + // This matches the alignment effects of the C11 _Atomic qualifier. + static constexpr int _S_alignment + = sizeof(_Tp) == sizeof(char) ? alignof(char) + : sizeof(_Tp) == sizeof(short) ? alignof(short) + : sizeof(_Tp) == sizeof(int) ? alignof(int) + : sizeof(_Tp) == sizeof(long) ? alignof(long) + : sizeof(_Tp) == sizeof(long long) ? alignof(long long) +#ifdef _GLIBCXX_USE_INT128 + : sizeof(_Tp) == sizeof(__int128) ? alignof(__int128) +#endif + : alignof(_Tp); + + alignas(_S_alignment) _Tp _M_i; static_assert(__is_trivially_copyable(_Tp), "std::atomic requires a trivially copyable type"); diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc b/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc index b59c6ba..806ccb1 100644 --- a/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc +++ b/libstdc++-v3/testsuite/29_atomics/atomic/60695.cc @@ -27,4 +27,4 @@ struct X { char stuff[0]; // GNU extension, type has zero size }; -std::atomic a; // { dg-error "not supported" "" { target *-*-* } 173 } +std::atomic a; // { dg-error "not supported" "" { target *-*-* } 186 } diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc b/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc new file mode 100644 index 0000000..cfe70b1 --- /dev/null +++ b/libstdc++-v3/testsuite/29_atomics/atomic/62259.cc @@ -0,0 +1,56 @@ +// Copyright (C) 2015 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-require-atomic-builtins "" } +// { dg-require-cstdint "" } +// { dg-options "-std=gnu++11" } +// { dg-do compile } + +#include +#include + +// libstdc++/62259 + +struct twoints { + int a; + int b; +}; + +static_assert( alignof(std::atomic) > alignof(int), + "std::atomic not suitably aligned" ); + +// libstdc++/65147 + +struct power_of_two_obj { + char c [8]; +}; + +std::atomic obj1; + +static_assert( alignof(obj1) == alignof(std::int64_t), + "std::atomic not suitably aligned" ); + +struct container_struct { + char c[1]; + std::atomic ao; +}; + +container_struct obj2; + +static_assert( alignof(obj2.ao) > alignof(char), + "std::atomic not suitably aligned" ); +