[libstdc++, testsuite] Add dg-require-thread-fence

[libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

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.

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  <pcarlini@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.


OK?

Other question: I've noticed similar errors in the g++ validation, but
I'm not sure what is the corresponding dg-require directive?

Thanks,

Christophe

[-- Attachment #2: require-thread-fence.patch.xz --]
[-- Type: application/force-download, Size: 99632 bytes --]

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

On 20 October 2016 at 09:55, Christophe Lyon <christophe.lyon@linaro.org> 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.
>
> 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  <pcarlini@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.
>
>
> OK?
>

Jonathan,

The new test you introduced yesterday would need a similar fix:
experimental/memory/shared_ptr/cons/enable_shared_from_this.cc

Christophe

> Other question: I've noticed similar errors in the g++ validation, but
> I'm not sure what is the corresponding dg-require directive?
>
> Thanks,
>
> Christophe

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

Please CC the libstdc++ list for libstdc++ patches, this is a
requirement for patch submission, see https://gcc.gnu.org/lists.html


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  <pcarlini@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?

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 20/10/16 10:40 +0100, Jonathan Wakely wrote:
>Please CC the libstdc++ list for libstdc++ patches, this is a
>requirement for patch submission, see https://gcc.gnu.org/lists.html

CCing ...

>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  <pcarlini@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?
>

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

On 20 October 2016 at 11:40, Jonathan Wakely <jwakely@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  <pcarlini@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.

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.

I'm happy to prepare a patch with a better fix, as I'd like
to get rid of these errors.

Thanks,

Christophe

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 20/10/16 14:01 +0200, Christophe Lyon wrote:
>On 20 October 2016 at 11:40, Jonathan Wakely <jwakely@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  <pcarlini@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.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Mike Stump]

On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> 
> 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.

Not quite in the GNU spirit?  The port people should decide the best way to get as much functionality as possible and everything should just work, no sharp edges.

Forcing people to think sounds like a sharp edge?

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 20/10/16 09:26 -0700, Mike Stump wrote:
>On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> 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.

The quoting makes it look like those are my words, but I was quoting
Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html

>Not quite in the GNU spirit?  The port people should decide the best way to get as much functionality as possible and everything should just work, no sharp edges.
>
>Forcing people to think sounds like a sharp edge?

I'm inclined to agree, but we are talking about bare metal systems,
where there is no one-size-fits-all solution. Choosing something that
makes most of the library unusable will upset one group of people, and
choosing something that adds overhead that could be avoided will upset
another group.

Either way, I don't think disabling 50% of the testsuite is the
answer. If you don't like the failures, configure the library to build
without threadsafe statics, or configure it to depend on libatomic, or
something else. (We might want new --enable-xxx switches to simplify
doing that).

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 20/10/16 10:40 +0100, Jonathan Wakely wrote:
>Please CC the libstdc++ list for libstdc++ patches, this is a
>requirement for patch submission, see https://gcc.gnu.org/lists.html
>
>
>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  <pcarlini@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?

Are those *all* the tests that link to libstdc++.so / libstdc++.a and
aren't disabled for arm-none-eabi by some other directive? It would be
about the right number.

If Every. Single. Test. that uses the libstdc++ library has this
failure, and the library can't be made to be usable, the answer is
surely to change the meaning of "dg-do run" to not link+run tests, not
add a new directive to Every. Single. Test.  (and every single test we
add in future too!)

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Ville Voutilainen]

On 20 October 2016 at 19:34, Jonathan Wakely <jwakely@redhat.com> wrote:
> Either way, I don't think disabling 50% of the testsuite is the
> answer. If you don't like the failures, configure the library to build
> without threadsafe statics, or configure it to depend on libatomic, or
> something else. (We might want new --enable-xxx switches to simplify
> doing that).
>

I think having to add a dg-require to *every* run-time test we have,
even and especially to ones that have *nothing* to do with
any threading is an indication that this approach might not be a good
way to solve the problem.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Ville Voutilainen]

On 20 October 2016 at 19:51, Jonathan Wakely <jwakely@redhat.com> wrote:
> add a new directive to Every. Single. Test.  (and every single test we
> add in future too!)


Uh, that would be a rather unfortunate burden for every library patch
submitter, and to the maintainer
as well, because we _will_ forget it and then it will break bare-metal
arm on every patch.
Let's please figure out a better way to solve this problem.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Mike Stump]

On Oct 20, 2016, at 9:51 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> If Every. Single. Test. that uses the libstdc++ library has this
> failure, and the library can't be made to be usable, the answer is
> surely to change the meaning of "dg-do run" to not link+run tests, not
> add a new directive to Every. Single. Test.  (and every single test we
> add in future too!)

