| Message ID | 54b2ad46-4e9c-1abe-07f9-7d123ff63c47@bell.net |
|---|---|
| State | New |
| Headers | show |
| Series | Fix libstdc++ tests requiring atomic support on hppa-hpux | expand |
On 09/03/19 12:46 -0500, John David Anglin wrote: >The hppa*-*-hpux* target has no builtin atomic support, so we need to explicitly link >applications requiring atomic support against libatomic. > >Okay? > >Dave >-- >John David Anglin dave.anglin@bell.net > >2019-03-09 John David Anglin <dave.anglin@bell.net> > > PR libstdc++/89461 > * testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc: Link > against libatomic on hppa*-*-hpux*. > * testsuite/20_util/shared_ptr/thread/mutex_weaktoshared.cc: Likewise. > * testsuite/experimental/net/timer/waitable/cons.cc: Likewise. > * testsuite/experimental/net/timer/waitable/dest.cc: Likewise. > * testsuite/experimental/net/timer/waitable/ops.cc: Likewise. > * testsuite/lib/libstdc++.exp: Locate libatomic. What do you think about adding the following? --- a/libstdc++-v3/testsuite/lib/dg-options.exp +++ b/libstdc++-v3/testsuite/lib/dg-options.exp @@ -257,6 +257,15 @@ proc add_options_for_net_ts { flags } { return $flags } +# Add to FLAGS all the target-specific flags to link to libatomic, if required. + +proc add_options_for_libatomic { flags } { + if { [istarget hppa*-*-hpux*] } { + return "$flags -L../../libatomic/.libs -latomic" + } + return $flags +} + # Like dg-options, but adds to the default options rather than replacing them. proc dg-additional-options { args } { > >Index: testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc >=================================================================== >--- testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc (revision 269442) >+++ testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc (working copy) >@@ -22,6 +22,7 @@ > // { dg-require-effective-target c++11 } > // { dg-require-effective-target pthread } > // { dg-require-cstdint "" } >+// { dg-additional-options "-L../../libatomic/.libs -latomic" { target hppa*-*-hpux* } } And then this would be: --- a/libstdc++-v3/testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc +++ b/libstdc++-v3/testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc @@ -19,6 +19,7 @@ // { dg-do run } // { dg-options "-pthread" } +// { dg-add-options libatomic } // { dg-require-effective-target c++11 } // { dg-require-effective-target pthread } // { dg-require-cstdint "" }
On Mär 11 2019, Jonathan Wakely <jwakely@redhat.com> wrote: > What do you think about adding the following? > > --- a/libstdc++-v3/testsuite/lib/dg-options.exp > +++ b/libstdc++-v3/testsuite/lib/dg-options.exp > @@ -257,6 +257,15 @@ proc add_options_for_net_ts { flags } { > return $flags > } > > +# Add to FLAGS all the target-specific flags to link to libatomic, if required. > + > +proc add_options_for_libatomic { flags } { > + if { [istarget hppa*-*-hpux*] } { > + return "$flags -L../../libatomic/.libs -latomic" > + } > + return $flags > +} That should probably also be enabled for riscv*-*-*. Andreas.
On 11/03/19 14:13 +0100, Andreas Schwab wrote: >On Mär 11 2019, Jonathan Wakely <jwakely@redhat.com> wrote: > >> What do you think about adding the following? >> >> --- a/libstdc++-v3/testsuite/lib/dg-options.exp >> +++ b/libstdc++-v3/testsuite/lib/dg-options.exp >> @@ -257,6 +257,15 @@ proc add_options_for_net_ts { flags } { >> return $flags >> } >> >> +# Add to FLAGS all the target-specific flags to link to libatomic, if required. >> + >> +proc add_options_for_libatomic { flags } { >> + if { [istarget hppa*-*-hpux*] } { >> + return "$flags -L../../libatomic/.libs -latomic" >> + } >> + return $flags >> +} > >That should probably also be enabled for riscv*-*-*. So all the more reason to do it this way, so the list of targets is only maintained in one place.
On 2019-03-11 9:16 a.m., Jonathan Wakely wrote: > On 11/03/19 14:13 +0100, Andreas Schwab wrote: >> On Mär 11 2019, Jonathan Wakely <jwakely@redhat.com> wrote: >> >>> What do you think about adding the following? >>> >>> --- a/libstdc++-v3/testsuite/lib/dg-options.exp >>> +++ b/libstdc++-v3/testsuite/lib/dg-options.exp >>> @@ -257,6 +257,15 @@ proc add_options_for_net_ts { flags } { >>> return $flags >>> } >>> >>> +# Add to FLAGS all the target-specific flags to link to libatomic, if required. >>> + >>> +proc add_options_for_libatomic { flags } { >>> + if { [istarget hppa*-*-hpux*] } { >>> + return "$flags -L../../libatomic/.libs -latomic" >>> + } >>> + return $flags >>> +} >> >> That should probably also be enabled for riscv*-*-*. > > So all the more reason to do it this way, so the list of targets is > only maintained in one place Your suggestion looks good to me. We still need hunk that sets up LD_LIBRARY_PATH. This is what is done in fortran tests: ! { dg-additional-options "-latomic" { target libatomic_available } } Dave
On 11/03/19 09:27 -0400, John David Anglin wrote: >On 2019-03-11 9:16 a.m., Jonathan Wakely wrote: >> On 11/03/19 14:13 +0100, Andreas Schwab wrote: >>> On Mär 11 2019, Jonathan Wakely <jwakely@redhat.com> wrote: >>> >>>> What do you think about adding the following? >>>> >>>> --- a/libstdc++-v3/testsuite/lib/dg-options.exp >>>> +++ b/libstdc++-v3/testsuite/lib/dg-options.exp >>>> @@ -257,6 +257,15 @@ proc add_options_for_net_ts { flags } { >>>> return $flags >>>> } >>>> >>>> +# Add to FLAGS all the target-specific flags to link to libatomic, if required. >>>> + >>>> +proc add_options_for_libatomic { flags } { >>>> + if { [istarget hppa*-*-hpux*] } { >>>> + return "$flags -L../../libatomic/.libs -latomic" >>>> + } >>>> + return $flags >>>> +} >>> >>> That should probably also be enabled for riscv*-*-*. >> >> So all the more reason to do it this way, so the list of targets is >> only maintained in one place >Your suggestion looks good to me. We still need hunk that sets up LD_LIBRARY_PATH. Right. >This is what is done in fortran tests: >! { dg-additional-options "-latomic" { target libatomic_available } } That adds it for every target that has a libatomic though, right? We have tests that check certain uses of atomics work without libatomic, which would no longer check anything if we added that to them :-) If we use { dg-add-options libatomic } it'll only be added for the listed targets (hpux and riscv to begin with) and if somebody disables building libatomic on those targets they probably should get test failures to say there's something wrong.
On 11/03/19 13:54 +0000, Jonathan Wakely wrote: >On 11/03/19 09:27 -0400, John David Anglin wrote: >>On 2019-03-11 9:16 a.m., Jonathan Wakely wrote: >>>On 11/03/19 14:13 +0100, Andreas Schwab wrote: >>>>On Mär 11 2019, Jonathan Wakely <jwakely@redhat.com> wrote: >>>> >>>>>What do you think about adding the following? >>>>> >>>>>--- a/libstdc++-v3/testsuite/lib/dg-options.exp >>>>>+++ b/libstdc++-v3/testsuite/lib/dg-options.exp >>>>>@@ -257,6 +257,15 @@ proc add_options_for_net_ts { flags } { >>>>> return $flags >>>>>} >>>>> >>>>>+# Add to FLAGS all the target-specific flags to link to libatomic, if required. >>>>>+ >>>>>+proc add_options_for_libatomic { flags } { >>>>>+ if { [istarget hppa*-*-hpux*] } { >>>>>+ return "$flags -L../../libatomic/.libs -latomic" >>>>>+ } >>>>>+ return $flags >>>>>+} >>>> >>>>That should probably also be enabled for riscv*-*-*. >>> >>>So all the more reason to do it this way, so the list of targets is >>>only maintained in one place >>Your suggestion looks good to me. We still need hunk that sets up LD_LIBRARY_PATH. > >Right. > >>This is what is done in fortran tests: >>! { dg-additional-options "-latomic" { target libatomic_available } } > >That adds it for every target that has a libatomic though, right? We >have tests that check certain uses of atomics work without libatomic, >which would no longer check anything if we added that to them :-) Ah, but unless -L../../libatomic./libs has been added to the flags, -latomic won't find anything, so it wouldn't get added. I still think {target libatomic_available } doesn't buy us much, due to ... >If we use { dg-add-options libatomic } it'll only be added for the >listed targets (hpux and riscv to begin with) and if somebody disables >building libatomic on those targets they probably should get test >failures to say there's something wrong. >
On Mon, 11 Mar 2019, Jonathan Wakely wrote: > +proc add_options_for_libatomic { flags } { > + if { [istarget hppa*-*-hpux*] } { > + return "$flags -L../../libatomic/.libs -latomic" > + } It's generally inappropriate to hardcode such ../../libatomic/.libs paths without making sure it's for build-tree rather than installed testing (for installed testing, -latomic can be found without any -L option and it's the board file's responsibility to set LD_LIBRARY_PATH if necessary).
On 12/03/19 23:01 +0000, Joseph Myers wrote: >On Mon, 11 Mar 2019, Jonathan Wakely wrote: > >> +proc add_options_for_libatomic { flags } { >> + if { [istarget hppa*-*-hpux*] } { >> + return "$flags -L../../libatomic/.libs -latomic" >> + } > >It's generally inappropriate to hardcode such ../../libatomic/.libs paths >without making sure it's for build-tree rather than installed testing (for >installed testing, -latomic can be found without any -L option and it's >the board file's responsibility to set LD_LIBRARY_PATH if necessary). I was going to say something about that, but then saw we already do it for libgomp/.libs as well. Two wrongs now though ...
On 2019-03-12 7:05 p.m., Jonathan Wakely wrote: > On 12/03/19 23:01 +0000, Joseph Myers wrote: >> On Mon, 11 Mar 2019, Jonathan Wakely wrote: >> >>> +proc add_options_for_libatomic { flags } { >>> + if { [istarget hppa*-*-hpux*] } { >>> + return "$flags -L../../libatomic/.libs -latomic" >>> + } option. >> >> It's generally inappropriate to hardcode such ../../libatomic/.libs paths >> without making sure it's for build-tree rather than installed testing (for >> installed testing, -latomic can be found without any -L option and it's >> the board file's responsibility to set LD_LIBRARY_PATH if necessary). > > I was going to say something about that, but then saw we already do it > for libgomp/.libs as well. Two wrongs now though .. I just installed the change before I saw Joseph's comment. I don't believe -latomic can be found without a -L option in the libstdc++ testsuite. That's why it's done for libgomp. However, it's not needed for c, c++, fortran testsuites. They also setup LD_LIBRARY_PATH for testing in build tree. I've never had to use a board file to get this right.
Regarding this sometimes-add--latomic(-in-testsuite) that is revisited: When is it appropriate to make the user add -latomic to link their program? Perhaps different answers for fortran and C++. I'm guessing "always when using any atomic construct" for C. I had a grep-look in gcc/doc before asking; I can't see we say anything. I don't think "when the target lacks certain atomic features" is a full valid answer, except pragmatically, to paper over a bug. brgds, H-P PS. I'll do the patch if you (someone) does the words.
On Tue, 12 Mar 2019, Hans-Peter Nilsson wrote: > When is it appropriate to make the user add -latomic to link > their program? Perhaps different answers for fortran and C++. > I'm guessing "always when using any atomic construct" for C. I think we should link with --as-needed -latomic --no-as-needed by default when --as-needed is supported (see bug 81358).
Index: testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc =================================================================== --- testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc (revision 269442) +++ testsuite/20_util/shared_ptr/thread/default_weaktoshared.cc (working copy) @@ -22,6 +22,7 @@ // { dg-require-effective-target c++11 } // { dg-require-effective-target pthread } // { dg-require-cstdint "" } +// { dg-additional-options "-L../../libatomic/.libs -latomic" { target hppa*-*-hpux* } } #include <memory> #include <random> Index: testsuite/20_util/shared_ptr/thread/mutex_weaktoshared.cc =================================================================== --- testsuite/20_util/shared_ptr/thread/mutex_weaktoshared.cc (revision 269442) +++ testsuite/20_util/shared_ptr/thread/mutex_weaktoshared.cc (working copy) @@ -22,6 +22,7 @@ // { dg-require-effective-target c++11 } // { dg-require-effective-target pthread } // { dg-require-cstdint "" } +// { dg-additional-options "-L../../libatomic/.libs -latomic" { target hppa*-*-hpux* } } #include <memory> #include <random> Index: testsuite/experimental/net/timer/waitable/cons.cc =================================================================== --- testsuite/experimental/net/timer/waitable/cons.cc (revision 269442) +++ testsuite/experimental/net/timer/waitable/cons.cc (working copy) @@ -16,6 +16,7 @@ // <http://www.gnu.org/licenses/>. // { dg-do run { target c++14 } } +// { dg-additional-options "-L../../libatomic/.libs -latomic" { target hppa*-*-hpux* } } #include <experimental/timer> #include <testsuite_hooks.h> Index: testsuite/experimental/net/timer/waitable/dest.cc =================================================================== --- testsuite/experimental/net/timer/waitable/dest.cc (revision 269442) +++ testsuite/experimental/net/timer/waitable/dest.cc (working copy) @@ -16,6 +16,7 @@ // <http://www.gnu.org/licenses/>. // { dg-do run { target c++14 } } +// { dg-additional-options "-L../../libatomic/.libs -latomic" { target hppa*-*-hpux* } } #include <experimental/timer> #include <testsuite_hooks.h> Index: testsuite/experimental/net/timer/waitable/ops.cc =================================================================== --- testsuite/experimental/net/timer/waitable/ops.cc (revision 269442) +++ testsuite/experimental/net/timer/waitable/ops.cc (working copy) @@ -16,6 +16,7 @@ // <http://www.gnu.org/licenses/>. // { dg-do run { target c++14 } } +// { dg-additional-options "-L../../libatomic/.libs -latomic" { target hppa*-*-hpux* } } #include <experimental/timer> #include <testsuite_hooks.h> Index: testsuite/lib/libstdc++.exp =================================================================== --- testsuite/lib/libstdc++.exp (revision 269442) +++ testsuite/lib/libstdc++.exp (working copy) @@ -161,6 +161,17 @@ } v3track gccdir 3 + # Locate libatomic. + set v3-libatomic 0 + set libatomicdir [lookfor_file $blddir/../libatomic .libs/libatomic.$shlib_ext] + if {$libatomicdir != ""} { + set v3-libatomic 1 + set libatomicdir [file dirname $libatomicdir] + append ld_library_path_tmp ":${libatomicdir}" + verbose -log "libatomic support detected" + } + v3track libatomicdir 3 + # Locate libgomp. This is only required for parallel mode. set v3-libgomp 0 set libgompdir [lookfor_file $blddir/../libgomp .libs/libgomp.$shlib_ext]