On 20/10/16 14:01 +0200, Christophe Lyon wrote:
On 20 October 2016 at 11:40, Jonathan Wakely <jwak...@redhat.com> wrote:
Please CC the libstdc++ list for libstdc++ patches, this is a
requirement for patch submission, see https://gcc.gnu.org/lists.html
OK, I thought I wasn't really modifying the lib itself :-)
On 20/10/16 09:55 +0200, Christophe Lyon wrote:
Hi,
Several times I have noticed/reported test failures because some test
cases wouldn't link on arm-none-eabi using the default 'old' cpu
target: __sync_synchronize cannot be resolved by the linker.
That isn't used by libstdc++ anywhere, so the call to it is being
emitted by the compiler. It would be good to know where it comes from.
The attached long patch adds
+// { dg-require-thread-fence "" }
to all the tests that require it according to the error messages I get.
The change is mechanical:
- insert this line below dg-do if present
- insert this line at the top of the file otherwise
For instance:
diff --git a/libstdc++-v3/testsuite/18_support/bad_typeid/what.cc
b/libstdc++-v3/testsuite/18_support/bad_typeid/what.cc
index 633175b..a048250 100644
--- a/libstdc++-v3/testsuite/18_support/bad_typeid/what.cc
+++ b/libstdc++-v3/testsuite/18_support/bad_typeid/what.cc
@@ -1,3 +1,4 @@
+// { dg-require-thread-fence "" }
// 2007-01-30 Paolo Carlini <pcarl...@suse.de>
// Copyright (C) 2007-2016 Free Software Foundation, Inc.
diff --git a/libstdc++-v3/testsuite/18_support/cxa_vec.cc
b/libstdc++-v3/testsuite/18_support/cxa_vec.cc
index e712655..f2a6c5a 100644
--- a/libstdc++-v3/testsuite/18_support/cxa_vec.cc
+++ b/libstdc++-v3/testsuite/18_support/cxa_vec.cc
@@ -1,4 +1,5 @@
// { dg-do run }
+// { dg-require-thread-fence "" }
// Avoid use of non-overridable new/delete operators in shared
// { dg-options "-static" { target *-*-mingw* } }
// Test __cxa_vec routines
If that's OK, I'm not sure how to write the ChangeLog entry: it
modifies 3287 files.
In my testing, it replaces 3287 FAILs by 3287 UNSUPPORTED.
Wouldn't it be better to remove the dependency on __sync_synchronize
rather than declare nearly 50% of the testsuite UNSUPPORTED?
If 3287 tests can't use it is the resulting libstdc++.so actually
useful to anyone?
I thought I would follow the discussion in
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01196.html
when this directive was introduced.
It makes sense to disable tests for atomics if the target doesn't
support atomics, which was the original purpose of that directive.
But I'm concerned about disabling tests for thousands of tests that
have nothing to do with atomics, like
18_support/numeric_limits/char16_32_t.cc
Furthermore, I saw Ramana's comment in
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
and I assumed the use of _sync_synchronize() was on
purpose.
Ah, so this explains where it's coming from. Any local static variable
that needs a guard will emit a reference to __sync_synchronize. As
Ramana said:
I am considering leaving this in the ARM backend to force people to
think what they want to do about thread safety with statics and C++
on bare-metal systems. If they really do not want thread safety they
can well add -fno-threadsafe-statics or provide an appropriate
implementation for __sync_synchronize on their platforms.
So if libstdc++ is built without -fno-threadsafe-statics for
bare-metal then it needs to link to libatomic or find some other
definition of __sync_synchronize. Alternatively, we could build it
with -fno-threadsafe-statics. That would allow 99% of the library (and
the testsuite) to work correctly.
We might want to review the library for any cases where we are relying
on -fthreadsafe-statics and conditionally perform initialization some
other way, e.g. pthread_once.
