diff mbox

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

Message ID CAKdteOb7iCVD87yRPnz=5vbUBPwkzqzUEGgx=aQPRUPaq3wZBg@mail.gmail.com
State New
Headers show

Commit Message

Christophe Lyon Nov. 16, 2016, 9:18 p.m. UTC
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.

>
libstdc++-v3/ChangeLog:

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

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

Comments

Christophe Lyon Nov. 28, 2016, 9:41 p.m. UTC | #1
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.

>>
Jonathan Wakely Nov. 29, 2016, 8:18 p.m. UTC | #2
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.
Christophe Lyon Nov. 30, 2016, 8:50 a.m. UTC | #3
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
Jonathan Wakely Nov. 30, 2016, 11:23 a.m. UTC | #4
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 :-)
diff mbox

Patch

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.
 //