Hi! On Tue, 22 Dec 2015 21:46:19 -0700, Martin Sebor <mse...@gmail.com> 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 <tho...@codesourcery.com>
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) <bool>: Remove
checks.
* configure: Regenerate.
---
libstdc++-v3/acinclude.m4 | 51 +-------------------------
libstdc++-v3/configure | 92 ++++-------------------------------------------
2 files changed, 8 insertions(+), 135 deletions(-)
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.])
diff --git libstdc++-v3/configure libstdc++-v3/configure
index acbc6a6..6f6f1d3 100755
--- libstdc++-v3/configure
+++ libstdc++-v3/configure
[...]
Grüße
Thomas
signature.asc
Description: PGP signature