So, from a test suite perspective, the correct fix it to make the port just work.  I have a bare metal port, I test libstdc++.
I'd be curious to hear from the arm folks about it.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Ville Voutilainen]

On 20 October 2016 at 20:08, Mike Stump <mikestump@comcast.net> wrote:
> So, from a test suite perspective, the correct fix it to make the port just work.  I have a bare metal port, I test libstdc++.
> I'd be curious to hear from the arm folks about it.


I daresay that would be the correct fix from many other perspectives
besides just from a test suite one. :)

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Mike Stump]

On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> 
> On 20/10/16 09:26 -0700, Mike Stump wrote:
>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>> 
>>> 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.
> 
> The quoting makes it look like those are my words, but I was quoting
> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
> 
>> Not quite in the GNU spirit?  The port people should decide the best way to get as much functionality as possible and everything should just work, no sharp edges.
>> 
>> Forcing people to think sounds like a sharp edge?
> 
> I'm inclined to agree, but we are talking about bare metal systems,

So?  gcc has been doing bare metal systems for more than 2 years now.  It is pretty good at it.  All my primary targets today are themselves bare metal systems (I test with newlib).

> where there is no one-size-fits-all solution.

Configurations are like ice cream cones.  Everyone gets their flavor no matter how weird or strange.  Putting nails in a cone because you don't know if they like vanilla or chocolate isn't reasonable.  If you want, make two flavors, and vend two, if you want to just do one, pick the flavor and vend it.  Put an enum #define default_flavor vanilla, and you then have support for any flavor you want.  Want to add a configure option for the flavor select, add it.  You want to make a -mflavor=chocolate option, add it.  gcc is literally littered with these things.

Anything vended should just work.  If it doesn't, that's a bug that needs fixing.  If a port person doesn't understand, we can educate them why _it just works_, is a nice design philosophy; maybe it is new to them.

> Choosing something that makes most of the library unusable will upset one group of people, and
> choosing something that adds overhead that could be avoided will upset
> another group.

No, this is a misunderstanding.  Users want things to just work, really.  Bosses often like it when things just work as well; so it's not just users.  Customers often like it as well.  Anyway, that's my experience.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 20/10/16 10:33 -0700, Mike Stump wrote:
>On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>
>>>> 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.
>>
>> The quoting makes it look like those are my words, but I was quoting
>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>
>>> Not quite in the GNU spirit?  The port people should decide the best way to get as much functionality as possible and everything should just work, no sharp edges.
>>>
>>> Forcing people to think sounds like a sharp edge?
>>
>> I'm inclined to agree, but we are talking about bare metal systems,
>
>So?  gcc has been doing bare metal systems for more than 2 years now.  It is pretty good at it.  All my primary targets today are themselves bare metal systems (I test with newlib).
>
>> where there is no one-size-fits-all solution.
>
>Configurations are like ice cream cones.  Everyone gets their flavor no matter how weird or strange.  Putting nails in a cone because you don't know if they like vanilla or chocolate isn't reasonable.  If you want, make two flavors, and vend two, if you want to just do one, pick the flavor and vend it.  Put an enum #define default_flavor vanilla, and you then have support for any flavor you want.  Want to add a configure option for the flavor select, add it.  You want to make a -mflavor=chocolate option, add it.  gcc is literally littered with these things.

Like I said, you can either build the library with
-fno-threadsafe-statics or you can provide a definition of the missing
symbol.

>Anything vended should just work.  If it doesn't, that's a bug that needs fixing.  If a port person doesn't understand, we can educate them why _it just works_, is a nice design philosophy; maybe it is new to them.

Which is basically what I'm saying. Marking 3000 tests UNSUPPORTED to
make some test results look clean is not a fix for anything.


>> Choosing something that makes most of the library unusable will upset one group of people, and
>> choosing something that adds overhead that could be avoided will upset
>> another group.
>
>No, this is a misunderstanding.  Users want things to just work, really.  Bosses often like it when things just work as well; so it's not just users.  Customers often like it as well.  Anyway, that's my experience.


OK, I'll put it another way. Under no circumstances am I going to
accept a patch that requires adding the same redundant directive to
every single 'do-dg run' test in libstdc++-v3/testsuite/.

Right now I don't care how or if the FAILs get fixed, but it won't be
by individually marking every file as UNSUPPORTED.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

