| Message ID | 4ebb6936-d652-84a5-0028-6ca0a5d2d238@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix coroutine tests for libstdc++ gnu-version-namespace mode | expand |
Hi Gentle reminder for this minor patch. Thanks On 23/09/2023 22:10, François Dumont wrote: > I'm eventually fixing those tests the same way we manage this problem > in libstdc++ testsuite. > > testsuite: Add optional libstdc++ version namespace in expected > diagnostic > > When libstdc++ is build with > --enable-symvers=gnu-versioned-namespace diagnostics are > showing this namespace, currently __8. > > gcc/testsuite/ChangeLog: > > * > testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C: Add optional > '__8' version namespace in expected diagnostic. > * > testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C: Likewise. > * > testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C: Likewise. > * > testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C: > Likewise. > * testsuite/g++.dg/coroutines/pr97438.C: Likewise. > * testsuite/g++.dg/coroutines/ramp-return-b.C: Likewise. > > Tested under Linux x86_64. > > I'm contributing to libstdc++ so I already have write access. > > Ok to commit ? > > François
On Mon, 2 Oct 2023 at 18:07, François Dumont <frs.dumont@gmail.com> wrote: > > Hi > > Gentle reminder for this minor patch. It looks like you attached the wrong patch. > > Thanks > > On 23/09/2023 22:10, François Dumont wrote: > > I'm eventually fixing those tests the same way we manage this problem > > in libstdc++ testsuite. > > > > testsuite: Add optional libstdc++ version namespace in expected > > diagnostic > > > > When libstdc++ is build with > > --enable-symvers=gnu-versioned-namespace diagnostics are > > showing this namespace, currently __8. > > > > gcc/testsuite/ChangeLog: > > > > * > > testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C: Add optional > > '__8' version namespace in expected diagnostic. > > * > > testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C: Likewise. > > * > > testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C: Likewise. > > * > > testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C: > > Likewise. > > * testsuite/g++.dg/coroutines/pr97438.C: Likewise. > > * testsuite/g++.dg/coroutines/ramp-return-b.C: Likewise. > > > > Tested under Linux x86_64. > > > > I'm contributing to libstdc++ so I already have write access. > > > > Ok to commit ? > > > > François
Indeed ! Here is the right one. On 03/10/2023 11:52, Jonathan Wakely wrote: > On Mon, 2 Oct 2023 at 18:07, François Dumont <frs.dumont@gmail.com> wrote: >> Hi >> >> Gentle reminder for this minor patch. > It looks like you attached the wrong patch. > > >> Thanks >> >> On 23/09/2023 22:10, François Dumont wrote: >>> I'm eventually fixing those tests the same way we manage this problem >>> in libstdc++ testsuite. >>> >>> testsuite: Add optional libstdc++ version namespace in expected >>> diagnostic >>> >>> When libstdc++ is build with >>> --enable-symvers=gnu-versioned-namespace diagnostics are >>> showing this namespace, currently __8. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * >>> testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C: Add optional >>> '__8' version namespace in expected diagnostic. >>> * >>> testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C: Likewise. >>> * >>> testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C: Likewise. >>> * >>> testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C: >>> Likewise. >>> * testsuite/g++.dg/coroutines/pr97438.C: Likewise. >>> * testsuite/g++.dg/coroutines/ramp-return-b.C: Likewise. >>> >>> Tested under Linux x86_64. >>> >>> I'm contributing to libstdc++ so I already have write access. >>> >>> Ok to commit ? >>> >>> François
Hi François, > On 23 Sep 2023, at 21:10, François Dumont <frs.dumont@gmail.com> wrote: > > I'm eventually fixing those tests the same way we manage this problem in libstdc++ testsuite. > > testsuite: Add optional libstdc++ version namespace in expected diagnostic > > When libstdc++ is build with --enable-symvers=gnu-versioned-namespace diagnostics are > showing this namespace, currently __8. > > gcc/testsuite/ChangeLog: > > * testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C: Add optional > '__8' version namespace in expected diagnostic. > * testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C: Likewise. > * testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C: Likewise. > * testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C: Likewise. > * testsuite/g++.dg/coroutines/pr97438.C: Likewise. > * testsuite/g++.dg/coroutines/ramp-return-b.C: Likewise. > > Tested under Linux x86_64. > > I'm contributing to libstdc++ so I already have write access. > > Ok to commit ? As author of the tests, this LGTM as a suitable fix for now (at least, once the main patch to fix versioned namespaces lands). However, IMO, this could become quite painful as more g++ tests make use of std headers (which is not really optional for facilities like this that are tightly-coupled between the FE and the library). For the future, it does seem that a more complete solution might be to introduce a testsuite-wide definition for the C++ versioned std:: introducer, so that we can update it in one place as the version changes. So (as a thought experiment): - we’d have something of the form “CXX_STD” as a tcl global - we’d add the presence/absence of versioning to the relevant site.exp (which means recognising the versioning choice also in the GCC configure) - we’d migrate tests to using ${CXX_STD} instead of "std::__N” in matches … I guess an alternative could be to cook up some alternate warning/error/etc match functions that cater for arbitrary inline namespaces but that seems like a much more tricky and invasive testsuite change. thoughts? Iain
On 08/10/2023 15:59, Iain Sandoe wrote: > Hi François, > >> On 23 Sep 2023, at 21:10, François Dumont <frs.dumont@gmail.com> wrote: >> >> I'm eventually fixing those tests the same way we manage this problem in libstdc++ testsuite. >> >> testsuite: Add optional libstdc++ version namespace in expected diagnostic >> >> When libstdc++ is build with --enable-symvers=gnu-versioned-namespace diagnostics are >> showing this namespace, currently __8. >> >> gcc/testsuite/ChangeLog: >> >> * testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C: Add optional >> '__8' version namespace in expected diagnostic. >> * testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C: Likewise. >> * testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C: Likewise. >> * testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C: Likewise. >> * testsuite/g++.dg/coroutines/pr97438.C: Likewise. >> * testsuite/g++.dg/coroutines/ramp-return-b.C: Likewise. >> >> Tested under Linux x86_64. >> >> I'm contributing to libstdc++ so I already have write access. >> >> Ok to commit ? > As author of the tests, this LGTM as a suitable fix for now (at least, once the main > patch to fix versioned namespaces lands). I just realized it was a "go", no ? Then why after the main patch ? The "main patch" do not fix the versioned namespace. It just makes it adopt the cxx11 abi. This patch fixes a problem that is as old as the tests and that is totally unrelated with the main one. I just wanted to improve the situation so that versioned namespace mode do not look more buggy than necessary when someone (like you) run those. > > However, IMO, this could become quite painful as more g++ tests make use of std headers > (which is not really optional for facilities like this that are tightly-coupled between the FE and > the library). > > For the future, it does seem that a more complete solution might be to introduce a > testsuite-wide definition for the C++ versioned std:: introducer, so that we can update it in one > place as the version changes. > > So (as a thought experiment): > - we’d have something of the form “CXX_STD” as a tcl global > - we’d add the presence/absence of versioning to the relevant site.exp (which > means recognising the versioning choice also in the GCC configure) > - we’d migrate tests to using ${CXX_STD} instead of "std::__N” in matches > > … I guess an alternative could be to cook up some alternate warning/error/etc > match functions that cater for arbitrary inline namespaces but that seems like a much > more tricky and invasive testsuite change. > > thoughts? I considered amending gcc/testsuite/lib/prune.exp to simply remove the version from the diagnostic. But the reply on why it was not working scared me, so this patch. https://gcc.gnu.org/pipermail/gcc/2023-September/242526.html
Hi François, > On 11 Oct 2023, at 05:49, François Dumont <frs.dumont@gmail.com> wrote: > On 08/10/2023 15:59, Iain Sandoe wrote: >>> On 23 Sep 2023, at 21:10, François Dumont <frs.dumont@gmail.com> wrote: >>> >>> I'm eventually fixing those tests the same way we manage this problem in libstdc++ testsuite. >>> >>> testsuite: Add optional libstdc++ version namespace in expected diagnostic >>> >>> When libstdc++ is build with --enable-symvers=gnu-versioned-namespace diagnostics are >>> showing this namespace, currently __8. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C: Add optional >>> '__8' version namespace in expected diagnostic. >>> * testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C: Likewise. >>> * testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C: Likewise. >>> * testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C: Likewise. >>> * testsuite/g++.dg/coroutines/pr97438.C: Likewise. >>> * testsuite/g++.dg/coroutines/ramp-return-b.C: Likewise. >>> >>> Tested under Linux x86_64. >>> >>> I'm contributing to libstdc++ so I already have write access. >>> >>> Ok to commit ? >> As author of the tests, this LGTM as a suitable fix for now (at least, once the main >> patch to fix versioned namespaces lands). > > I just realized it was a "go", no ? Then why after the main patch ? > > The "main patch" do not fix the versioned namespace. It just makes it adopt the cxx11 abi. > > This patch fixes a problem that is as old as the tests and that is totally unrelated with the main one. I just wanted to improve the situation so that versioned namespace mode do not look more buggy than necessary when someone (like you) run those. Maybe a misunderstanding on my part. I was under the impression that versioned-namespace was currently unusable because it forces the old string ABI. If that is not the case, then I guess the changes are OK now. I am pretty concerned about the maintainability of this tho, hence this … >> However, IMO, this could become quite painful as more g++ tests make use of std headers >> (which is not really optional for facilities like this that are tightly-coupled between the FE and >> the library). >> >> For the future, it does seem that a more complete solution might be to introduce a >> testsuite-wide definition for the C++ versioned std:: introducer, so that we can update it in one >> place as the version changes. >> >> So (as a thought experiment): >> - we’d have something of the form “CXX_STD” as a tcl global >> - we’d add the presence/absence of versioning to the relevant site.exp (which >> means recognising the versioning choice also in the GCC configure) >> - we’d migrate tests to using ${CXX_STD} instead of "std::__N” in matches >> >> … I guess an alternative could be to cook up some alternate warning/error/etc >> match functions that cater for arbitrary inline namespaces but that seems like a much >> more tricky and invasive testsuite change. >> >> thoughts? > > I considered amending gcc/testsuite/lib/prune.exp to simply remove the version from the diagnostic. But the reply on why it was not working scared me, so this patch. > > https://gcc.gnu.org/pipermail/gcc/2023-September/242526.html Ah, I didn’t see that mail - will try to take a look at the weekend. Iain
Hi Iain On 11/10/2023 09:30, Iain Sandoe wrote: > Hi François, > >> On 11 Oct 2023, at 05:49, François Dumont <frs.dumont@gmail.com> wrote: >> On 08/10/2023 15:59, Iain Sandoe wrote: >>>> On 23 Sep 2023, at 21:10, François Dumont <frs.dumont@gmail.com> wrote: >>>> >>>> I'm eventually fixing those tests the same way we manage this problem in libstdc++ testsuite. >>>> >>>> testsuite: Add optional libstdc++ version namespace in expected diagnostic >>>> >>>> When libstdc++ is build with --enable-symvers=gnu-versioned-namespace diagnostics are >>>> showing this namespace, currently __8. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> * testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C: Add optional >>>> '__8' version namespace in expected diagnostic. >>>> * testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C: Likewise. >>>> * testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C: Likewise. >>>> * testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C: Likewise. >>>> * testsuite/g++.dg/coroutines/pr97438.C: Likewise. >>>> * testsuite/g++.dg/coroutines/ramp-return-b.C: Likewise. >>>> >>>> Tested under Linux x86_64. >>>> >>>> I'm contributing to libstdc++ so I already have write access. >>>> >>>> Ok to commit ? >>> As author of the tests, this LGTM as a suitable fix for now (at least, once the main >>> patch to fix versioned namespaces lands). >> I just realized it was a "go", no ? Then why after the main patch ? >> >> The "main patch" do not fix the versioned namespace. It just makes it adopt the cxx11 abi. >> >> This patch fixes a problem that is as old as the tests and that is totally unrelated with the main one. I just wanted to improve the situation so that versioned namespace mode do not look more buggy than necessary when someone (like you) run those. > Maybe a misunderstanding on my part. I was under the impression that versioned-namespace was currently unusable because it forces the old string ABI. If that is not the case, then I guess the changes are OK now. Said this way it sure makes this mode usability quite limited. It's only functional to (almost) pass make check-c++ :-) > > I am pretty concerned about the maintainability of this tho, hence this … > >>> However, IMO, this could become quite painful as more g++ tests make use of std headers >>> (which is not really optional for facilities like this that are tightly-coupled between the FE and >>> the library). >>> >>> For the future, it does seem that a more complete solution might be to introduce a >>> testsuite-wide definition for the C++ versioned std:: introducer, so that we can update it in one >>> place as the version changes. >>> >>> So (as a thought experiment): >>> - we’d have something of the form “CXX_STD” as a tcl global >>> - we’d add the presence/absence of versioning to the relevant site.exp (which >>> means recognising the versioning choice also in the GCC configure) >>> - we’d migrate tests to using ${CXX_STD} instead of "std::__N” in matches >>> >>> … I guess an alternative could be to cook up some alternate warning/error/etc >>> match functions that cater for arbitrary inline namespaces but that seems like a much >>> more tricky and invasive testsuite change. >>> >>> thoughts? >> I considered amending gcc/testsuite/lib/prune.exp to simply remove the version from the diagnostic. But the reply on why it was not working scared me, so this patch. >> >> https://gcc.gnu.org/pipermail/gcc/2023-September/242526.html > Ah, I didn’t see that mail - will try to take a look at the weekend. Ok, I'll instead chase for the patches on libstdc++ side then. Moreover adopting cxx11 abi in versioned-namespace mode will imply a version bump which would force to patch those files again if we do not find another approach before. Thanks, François
diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 index 73e7f381020..c44b71a04cb 100644 --- a/libstdc++-v3/acinclude.m4 +++ b/libstdc++-v3/acinclude.m4 @@ -4867,7 +4867,7 @@ dnl dnl Control whether the library should define symbols for old and new ABIs. dnl This affects definitions of strings, stringstreams and locale facets. dnl -dnl --disable-libstdcxx-dual-abi will use old ABI for all types. +dnl --disable-libstdcxx-dual-abi will use new ABI for all types. dnl dnl Defines: dnl _GLIBCXX_USE_DUAL_ABI (always defined, either to 1 or 0) @@ -4883,7 +4883,7 @@ AC_DEFUN([GLIBCXX_ENABLE_LIBSTDCXX_DUAL_ABI], [ else if test x"$enable_libstdcxx_dual_abi" != xyes; then AC_MSG_NOTICE([dual ABI is disabled]) - default_libstdcxx_abi="gcc4-compatible" + default_libstdcxx_abi="new" fi fi GLIBCXX_CONDITIONAL(ENABLE_DUAL_ABI, test $enable_libstdcxx_dual_abi = yes) @@ -4898,7 +4898,6 @@ dnl Defines: dnl _GLIBCXX_USE_CXX11_ABI (always defined, either to 1 or 0) dnl AC_DEFUN([GLIBCXX_DEFAULT_ABI], [ - if test x$enable_libstdcxx_dual_abi = xyes; then AC_MSG_CHECKING([for default std::string ABI to use]) AC_ARG_WITH([default-libstdcxx-abi], AS_HELP_STRING([--with-default-libstdcxx-abi], @@ -4912,7 +4911,6 @@ AC_DEFUN([GLIBCXX_DEFAULT_ABI], [ ], [default_libstdcxx_abi="new"]) AC_MSG_RESULT(${default_libstdcxx_abi}) - fi if test $default_libstdcxx_abi = "new"; then glibcxx_cxx11_abi=1 glibcxx_cxx98_abi=0 diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure index 6e9a532a359..14f9569597a 100755 --- a/libstdc++-v3/configure +++ b/libstdc++-v3/configure @@ -70712,13 +70712,12 @@ $as_echo "$as_me: dual ABI is disabled" >&6;} if test x"$enable_libstdcxx_dual_abi" != xyes; then { $as_echo "$as_me:${as_lineno-$LINENO}: dual ABI is disabled" >&5 $as_echo "$as_me: dual ABI is disabled" >&6;} - default_libstdcxx_abi="gcc4-compatible" + default_libstdcxx_abi="new" fi fi - if test x$enable_libstdcxx_dual_abi = xyes; then { $as_echo "$as_me:${as_lineno-$LINENO}: checking for default std::string ABI to use" >&5 $as_echo_n "checking for default std::string ABI to use... " >&6; } @@ -70737,7 +70736,6 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: ${default_libstdcxx_abi}" >&5 $as_echo "${default_libstdcxx_abi}" >&6; } - fi if test $default_libstdcxx_abi = "new"; then glibcxx_cxx11_abi=1 glibcxx_cxx98_abi=0