| Message ID | CAKdteObWfMso8e5R_eE=q=_ROD7nkmKphVM1=-zk_J1j+As3LA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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?
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? >
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
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.
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?
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).
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!)
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.
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.
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.
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. :)
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.
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.
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.
[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
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 >
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. > >
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.
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?
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.
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 >
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.
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
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: 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