On 20 October 2016 at 18:51, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 20/10/16 10:40 +0100, Jonathan Wakely wrote:
>>
>> Please CC the libstdc++ list for libstdc++ patches, this is a
>> requirement for patch submission, see https://gcc.gnu.org/lists.html
>>
>>
>> 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  <pcarlini@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?
>
>
> Are those *all* the tests that link to libstdc++.so / libstdc++.a and
> aren't disabled for arm-none-eabi by some other directive? It would be
> about the right number.
>
> If Every. Single. Test. that uses the libstdc++ library has this
> failure, and the library can't be made to be usable, the answer is
> surely to change the meaning of "dg-do run" to not link+run tests, not
> add a new directive to Every. Single. Test.  (and every single test we
> add in future too!)
>
I'm not sure what you really mean here.
I can see 147 execution tests pass, 1 fail.
and 3287 fail to link.

So that's most of the executable tests, yes.

And I agree that my patch is not a viable solution.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

[ccying Ramana]

On 20 October 2016 at 18:34, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>
>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>
>>>
>>> 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.
>
>
> The quoting makes it look like those are my words, but I was quoting
> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>
>> Not quite in the GNU spirit?  The port people should decide the best way
>> to get as much functionality as possible and everything should just work, no
>> sharp edges.
>>
>> Forcing people to think sounds like a sharp edge?
>
>
> I'm inclined to agree, but we are talking about bare metal systems,
> where there is no one-size-fits-all solution. Choosing something that
> makes most of the library unusable will upset one group of people, and
> choosing something that adds overhead that could be avoided will upset
> another group.
>
> Either way, I don't think disabling 50% of the testsuite is the
> answer. If you don't like the failures, configure the library to build
> without threadsafe statics, or configure it to depend on libatomic, or
> something else. (We might want new --enable-xxx switches to simplify
> doing that).
>

So if we say that the current behaviour has to keep being the default,
so that users think about what they are really doing, I can certainly
patch my validation scripts to add a configure flag for this particular
configuration.

Is arm-none-eabi the only target having this problem?

Thanks,

Christophe

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Kyrill Tkachov]

Hi all,

On 21/10/16 09:00, Christophe Lyon wrote:
> [ccying Ramana]

Ramana is currently OoO all of this week.

Kyrill

> On 20 October 2016 at 18:34, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>
>>>> 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.
>>
>> The quoting makes it look like those are my words, but I was quoting
>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>
>>> Not quite in the GNU spirit?  The port people should decide the best way
>>> to get as much functionality as possible and everything should just work, no
>>> sharp edges.
>>>
>>> Forcing people to think sounds like a sharp edge?
>>
>> I'm inclined to agree, but we are talking about bare metal systems,
>> where there is no one-size-fits-all solution. Choosing something that
>> makes most of the library unusable will upset one group of people, and
>> choosing something that adds overhead that could be avoided will upset
>> another group.
>>
>> Either way, I don't think disabling 50% of the testsuite is the
>> answer. If you don't like the failures, configure the library to build
>> without threadsafe statics, or configure it to depend on libatomic, or
>> something else. (We might want new --enable-xxx switches to simplify
>> doing that).
>>
> So if we say that the current behaviour has to keep being the default,
> so that users think about what they are really doing, I can certainly
> patch my validation scripts to add a configure flag for this particular
> configuration.
>
> Is arm-none-eabi the only target having this problem?
>
> Thanks,
>
> Christophe
>

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

On 20 October 2016 at 19:40, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 20/10/16 10:33 -0700, Mike Stump wrote:
>>
>> On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>
>>>
>>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>>>
>>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>>
>>>>>
>>>>> 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.
>>>
>>>
>>> The quoting makes it look like those are my words, but I was quoting
>>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>>
>>>> Not quite in the GNU spirit?  The port people should decide the best way
>>>> to get as much functionality as possible and everything should just work, no
>>>> sharp edges.
>>>>
>>>> Forcing people to think sounds like a sharp edge?
>>>
>>>
>>> I'm inclined to agree, but we are talking about bare metal systems,
>>
>>
>> So?  gcc has been doing bare metal systems for more than 2 years now.  It
>> is pretty good at it.  All my primary targets today are themselves bare
>> metal systems (I test with newlib).
>>
>>> where there is no one-size-fits-all solution.
>>
>>
>> Configurations are like ice cream cones.  Everyone gets their flavor no
>> matter how weird or strange.  Putting nails in a cone because you don't know
>> if they like vanilla or chocolate isn't reasonable.  If you want, make two
>> flavors, and vend two, if you want to just do one, pick the flavor and vend
>> it.  Put an enum #define default_flavor vanilla, and you then have support
>> for any flavor you want.  Want to add a configure option for the flavor
>> select, add it.  You want to make a -mflavor=chocolate option, add it.  gcc
>> is literally littered with these things.
>
>
> Like I said, you can either build the library with
> -fno-threadsafe-statics or you can provide a definition of the missing
> symbol.
>
I gave this a try (using CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics).
It seems to do the trick indeed: almost all tests now pass, the flag is added
to testcase compilation.

