Fix coroutine tests for libstdc++ gnu-version-namespace mode
Checks
Commit Message
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
Comments
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 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
@@ -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
@@ -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