[PATCH] Add __cow_string C string constructor

[PATCH] Add __cow_string C string constructor

[François Dumont]

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

Hi

While working on the patch to use the cxx11 abi in gnu version namespace 
mode I got a small problem with this missing constructor. I'm not sure 
that the main patch will be integrated in gcc 14 so I think it is better 
if I propose this patch independently.

     libstdc++: Add __cow_string constructor from C string

     The __cow_string is instantiated from a C string in 
cow-stdexcept.cc. At the moment
     the constructor from std::string is being used with the drawback of 
an intermediate
     potential allocation/deallocation and copy. With the C string 
constructor we bypass
     all those operations.

     libstdc++-v3/ChangeLog:

             * include/std/stdexcept (__cow_string(const char*)): New 
definition.
             * src/c++11/cow-stdexcept.cc (__cow_string(const char*)): 
New definition and
             declaration.

Tested under Linux x64, ok to commit ?

François


[-- Attachment #2: cow_string_patch.txt --]
[-- Type: text/plain, Size: 1366 bytes --]

diff --git a/libstdc++-v3/include/std/stdexcept b/libstdc++-v3/include/std/stdexcept
index 66c8572d0cd..2e3c9f3bf71 100644
--- a/libstdc++-v3/include/std/stdexcept
+++ b/libstdc++-v3/include/std/stdexcept
@@ -54,6 +54,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     __cow_string();
     __cow_string(const std::string&);
+    __cow_string(const char*);
     __cow_string(const char*, size_t);
     __cow_string(const __cow_string&) _GLIBCXX_NOTHROW;
     __cow_string& operator=(const __cow_string&) _GLIBCXX_NOTHROW;
diff --git a/libstdc++-v3/src/c++11/cow-stdexcept.cc b/libstdc++-v3/src/c++11/cow-stdexcept.cc
index 8d1cc4605d4..12b189b43b5 100644
--- a/libstdc++-v3/src/c++11/cow-stdexcept.cc
+++ b/libstdc++-v3/src/c++11/cow-stdexcept.cc
@@ -127,6 +127,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     __cow_string();
     __cow_string(const std::string& s);
+    __cow_string(const char*);
     __cow_string(const char*, size_t n);
     __cow_string(const __cow_string&) noexcept;
     __cow_string& operator=(const __cow_string&) noexcept;
@@ -139,6 +140,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   __cow_string::__cow_string(const std::string& s) : _M_str(s) { }
 
+  __cow_string::__cow_string(const char* s) : _M_str(s) { }
+
   __cow_string::__cow_string(const char* s, size_t n) : _M_str(s, n) { }
 
   __cow_string::__cow_string(const __cow_string& s) noexcept

Re: [PATCH] Add __cow_string C string constructor

[Jonathan Wakely]

On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote:
>
> Hi
>
> While working on the patch to use the cxx11 abi in gnu version namespace
> mode I got a small problem with this missing constructor. I'm not sure
> that the main patch will be integrated in gcc 14 so I think it is better
> if I propose this patch independently.
>
>      libstdc++: Add __cow_string constructor from C string
>
>      The __cow_string is instantiated from a C string in
> cow-stdexcept.cc. At the moment
>      the constructor from std::string is being used with the drawback of
> an intermediate
>      potential allocation/deallocation and copy. With the C string
> constructor we bypass
>      all those operations.

But in that file, the std::string is the COW string, which means that
when we construct a std::string and copy it, it's cheap. It's just a
reference count increment/decrement. There should be no additional
allocation or deallocation.

Am I missing something?


>
>      libstdc++-v3/ChangeLog:
>
>              * include/std/stdexcept (__cow_string(const char*)): New
> definition.
>              * src/c++11/cow-stdexcept.cc (__cow_string(const char*)):
> New definition and
>              declaration.
>
> Tested under Linux x64, ok to commit ?
>
> François
>

Re: [PATCH] Add __cow_string C string constructor

[François Dumont]

On 07/01/2024 17:34, Jonathan Wakely wrote:
> On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote:
>> Hi
>>
>> While working on the patch to use the cxx11 abi in gnu version namespace
>> mode I got a small problem with this missing constructor. I'm not sure
>> that the main patch will be integrated in gcc 14 so I think it is better
>> if I propose this patch independently.
>>
>>       libstdc++: Add __cow_string constructor from C string
>>
>>       The __cow_string is instantiated from a C string in
>> cow-stdexcept.cc. At the moment
>>       the constructor from std::string is being used with the drawback of
>> an intermediate
>>       potential allocation/deallocation and copy. With the C string
>> constructor we bypass
>>       all those operations.
> But in that file, the std::string is the COW string, which means that
> when we construct a std::string and copy it, it's cheap. It's just a
> reference count increment/decrement. There should be no additional
> allocation or deallocation.

Good remark but AFAI understand in this case std::string is the cxx11 
one. I'll take a second look.

Clearly in my gnu version namespace patch it is the cxx11 implementation.

Even if so, why do we want to do those additional operations ? Adding 
this C string constructor will make sure that no useless operations will 
be done.

>
> Am I missing something?
>
>
>>       libstdc++-v3/ChangeLog:
>>
>>               * include/std/stdexcept (__cow_string(const char*)): New
>> definition.
>>               * src/c++11/cow-stdexcept.cc (__cow_string(const char*)):
>> New definition and
>>               declaration.
>>
>> Tested under Linux x64, ok to commit ?
>>
>> François
>>

Re: [PATCH] Add __cow_string C string constructor

[Jonathan Wakely]

On Sun, 7 Jan 2024 at 18:50, François Dumont <frs.dumont@gmail.com> wrote:
>
>
> On 07/01/2024 17:34, Jonathan Wakely wrote:
> > On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote:
> >> Hi
> >>
> >> While working on the patch to use the cxx11 abi in gnu version namespace
> >> mode I got a small problem with this missing constructor. I'm not sure
> >> that the main patch will be integrated in gcc 14 so I think it is better
> >> if I propose this patch independently.
> >>
> >>       libstdc++: Add __cow_string constructor from C string
> >>
> >>       The __cow_string is instantiated from a C string in
> >> cow-stdexcept.cc. At the moment
> >>       the constructor from std::string is being used with the drawback of
> >> an intermediate
> >>       potential allocation/deallocation and copy. With the C string
> >> constructor we bypass
> >>       all those operations.
> > But in that file, the std::string is the COW string, which means that
> > when we construct a std::string and copy it, it's cheap. It's just a
> > reference count increment/decrement. There should be no additional
> > allocation or deallocation.
>
> Good remark but AFAI understand in this case std::string is the cxx11
> one. I'll take a second look.
>
> Clearly in my gnu version namespace patch it is the cxx11 implementation.

I hope not! The whole point of that type is to always be a COW string,
which it does by storing a COW std::basic_string in the union, but
wrapping it in a class with a different name, __cow_string.

If your patch to use the SSO string in the versioned namespace doesn't
change that file to guarantee that __cow_string is still a
copy-on-write type then the patch is wrong and must be fixed.

>
> Even if so, why do we want to do those additional operations ? Adding
> this C string constructor will make sure that no useless operations will
> be done.

Yes, we could avoid an atomic increment and decrement, but that type
is only used when throwing an exception so the overhead of allocating
memory and calling __cxa_throw etc. is far higher than an atomic
inc/dec pair.

I was going to say that the new constructor would need to be exported
from the shared lib, but I think the new constructor is only ever used
in these two places, both defined in that same file:

  logic_error::logic_error(const char* __arg)
  : exception(), _M_msg(__arg) { }

  runtime_error::runtime_error(const char* __arg)
  : exception(), _M_msg(__arg) { }

So I think the change is safe, but I don't think it's urgent, and
certainly not needed for the reasons claimed in the patch description.

Re: [PATCH] Add __cow_string C string constructor

[François Dumont]

On 07/01/2024 21:53, Jonathan Wakely wrote:
> On Sun, 7 Jan 2024 at 18:50, François Dumont <frs.dumont@gmail.com> wrote:
>>
>> On 07/01/2024 17:34, Jonathan Wakely wrote:
>>> On Sun, 7 Jan 2024 at 12:57, François Dumont <frs.dumont@gmail.com> wrote:
>>>> Hi
>>>>
>>>> While working on the patch to use the cxx11 abi in gnu version namespace
>>>> mode I got a small problem with this missing constructor. I'm not sure
>>>> that the main patch will be integrated in gcc 14 so I think it is better
>>>> if I propose this patch independently.
>>>>
>>>>        libstdc++: Add __cow_string constructor from C string
>>>>
>>>>        The __cow_string is instantiated from a C string in
>>>> cow-stdexcept.cc. At the moment
>>>>        the constructor from std::string is being used with the drawback of
>>>> an intermediate
>>>>        potential allocation/deallocation and copy. With the C string
>>>> constructor we bypass
>>>>        all those operations.
>>> But in that file, the std::string is the COW string, which means that
>>> when we construct a std::string and copy it, it's cheap. It's just a
>>> reference count increment/decrement. There should be no additional
>>> allocation or deallocation.
>> Good remark but AFAI understand in this case std::string is the cxx11
>> one. I'll take a second look.
>>
>> Clearly in my gnu version namespace patch it is the cxx11 implementation.
> I hope not! The whole point of that type is to always be a COW string,
> which it does by storing a COW std::basic_string in the union, but
> wrapping it in a class with a different name, __cow_string.
>
> If your patch to use the SSO string in the versioned namespace doesn't
> change that file to guarantee that __cow_string is still a
> copy-on-write type then the patch is wrong and must be fixed.

Don't worry, __cow_string is indeed wrapping a COW string.

What I meant is that in this constructor in <stdexcept>:

__cow_string(const std::string&);

The std::string parameter is the SSO string.

However, as you said, in cow-stdexcept.cc the similar constructor is in 
fact taking a COW string so it has less importance. It's just a ODR issue.

In my gnu version namespace patch however this type is still the SSO 
string in cow-stdexcept.cc so I'll keep it in this context.


>> Even if so, why do we want to do those additional operations ? Adding
>> this C string constructor will make sure that no useless operations will
>> be done.
> Yes, we could avoid an atomic increment and decrement, but that type
> is only used when throwing an exception so the overhead of allocating
> memory and calling __cxa_throw etc. is far higher than an atomic
> inc/dec pair.
>
> I was going to say that the new constructor would need to be exported
> from the shared lib, but I think the new constructor is only ever used
> in these two places, both defined in that same file:
>
>    logic_error::logic_error(const char* __arg)
>    : exception(), _M_msg(__arg) { }
>
>    runtime_error::runtime_error(const char* __arg)
>    : exception(), _M_msg(__arg) { }
>
> So I think the change is safe, but I don't think it's urgent, and
> certainly not needed for the reasons claimed in the patch description.
>
The ODR violation has no side effect, it confirms your statement, looks 
like the __cow_string(const std::string&) could be removed from <stdexcept>.