Among the 6 remaining failures, I noticed these two:
- experimental/type_erased_allocator/2.cc: still complains about the missing
__sync_synchronize. Does it need dg-require-thread-fence?

- abi/header_cxxabi.c complains because the option is not valid for C.
I can see the test is already skipped for other C++-only options: it is OK
if I submit a patch to skip it if -fno-threadsafe-statics is used?


I think I'm going to use this flag in validations from now on (target
arm-none-eabi
only, with default mode/cpu/fpu).

Thanks,

Christophe

>> Anything vended should just work.  If it doesn't, that's a bug that needs
>> fixing.  If a port person doesn't understand, we can educate them why _it
>> just works_, is a nice design philosophy; maybe it is new to them.
>
>
> Which is basically what I'm saying. Marking 3000 tests UNSUPPORTED to
> make some test results look clean is not a fix for anything.
>
>
>>> Choosing something that makes most of the library unusable will upset one
>>> group of people, and
>>> choosing something that adds overhead that could be avoided will upset
>>> another group.
>>
>>
>> No, this is a misunderstanding.  Users want things to just work, really.
>> Bosses often like it when things just work as well; so it's not just users.
>> Customers often like it as well.  Anyway, that's my experience.
>
>
>
> OK, I'll put it another way. Under no circumstances am I going to
> accept a patch that requires adding the same redundant directive to
> every single 'do-dg run' test in libstdc++-v3/testsuite/.
>
> Right now I don't care how or if the FAILs get fixed, but it won't be
> by individually marking every file as UNSUPPORTED.
>
>

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Mike Stump]

On Oct 21, 2016, at 1:00 AM, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> 
> So if we say that the current behaviour has to keep being the default,
> so that users think about what they are really doing, 

Having a toolchain not work by default to force users to think, isn't a winning strategy.

Everything should always, just work.  Those things that don't, we should fix.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

On 14 November 2016 at 18:54, Mike Stump <mikestump@comcast.net> wrote:
> On Oct 21, 2016, at 1:00 AM, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>
>> So if we say that the current behaviour has to keep being the default,
>> so that users think about what they are really doing,
>
> Having a toolchain not work by default to force users to think, isn't a winning strategy.
>
> Everything should always, just work.  Those things that don't, we should fix.
>
I tend to agree :-)

Maybe Ramana changed his mind and would now no longer want to force
users to think?

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Mike Stump]

On Nov 14, 2016, at 12:31 PM, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote:
> 
> https://sourceware.org/ml/newlib/2015/msg00653.html

I think that patch is wrong.  It is wrong to warn on a system where an empty body is correct.  It is wrong to warn when something more than nothing needs to be done.  In short, it is never right.

If you implement what is required for any machine (configuration), at least it will be right for that configuration.  Others where that isn't correct, can then implement what is correct for their machine (configuration), if you cannot.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

On 14 November 2016 at 21:31, Ramana Radhakrishnan
<ramana.gcc@googlemail.com> wrote:
>
> On Mon, 14 Nov 2016 at 19:59, Christophe Lyon <christophe.lyon@linaro.org>
> wrote:
>>
>> On 14 November 2016 at 18:54, Mike Stump <mikestump@comcast.net> wrote:
>> > On Oct 21, 2016, at 1:00 AM, Christophe Lyon
>> > <christophe.lyon@linaro.org> wrote:
>> >>
>> >> So if we say that the current behaviour has to keep being the default,
>> >> so that users think about what they are really doing,
>> >
>> > Having a toolchain not work by default to force users to think, isn't a
>> > winning strategy.
>> >
>> > Everything should always, just work.  Those things that don't, we should
>> > fix.
>> >
>> I tend to agree :-)
>>
>> Maybe Ramana changed his mind and would now no longer want to force
>> users to think?
>
>
>
> I haven't been able to deal with this thread having being in and out of the
> office for the past month thanks to various reasons. I am not back at my
> desk until next week for various reasons and ran out of time when I was at
> my desk to get back to this and actually fix the comments in newlib patch
> review.
>
>
> https://sourceware.org/ml/newlib/2015/msg00653.html
>

Thanks for the pointer, I missed it.

