From patchwork Mon Mar 21 12:08:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 600092 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 3qTF4F68jpz9s4x for ; Mon, 21 Mar 2016 23:09:15 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=a0ui3YKf; 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:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; q=dns; s=default; b=qPidNsiF6MGlzvMt zpkZAmnFTY/xR7unZqMNciGZlF6EsCZOfig8gnLNwYPcxGDyUKTR8Gt5tdpguzSZ 3gm/WEQIQ05PlyQJxVKJwvJco3xcLrnNweXQCn3JsFf5D75XDExiYt3W1j7Rt4jn t8ol9qEc6hsoWnHMFCLXy2lDXeI= 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:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=default; bh=beH3/MsUTY2t2cVENwq8rE YqBhI=; b=a0ui3YKfkLSgl6XNCc9AQkDx+chm1QvnPa1QMNUCoFU8Fm+rCyx2ei kyGYW6ofWq2ltLh3Nfqhi+VO6GcslsWO7G+r2WvMskaBp05CKG62fyx7b4lOKSQd KSuunFLvmEr95gExkTQgB+gj8tCyJIpaEqDw4IqcuXGQs+Ss2WbrM= Received: (qmail 49107 invoked by alias); 21 Mar 2016 12:09:06 -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 49085 invoked by uid 89); 21 Mar 2016 12:09:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=incompatible, admittedly, indepth, in-depth X-Spam-User: qpsmtpd, 2 recipients X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 21 Mar 2016 12:09:03 +0000 Received: from svr-orw-fem-05.mgc.mentorg.com ([147.34.97.43]) by relay1.mentorg.com with esmtp id 1ahydo-0002Dx-PZ from Thomas_Schwinge@mentor.com ; Mon, 21 Mar 2016 05:09:00 -0700 Received: from tftp-cs (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.3.224.2; Mon, 21 Mar 2016 05:09:00 -0700 Received: by tftp-cs (Postfix, from userid 49978) id F0D4BC2316; Mon, 21 Mar 2016 05:08:59 -0700 (PDT) From: Thomas Schwinge To: Martin Sebor , , CC: Marek Polacek , "Joseph S. Myers" , Jeff Law Subject: [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed) In-Reply-To: <567A271B.2090403@gmail.com> References: <567A271B.2090403@gmail.com> User-Agent: Notmuch/0.9-125-g4686d11 (http://notmuchmail.org) Emacs/24.5.1 (i586-pc-linux-gnu) Date: Mon, 21 Mar 2016 13:08:51 +0100 Message-ID: <87zitsqbho.fsf@kepler.schwinge.homeip.net> MIME-Version: 1.0 Hi! On Tue, 22 Dec 2015 21:46:19 -0700, Martin Sebor wrote: > The attached patch rejects invocations of atomic fetch_op intrinsics > on objects of _Bool type as discussed in the context of PR c/68908. > > Tested on x86_64. I'd noticed something "strange". ;-) For reference: > --- gcc/c-family/c-common.c (revision 231903) > +++ gcc/c-family/c-common.c (working copy) > if (!INTEGRAL_TYPE_P (type) && !POINTER_TYPE_P (type)) > goto incompatible; > > + if (fetch && TREE_CODE (type) == BOOLEAN_TYPE) > + goto incompatible; > + > size = tree_to_uhwi (TYPE_SIZE_UNIT (type)); > if (size == 1 || size == 2 || size == 4 || size == 8 || size == 16) > return size; > > incompatible: > - error ("incompatible type for argument %d of %qE", 1, function); > + error ("operand type %qT is incompatible with argument %d of %qE", > + argtype, 1, function); > return 0; > } When comparing GCC build logs before/after your change got committed (r232147), libstdc++-v3 shows the following configure-time differences: -checking for atomic builtins for bool... yes +checking for atomic builtins for bool... no checking for atomic builtins for short... yes checking for atomic builtins for int... yes checking for atomic builtins for long long... yes -ln -s [...]/source-gcc/libstdc++-v3/config/cpu/generic/atomicity_builtins/atomicity.h- ./atomicity.cc || true +ln -s [...]/source-gcc/libstdc++-v3/config/cpu/i486/atomicity.h ./atomicity.cc || true -ATOMICITY_SRCDIR = config/cpu/generic/atomicity_builtins +ATOMICITY_SRCDIR = config/cpu/i486 /* Define if the compiler supports C++11 atomics. */ -#define _GLIBCXX_ATOMIC_BUILTINS 1 +/* #undef _GLIBCXX_ATOMIC_BUILTINS */ Per the new BOOLEAN_TYPE checking cited above, the following test now is -- expectedly -- failing: configure:15211: checking for atomic builtins for bool configure:15240: [...]/build-gcc/./gcc/xgcc -shared-libgcc -B[...]/build-gcc/./gcc -nostdinc++ -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -B/x86_64-pc-linux-gnu/bin/ -B/x86_64-pc-linux-gnu/lib/ -isystem /x86_64-pc-linux-gnu/include -isystem /x86_64-pc-linux-gnu/sys-include -o conftest -g -O2 -D_GNU_SOURCE -fno-exceptions conftest.cpp >&5 conftest.cpp: In function 'int main()': conftest.cpp:31:52: error: operand type 'atomic_type* {aka bool*}' is incompatible with argument 1 of '__atomic_fetch_add' __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); ^ configure:15240: $? = 1 configure: failed program was: | [...] | int | main () | { | typedef bool atomic_type; | atomic_type c1; | atomic_type c2; | atomic_type c3(0); | __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); | __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, | __ATOMIC_RELAXED); | __atomic_test_and_set(&c1, __ATOMIC_RELAXED); | __atomic_load_n(&c1, __ATOMIC_RELAXED); | | ; | return 0; | } configure:15250: result: no The other data types still work fine: configure:15253: checking for atomic builtins for short configure:15282: [...]/build-gcc/./gcc/xgcc -shared-libgcc -B[...]/build-gcc/./gcc -nostdinc++ -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -B/x86_64-pc-linux-gnu/bin/ -B/x86_64-pc-linux-gnu/lib/ -isystem /x86_64-pc-linux-gnu/include -isystem /x86_64-pc-linux-gnu/sys-include -o conftest -g -O2 -D_GNU_SOURCE -fno-exceptions conftest.cpp >&5 configure:15282: $? = 0 configure:15292: result: yes [same for int and long long] Per my (admittedly, not in-depth) reading of libstdc++-v3 source code, the _GLIBCXX_ATOMIC_BUILTINS conditional is only used in combination with the _Atomic_word data type, which in libstdc++-v3/doc/xml/manual/concurrency_extensions.xml is described as "a signed integral type" (so, matching the semantics as clarified by your patch). That makes sense: it's used to keep reference counts, for example. So, it seems sound to just remove the bool atomics check. That said, I have not looked into why the libstdc++-v3 configure script checks short, int, and long long atomics, but not long. But then, only the short and int results are used to decide on _GLIBCXX_ATOMIC_BUILTINS, and on the other hand there actually are "typedef long _Atomic_word" definitions, but long atomics are not explicitly checked for. OK to commit to following? commit 134784da2f0274b194bfd53264af173d5c5920d4 Author: Thomas Schwinge Date: Mon Mar 21 11:59:03 2016 +0100 [PR c/68966] Restore atomic builtins usage in libstdc++-v3 libstdc++-v3/ PR c/68966 * acinclude.m4 (GLIBCXX_ENABLE_ATOMIC_BUILTINS) : Remove checks. * configure: Regenerate. --- libstdc++-v3/acinclude.m4 | 51 +------------------------- libstdc++-v3/configure | 92 ++++------------------------------------------- 2 files changed, 8 insertions(+), 135 deletions(-) diff --git libstdc++-v3/configure libstdc++-v3/configure index acbc6a6..6f6f1d3 100755 --- libstdc++-v3/configure +++ libstdc++-v3/configure [...] Grüße Thomas diff --git libstdc++-v3/acinclude.m4 libstdc++-v3/acinclude.m4 index 95df24a..145d0f9 100644 --- libstdc++-v3/acinclude.m4 +++ libstdc++-v3/acinclude.m4 @@ -3282,25 +3282,6 @@ AC_DEFUN([GLIBCXX_ENABLE_ATOMIC_BUILTINS], [ CXXFLAGS="$CXXFLAGS -fno-exceptions" - AC_MSG_CHECKING([for atomic builtins for bool]) - AC_CACHE_VAL(glibcxx_cv_atomic_bool, [ - AC_TRY_LINK( - [ ], - [typedef bool atomic_type; - atomic_type c1; - atomic_type c2; - atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); - __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, - __ATOMIC_RELAXED); - __atomic_test_and_set(&c1, __ATOMIC_RELAXED); - __atomic_load_n(&c1, __ATOMIC_RELAXED); - ], - [glibcxx_cv_atomic_bool=yes], - [glibcxx_cv_atomic_bool=no]) - ]) - AC_MSG_RESULT($glibcxx_cv_atomic_bool) - AC_MSG_CHECKING([for atomic builtins for short]) AC_CACHE_VAL(glibcxx_cv_atomic_short, [ AC_TRY_LINK( @@ -3371,35 +3352,6 @@ AC_DEFUN([GLIBCXX_ENABLE_ATOMIC_BUILTINS], [ [#]line __oline__ "configure" int main() { - typedef bool atomic_type; - atomic_type c1; - atomic_type c2; - atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); - __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, - __ATOMIC_RELAXED); - __atomic_test_and_set(&c1, __ATOMIC_RELAXED); - __atomic_load_n(&c1, __ATOMIC_RELAXED); - - return 0; -} -EOF - - AC_MSG_CHECKING([for atomic builtins for bool]) - if AC_TRY_EVAL(ac_compile); then - if grep __atomic_ conftest.s >/dev/null 2>&1 ; then - glibcxx_cv_atomic_bool=no - else - glibcxx_cv_atomic_bool=yes - fi - fi - AC_MSG_RESULT($glibcxx_cv_atomic_bool) - rm -f conftest* - - cat > conftest.$ac_ext << EOF -[#]line __oline__ "configure" -int main() -{ typedef short atomic_type; atomic_type c1; atomic_type c2; @@ -3490,8 +3442,7 @@ EOF AC_LANG_RESTORE # Set atomicity_dir to builtins if all but the long long test above passes. - if test "$glibcxx_cv_atomic_bool" = yes \ - && test "$glibcxx_cv_atomic_short" = yes \ + if test "$glibcxx_cv_atomic_short" = yes \ && test "$glibcxx_cv_atomic_int" = yes; then AC_DEFINE(_GLIBCXX_ATOMIC_BUILTINS, 1, [Define if the compiler supports C++11 atomics.])