Add __cow_string C string constructor
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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
Comments
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
>
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
>>
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.
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>.
@@ -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;
@@ -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