> This seems to have dropped between the cracks for various reasons but that
> was the approach I was going for. Some of the points made are taken, but
> having users not think about what they want to do about synchronisation and
> just provide empty stub functions which result in random run time crashes
> aren't correct in my book. If anyone is interested in moving forward I would
> suggest they take that approach or refine it further.
>
>
> Thanks,
> Ramana
>

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 14/11/16 14:32 +0100, Christophe Lyon wrote:
>On 20 October 2016 at 19:40, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 20/10/16 10:33 -0700, Mike Stump wrote:
>>>
>>> On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>
>>>>
>>>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>>>>
>>>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>>
>>>> The quoting makes it look like those are my words, but I was quoting
>>>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>>>
>>>>> Not quite in the GNU spirit?  The port people should decide the best way
>>>>> to get as much functionality as possible and everything should just work, no
>>>>> sharp edges.
>>>>>
>>>>> Forcing people to think sounds like a sharp edge?
>>>>
>>>>
>>>> I'm inclined to agree, but we are talking about bare metal systems,
>>>
>>>
>>> So?  gcc has been doing bare metal systems for more than 2 years now.  It
>>> is pretty good at it.  All my primary targets today are themselves bare
>>> metal systems (I test with newlib).
>>>
>>>> where there is no one-size-fits-all solution.
>>>
>>>
>>> Configurations are like ice cream cones.  Everyone gets their flavor no
>>> matter how weird or strange.  Putting nails in a cone because you don't know
>>> if they like vanilla or chocolate isn't reasonable.  If you want, make two
>>> flavors, and vend two, if you want to just do one, pick the flavor and vend
>>> it.  Put an enum #define default_flavor vanilla, and you then have support
>>> for any flavor you want.  Want to add a configure option for the flavor
>>> select, add it.  You want to make a -mflavor=chocolate option, add it.  gcc
>>> is literally littered with these things.
>>
>>
>> Like I said, you can either build the library with
>> -fno-threadsafe-statics or you can provide a definition of the missing
>> symbol.
>>
>I gave this a try (using CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics).
>It seems to do the trick indeed: almost all tests now pass, the flag is added
>to testcase compilation.
>
>Among the 6 remaining failures, I noticed these two:
>- experimental/type_erased_allocator/2.cc: still complains about the missing
>__sync_synchronize. Does it need dg-require-thread-fence?

Yes, I think that test actually uses atomics directly, so does depend
on the fence.

>- abi/header_cxxabi.c complains because the option is not valid for C.
>I can see the test is already skipped for other C++-only options: it is OK
>if I submit a patch to skip it if -fno-threadsafe-statics is used?

Yes, it makes sense there too.

>I think I'm going to use this flag in validations from now on (target
>arm-none-eabi
>only, with default mode/cpu/fpu).

Thanks for the update on this.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

On 15 November 2016 at 12:50, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 14/11/16 14:32 +0100, Christophe Lyon wrote:
>>
>> On 20 October 2016 at 19:40, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>
>>> On 20/10/16 10:33 -0700, Mike Stump wrote:
>>>>
>>>>
>>>> On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>>>>>
>>>>>>
>>>>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>> The quoting makes it look like those are my words, but I was quoting
>>>>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>>>>
>>>>>> Not quite in the GNU spirit?  The port people should decide the best
>>>>>> way
>>>>>> to get as much functionality as possible and everything should just
>>>>>> work, no
>>>>>> sharp edges.
>>>>>>
>>>>>> Forcing people to think sounds like a sharp edge?
>>>>>
>>>>>
>>>>>
>>>>> I'm inclined to agree, but we are talking about bare metal systems,
>>>>
>>>>
>>>>
>>>> So?  gcc has been doing bare metal systems for more than 2 years now.
>>>> It
>>>> is pretty good at it.  All my primary targets today are themselves bare
>>>> metal systems (I test with newlib).
>>>>
>>>>> where there is no one-size-fits-all solution.
>>>>
>>>>
>>>>
>>>> Configurations are like ice cream cones.  Everyone gets their flavor no
>>>> matter how weird or strange.  Putting nails in a cone because you don't
>>>> know
>>>> if they like vanilla or chocolate isn't reasonable.  If you want, make
>>>> two
>>>> flavors, and vend two, if you want to just do one, pick the flavor and
>>>> vend
>>>> it.  Put an enum #define default_flavor vanilla, and you then have
>>>> support
>>>> for any flavor you want.  Want to add a configure option for the flavor
>>>> select, add it.  You want to make a -mflavor=chocolate option, add it.
>>>> gcc
>>>> is literally littered with these things.
>>>
>>>
>>>
>>> Like I said, you can either build the library with
>>> -fno-threadsafe-statics or you can provide a definition of the missing
>>> symbol.
>>>
>> I gave this a try (using CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics).
>> It seems to do the trick indeed: almost all tests now pass, the flag is
>> added
>> to testcase compilation.
>>
>> Among the 6 remaining failures, I noticed these two:
>> - experimental/type_erased_allocator/2.cc: still complains about the
>> missing
>> __sync_synchronize. Does it need dg-require-thread-fence?
>
>
> Yes, I think that test actually uses atomics directly, so does depend
> on the fence.
>
I've attached the patch to achieve this.
Is it OK?

