public inbox for gcc@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
@ 2023-09-23 20:10 François Dumont
  2023-10-02 17:07 ` François Dumont
  2023-10-08 13:59 ` Iain Sandoe
  0 siblings, 2 replies; 8+ messages in thread
From: François Dumont @ 2023-09-23 20:10 UTC (permalink / raw)
  To: gcc, libstdc++; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

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

[-- Attachment #2: coroutines_tests.patch --]
[-- Type: text/x-patch, Size: 2493 bytes --]

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
  2023-09-23 20:10 [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode François Dumont
@ 2023-10-02 17:07 ` François Dumont
  2023-10-03  9:52   ` Jonathan Wakely
  2023-10-08 13:59 ` Iain Sandoe
  1 sibling, 1 reply; 8+ messages in thread
From: François Dumont @ 2023-10-02 17:07 UTC (permalink / raw)
  To: gcc, libstdc++; +Cc: gcc-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
  2023-10-02 17:07 ` François Dumont
@ 2023-10-03  9:52   ` Jonathan Wakely
  2023-10-03 20:14     ` François Dumont
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2023-10-03  9:52 UTC (permalink / raw)
  To: François Dumont; +Cc: gcc, libstdc++, gcc-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
  2023-10-03  9:52   ` Jonathan Wakely
@ 2023-10-03 20:14     ` François Dumont
  0 siblings, 0 replies; 8+ messages in thread
From: François Dumont @ 2023-10-03 20:14 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: gcc, libstdc++, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

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

[-- Attachment #2: coroutines_tests.patch --]
[-- Type: text/x-patch, Size: 4810 bytes --]

diff --git a/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C b/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C
index 4706deebf4e..928e0c974e1 100644
--- a/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C
+++ b/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-00-bad-op-new.C
@@ -6,7 +6,7 @@
 #include "coro1-allocators.h"
 
 struct coro1
-f ()  /* { dg-error {'operator new' is provided by 'std::__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\} but is not usable with the function signature 'coro1 f\(\)'} } */
+f ()  /* { dg-error {'operator new' is provided by 'std::(__8::)?__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\} but is not usable with the function signature 'coro1 f\(\)'} } */
 {
   co_return;
 }
diff --git a/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C b/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C
index 252cb5e442c..6bed524aa0a 100644
--- a/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C
+++ b/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-01-bad-op-del.C
@@ -6,7 +6,7 @@
 #include "coro1-allocators.h"
 
 struct coro1
-f ()  /* { dg-error {'operator delete' is provided by 'std::__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\} but is not usable with the function signature 'coro1 f\(\)'} } */
+f ()  /* { dg-error {'operator delete' is provided by 'std::(__8::)?__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\} but is not usable with the function signature 'coro1 f\(\)'} } */
 {
   co_return;
 }
diff --git a/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C b/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C
index 89972b60945..0a545fed0e3 100644
--- a/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C
+++ b/gcc/testsuite/g++.dg/coroutines/coro-bad-alloc-02-no-op-new-nt.C
@@ -9,7 +9,7 @@
 #include "coro1-allocators.h"
 
 struct coro1
-f () /* { dg-error {'coro1::promise_type::get_return_object_on_allocation_failure\(\)\(\)' is provided by 'std::__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\} but 'operator new' is not marked 'throw\(\)' or 'noexcept'} } */
+f () /* { dg-error {'coro1::promise_type::get_return_object_on_allocation_failure\(\)\(\)' is provided by 'std::(__8::)?__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\} but 'operator new' is not marked 'throw\(\)' or 'noexcept'} } */
 {
   co_return;
 }
diff --git a/gcc/testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C b/gcc/testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C
index 9fa3d64a9f2..b36e88f871a 100644
--- a/gcc/testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C
+++ b/gcc/testsuite/g++.dg/coroutines/coro-bad-grooaf-01-grooaf-expected.C
@@ -6,7 +6,7 @@
 int used_grooaf = 0;
 
 struct coro1
-f () noexcept // { dg-warning {'operator new' is marked 'throw\(\)' or 'noexcept' but no usable 'get_return_object_on_allocation_failure' is provided by 'std::__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\}} }
+f () noexcept // { dg-warning {'operator new' is marked 'throw\(\)' or 'noexcept' but no usable 'get_return_object_on_allocation_failure' is provided by 'std::(__8::)?__n4861::__coroutine_traits_impl<coro1, void>::promise_type' \{aka 'coro1::promise_type'\}} }
 {
   PRINT ("coro1: about to return");
   co_return;
diff --git a/gcc/testsuite/g++.dg/coroutines/pr97438.C b/gcc/testsuite/g++.dg/coroutines/pr97438.C
index 95376648ed7..ac37118eae7 100644
--- a/gcc/testsuite/g++.dg/coroutines/pr97438.C
+++ b/gcc/testsuite/g++.dg/coroutines/pr97438.C
@@ -25,6 +25,6 @@ public:
 }
 
 dummy_coroutine
-foo() { // { dg-error {the coroutine promise type 'std::__n4861::coroutine_traits<dummy_coroutine>::promise_type' declares both 'return_value' and 'return_void'} }
+foo() { // { dg-error {the coroutine promise type 'std::(__8::)?__n4861::coroutine_traits<dummy_coroutine>::promise_type' declares both 'return_value' and 'return_void'} }
     co_return 17;
 }
diff --git a/gcc/testsuite/g++.dg/coroutines/ramp-return-b.C b/gcc/testsuite/g++.dg/coroutines/ramp-return-b.C
index d0e5d1f3c7f..fcca1d73a02 100644
--- a/gcc/testsuite/g++.dg/coroutines/ramp-return-b.C
+++ b/gcc/testsuite/g++.dg/coroutines/ramp-return-b.C
@@ -19,4 +19,4 @@ task<std::vector<int>>
 baz ()
 {
   co_return std::vector<int>();
-} // { dg-error {use of deleted function 'task<T>::task\(const task<T>&\) \[with T = std::vector<int>\]'} }
+} // { dg-error {use of deleted function 'task<T>::task\(const task<T>&\) \[with T = std::(__8::)?vector<int>\]'} }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
  2023-09-23 20:10 [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode François Dumont
  2023-10-02 17:07 ` François Dumont
@ 2023-10-08 13:59 ` Iain Sandoe
  2023-10-11  4:49   ` François Dumont
  1 sibling, 1 reply; 8+ messages in thread
From: Iain Sandoe @ 2023-10-08 13:59 UTC (permalink / raw)
  To: François Dumont; +Cc: GCC Development, libstdc++, GCC Patches

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
  2023-10-08 13:59 ` Iain Sandoe
@ 2023-10-11  4:49   ` François Dumont
  2023-10-11  7:30     ` Iain Sandoe
  0 siblings, 1 reply; 8+ messages in thread
From: François Dumont @ 2023-10-11  4:49 UTC (permalink / raw)
  To: Iain Sandoe; +Cc: GCC Development, libstdc++, GCC Patches


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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
  2023-10-11  4:49   ` François Dumont
@ 2023-10-11  7:30     ` Iain Sandoe
  2023-10-11 17:22       ` François Dumont
  0 siblings, 1 reply; 8+ messages in thread
From: Iain Sandoe @ 2023-10-11  7:30 UTC (permalink / raw)
  To: François Dumont; +Cc: GCC Development, libstdc++, GCC Patches

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode
  2023-10-11  7:30     ` Iain Sandoe
@ 2023-10-11 17:22       ` François Dumont
  0 siblings, 0 replies; 8+ messages in thread
From: François Dumont @ 2023-10-11 17:22 UTC (permalink / raw)
  To: Iain Sandoe; +Cc: GCC Development, libstdc++, GCC Patches

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-10-11 17:22 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-23 20:10 [PATCH] Fix coroutine tests for libstdc++ gnu-version-namespace mode François Dumont
2023-10-02 17:07 ` François Dumont
2023-10-03  9:52   ` Jonathan Wakely
2023-10-03 20:14     ` François Dumont
2023-10-08 13:59 ` Iain Sandoe
2023-10-11  4:49   ` François Dumont
2023-10-11  7:30     ` Iain Sandoe
2023-10-11 17:22       ` François Dumont

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).