>> - abi/header_cxxabi.c complains because the option is not valid for C.
>> I can see the test is already skipped for other C++-only options: it is OK
>> if I submit a patch to skip it if -fno-threadsafe-statics is used?
>
>
> Yes, it makes sense there too.

This one is not as obvious as I hoped. I tried:
-// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
"-std=gnu++??" } }
+// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
"-std=gnu++??" "-fno-threadsafe-statics" } }

but it does not work.

I set CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics
before running GCC's configure.

This results in -fno-threadsafe-statics being used when compiling the tests,
but dg-skip-if does not consider it: it would if I passed it via
runtestflags/target-board, but then it would mean passing this flag
to all tests, not only the c++ ones, leading to errors everywhere.

Am I missing something?

Thanks,

Christophe

>> I think I'm going to use this flag in validations from now on (target
>> arm-none-eabi
>> only, with default mode/cpu/fpu).
>
>
> Thanks for the update on this.
>

[-- Attachment #2: allocator-fence.log.txt --]
[-- Type: text/plain, Size: 178 bytes --]

libstdc++-v3/ChangeLog:

2016-11-16  Christophe Lyon  <christophe.lyon@linaro.org>

	* testsuite/experimental/type_erased_allocator/2.cc: Add
          dg-require-thread-fence.


[-- Attachment #3: allocator-fence.patch.txt --]
[-- Type: text/plain, Size: 460 bytes --]

diff --git a/libstdc++-v3/testsuite/experimental/type_erased_allocator/2.cc b/libstdc++-v3/testsuite/experimental/type_erased_allocator/2.cc
index 216a88c..0b73359 100644
--- a/libstdc++-v3/testsuite/experimental/type_erased_allocator/2.cc
+++ b/libstdc++-v3/testsuite/experimental/type_erased_allocator/2.cc
@@ -1,4 +1,5 @@
 // { dg-do run { target c++14 } }
+// { dg-require-thread-fence "" }
 
 // Copyright (C) 2015-2016 Free Software Foundation, Inc.
 //

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

Ping?

On 16 November 2016 at 22:18, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> On 15 November 2016 at 12:50, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 14/11/16 14:32 +0100, Christophe Lyon wrote:
>>>
>>> On 20 October 2016 at 19:40, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>
>>>> On 20/10/16 10:33 -0700, Mike Stump wrote:
>>>>>
>>>>>
>>>>> On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>> The quoting makes it look like those are my words, but I was quoting
>>>>>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>>>>>
>>>>>>> Not quite in the GNU spirit?  The port people should decide the best
>>>>>>> way
>>>>>>> to get as much functionality as possible and everything should just
>>>>>>> work, no
>>>>>>> sharp edges.
>>>>>>>
>>>>>>> Forcing people to think sounds like a sharp edge?
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'm inclined to agree, but we are talking about bare metal systems,
>>>>>
>>>>>
>>>>>
>>>>> So?  gcc has been doing bare metal systems for more than 2 years now.
>>>>> It
>>>>> is pretty good at it.  All my primary targets today are themselves bare
>>>>> metal systems (I test with newlib).
>>>>>
>>>>>> where there is no one-size-fits-all solution.
>>>>>
>>>>>
>>>>>
>>>>> Configurations are like ice cream cones.  Everyone gets their flavor no
>>>>> matter how weird or strange.  Putting nails in a cone because you don't
>>>>> know
>>>>> if they like vanilla or chocolate isn't reasonable.  If you want, make
>>>>> two
>>>>> flavors, and vend two, if you want to just do one, pick the flavor and
>>>>> vend
>>>>> it.  Put an enum #define default_flavor vanilla, and you then have
>>>>> support
>>>>> for any flavor you want.  Want to add a configure option for the flavor
>>>>> select, add it.  You want to make a -mflavor=chocolate option, add it.
>>>>> gcc
>>>>> is literally littered with these things.
>>>>
>>>>
>>>>
>>>> Like I said, you can either build the library with
>>>> -fno-threadsafe-statics or you can provide a definition of the missing
>>>> symbol.
>>>>
>>> I gave this a try (using CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics).
>>> It seems to do the trick indeed: almost all tests now pass, the flag is
>>> added
>>> to testcase compilation.
>>>
>>> Among the 6 remaining failures, I noticed these two:
>>> - experimental/type_erased_allocator/2.cc: still complains about the
>>> missing
>>> __sync_synchronize. Does it need dg-require-thread-fence?
>>
>>
>> Yes, I think that test actually uses atomics directly, so does depend
>> on the fence.
>>
> I've attached the patch to achieve this.
> Is it OK?
>
>>> - abi/header_cxxabi.c complains because the option is not valid for C.
>>> I can see the test is already skipped for other C++-only options: it is OK
>>> if I submit a patch to skip it if -fno-threadsafe-statics is used?
>>
>>
>> Yes, it makes sense there too.
>
> This one is not as obvious as I hoped. I tried:
> -// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
> "-std=gnu++??" } }
> +// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
> "-std=gnu++??" "-fno-threadsafe-statics" } }
>
> but it does not work.
>
> I set CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics
> before running GCC's configure.
>
> This results in -fno-threadsafe-statics being used when compiling the tests,
> but dg-skip-if does not consider it: it would if I passed it via
> runtestflags/target-board, but then it would mean passing this flag
> to all tests, not only the c++ ones, leading to errors everywhere.
>
> Am I missing something?
>
> Thanks,
>
> Christophe
>
>>> I think I'm going to use this flag in validations from now on (target
>>> arm-none-eabi
>>> only, with default mode/cpu/fpu).
>>
>>
>> Thanks for the update on this.
>>

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 16/11/16 22:18 +0100, Christophe Lyon wrote:
>On 15 November 2016 at 12:50, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 14/11/16 14:32 +0100, Christophe Lyon wrote:
>>>
>>> On 20 October 2016 at 19:40, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>
>>>> On 20/10/16 10:33 -0700, Mike Stump wrote:
>>>>>
>>>>>
>>>>> On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>> The quoting makes it look like those are my words, but I was quoting
>>>>>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>>>>>
>>>>>>> Not quite in the GNU spirit?  The port people should decide the best
>>>>>>> way
>>>>>>> to get as much functionality as possible and everything should just
>>>>>>> work, no
>>>>>>> sharp edges.
>>>>>>>
>>>>>>> Forcing people to think sounds like a sharp edge?
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'm inclined to agree, but we are talking about bare metal systems,
>>>>>
>>>>>
>>>>>
>>>>> So?  gcc has been doing bare metal systems for more than 2 years now.
>>>>> It
>>>>> is pretty good at it.  All my primary targets today are themselves bare
>>>>> metal systems (I test with newlib).
>>>>>
>>>>>> where there is no one-size-fits-all solution.
>>>>>
>>>>>
>>>>>
>>>>> Configurations are like ice cream cones.  Everyone gets their flavor no
>>>>> matter how weird or strange.  Putting nails in a cone because you don't
>>>>> know
>>>>> if they like vanilla or chocolate isn't reasonable.  If you want, make
>>>>> two
>>>>> flavors, and vend two, if you want to just do one, pick the flavor and
>>>>> vend
>>>>> it.  Put an enum #define default_flavor vanilla, and you then have
>>>>> support
>>>>> for any flavor you want.  Want to add a configure option for the flavor
>>>>> select, add it.  You want to make a -mflavor=chocolate option, add it.
>>>>> gcc
>>>>> is literally littered with these things.
>>>>
>>>>
>>>>
>>>> Like I said, you can either build the library with
>>>> -fno-threadsafe-statics or you can provide a definition of the missing
>>>> symbol.
>>>>
>>> I gave this a try (using CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics).
>>> It seems to do the trick indeed: almost all tests now pass, the flag is
>>> added
>>> to testcase compilation.
>>>
>>> Among the 6 remaining failures, I noticed these two:
>>> - experimental/type_erased_allocator/2.cc: still complains about the
>>> missing
>>> __sync_synchronize. Does it need dg-require-thread-fence?
>>
>>
>> Yes, I think that test actually uses atomics directly, so does depend
>> on the fence.
>>
>I've attached the patch to achieve this.
>Is it OK?

Yes, OK, thanks.

>>> - abi/header_cxxabi.c complains because the option is not valid for C.
>>> I can see the test is already skipped for other C++-only options: it is OK
>>> if I submit a patch to skip it if -fno-threadsafe-statics is used?
>>
>>
>> Yes, it makes sense there too.
>
>This one is not as obvious as I hoped. I tried:
>-// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
>"-std=gnu++??" } }
>+// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
>"-std=gnu++??" "-fno-threadsafe-statics" } }
>
>but it does not work.
>
>I set CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics
>before running GCC's configure.
>
>This results in -fno-threadsafe-statics being used when compiling the tests,
>but dg-skip-if does not consider it: it would if I passed it via
>runtestflags/target-board, but then it would mean passing this flag
>to all tests, not only the c++ ones, leading to errors everywhere.
>
>Am I missing something?

I'm not sure how to deal with that.

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Christophe Lyon]

On 29 November 2016 at 21:18, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 16/11/16 22:18 +0100, Christophe Lyon wrote:
>>
>> On 15 November 2016 at 12:50, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>
>>> On 14/11/16 14:32 +0100, Christophe Lyon wrote:
>>>>
>>>>
>>>> On 20 October 2016 at 19:40, Jonathan Wakely <jwakely@redhat.com> wrote:
>>>>>
>>>>>
>>>>> On 20/10/16 10:33 -0700, Mike Stump wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Oct 20, 2016, at 9:34 AM, Jonathan Wakely <jwakely@redhat.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 20/10/16 09:26 -0700, Mike Stump wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Oct 20, 2016, at 5:20 AM, Jonathan Wakely <jwakely@redhat.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The quoting makes it look like those are my words, but I was quoting
>>>>>>> Ramana from https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02751.html
>>>>>>>
>>>>>>>> Not quite in the GNU spirit?  The port people should decide the best
>>>>>>>> way
>>>>>>>> to get as much functionality as possible and everything should just
>>>>>>>> work, no
>>>>>>>> sharp edges.
>>>>>>>>
>>>>>>>> Forcing people to think sounds like a sharp edge?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'm inclined to agree, but we are talking about bare metal systems,
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> So?  gcc has been doing bare metal systems for more than 2 years now.
>>>>>> It
>>>>>> is pretty good at it.  All my primary targets today are themselves
>>>>>> bare
>>>>>> metal systems (I test with newlib).
>>>>>>
>>>>>>> where there is no one-size-fits-all solution.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Configurations are like ice cream cones.  Everyone gets their flavor
>>>>>> no
>>>>>> matter how weird or strange.  Putting nails in a cone because you
>>>>>> don't
>>>>>> know
>>>>>> if they like vanilla or chocolate isn't reasonable.  If you want, make
>>>>>> two
>>>>>> flavors, and vend two, if you want to just do one, pick the flavor and
>>>>>> vend
>>>>>> it.  Put an enum #define default_flavor vanilla, and you then have
>>>>>> support
>>>>>> for any flavor you want.  Want to add a configure option for the
>>>>>> flavor
>>>>>> select, add it.  You want to make a -mflavor=chocolate option, add it.
>>>>>> gcc
>>>>>> is literally littered with these things.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Like I said, you can either build the library with
>>>>> -fno-threadsafe-statics or you can provide a definition of the missing
>>>>> symbol.
>>>>>
>>>> I gave this a try (using CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics).
>>>> It seems to do the trick indeed: almost all tests now pass, the flag is
>>>> added
>>>> to testcase compilation.
>>>>
>>>> Among the 6 remaining failures, I noticed these two:
>>>> - experimental/type_erased_allocator/2.cc: still complains about the
>>>> missing
>>>> __sync_synchronize. Does it need dg-require-thread-fence?
>>>
>>>
>>>
>>> Yes, I think that test actually uses atomics directly, so does depend
>>> on the fence.
>>>
>> I've attached the patch to achieve this.
>> Is it OK?
>
>
> Yes, OK, thanks.
>
Thanks, committed.

>>>> - abi/header_cxxabi.c complains because the option is not valid for C.
>>>> I can see the test is already skipped for other C++-only options: it is
>>>> OK
>>>> if I submit a patch to skip it if -fno-threadsafe-statics is used?
>>>
>>>
>>>
>>> Yes, it makes sense there too.
>>
>>
>> This one is not as obvious as I hoped. I tried:
>> -// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
>> "-std=gnu++??" } }
>> +// { dg-skip-if "invalid options for C" { *-*-* } { "-std=c++??"
>> "-std=gnu++??" "-fno-threadsafe-statics" } }
>>
>> but it does not work.
>>
>> I set CXXFLAGS_FOR_TARGET=-fno-threadsafe-statics
>> before running GCC's configure.
>>
>> This results in -fno-threadsafe-statics being used when compiling the
>> tests,
>> but dg-skip-if does not consider it: it would if I passed it via
>> runtestflags/target-board, but then it would mean passing this flag
>> to all tests, not only the c++ ones, leading to errors everywhere.
>>
>> Am I missing something?
>
>
> I'm not sure how to deal with that.
>
I'll try to think about it, but I can live with that single "known" failure.
It's better than the ~3500 failures I used to have, and hopefully
won't report false regressions anymore.

Christophe

Re: [libstdc++, testsuite] Add dg-require-thread-fence

[Jonathan Wakely]

On 30/11/16 09:50 +0100, Christophe Lyon wrote:
>I'll try to think about it, but I can live with that single "known" failure.
>It's better than the ~3500 failures I used to have, and hopefully
>won't report false regressions anymore.

OK, cool. One rather than 3500 is a nice improvement :-)