public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Do not take address of empty string front
@ 2015-06-20 10:47 François Dumont
  2015-06-20 20:38 ` Jonathan Wakely
  0 siblings, 1 reply; 7+ messages in thread
From: François Dumont @ 2015-06-20 10:47 UTC (permalink / raw)
  To: libstdc++, gcc-patches

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

Hi

    2 experimental tests are failing in debug mode because
__do_str_codecvt is sometimes taking address of string front() and
back() even if empty. It wasn't use so not a big issue but it still
seems better to avoid. I propose to rather use string begin() to get
buffer address. I kept operator& as it is what was used, do you think we
should use addressof ?

    At the same time I improve string debug mode cause with front()
implemented as operator[](0) it is ok even if string is empty while
back() implemented as operator[](size()-1) is not which is quite
inconsistent. So I added a number of !empty() checks in several methods
to detect more easily all those invalid usages.

    * include/bits/basic_string.h (basic_string<>::front()): Add !empty
    debug check.
    (basic_string<>::back()): Likewise.
    (basic_string<>::pop_back()): Likewise.
    * include/bits/locale_env.h (__do_std_codecvt): Do not take address of
    string::front() when string is empty.
    * include/debug/macros.h
    (__glibcxx_check_string, __glibcxx_check_string_len): Implement using
    _GLIBCXX_DEBUG_PEDASSERT.

Tested under Linux x86_64 debug mode.

Ok to commit ?

François


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

diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 093f502..923fb83 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -39,6 +39,7 @@
 #include <ext/atomicity.h>
 #include <ext/alloc_traits.h>
 #include <debug/debug.h>
+
 #if __cplusplus >= 201103L
 #include <initializer_list>
 #endif
@@ -903,7 +904,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
        */
       reference
       front() noexcept
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the first
@@ -911,7 +915,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
        */
       const_reference
       front() const noexcept
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read/write reference to the data at the last
@@ -919,7 +926,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
        */
       reference
       back() noexcept
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the
@@ -927,7 +937,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
        */
       const_reference
       back() const noexcept
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 #endif
 
       // Modifiers:
@@ -1506,7 +1519,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
        */
       void
       pop_back() noexcept
-      { _M_erase(size()-1, 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	_M_erase(size() - 1, 1);
+      }
 #endif // C++11
 
       /**
@@ -3308,7 +3324,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
        */
       reference
       front()
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the first
@@ -3316,7 +3335,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
        */
       const_reference
       front() const _GLIBCXX_NOEXCEPT
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read/write reference to the data at the last
@@ -3324,7 +3346,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
        */
       reference
       back()
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the
@@ -3332,7 +3357,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
        */
       const_reference
       back() const _GLIBCXX_NOEXCEPT
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 #endif
 
       // Modifiers:
@@ -3819,7 +3847,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
        */
       void
       pop_back() // FIXME C++11: should be noexcept.
-      { erase(size()-1, 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	erase(size() - 1, 1);
+      }
 #endif // C++11
 
       /**
diff --git a/libstdc++-v3/include/bits/locale_conv.h b/libstdc++-v3/include/bits/locale_conv.h
index 61b535c..8def30d 100644
--- a/libstdc++-v3/include/bits/locale_conv.h
+++ b/libstdc++-v3/include/bits/locale_conv.h
@@ -66,10 +66,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       do
 	{
 	  __outstr.resize(__outstr.size() + (__last - __next) * __maxlen);
-	  auto __outnext = &__outstr.front() + __outchars;
-	  auto const __outlast = &__outstr.back() + 1;
+	  auto __outnext = &(*__outstr.begin()) + __outchars;
+	  auto const __outlast = &(*__outstr.begin()) + __outstr.size();
 	  __result = (__cvt.*__fn)(__state, __next, __last, __next,
-					__outnext, __outlast, __outnext);
+				   __outnext, __outlast, __outnext);
 	  __outchars = __outnext - &__outstr.front();
 	}
       while (__result == codecvt_base::partial && __next != __last
diff --git a/libstdc++-v3/include/debug/macros.h b/libstdc++-v3/include/debug/macros.h
index 5136c4b..39cb0d6 100644
--- a/libstdc++-v3/include/debug/macros.h
+++ b/libstdc++-v3/include/debug/macros.h
@@ -358,14 +358,9 @@ _GLIBCXX_DEBUG_VERIFY(_This.get_allocator() == _Other.get_allocator(),	\
 		      _M_message(__gnu_debug::__msg_equal_allocs)	\
 		      ._M_sequence(_This, "this"))
 
-#ifdef _GLIBCXX_DEBUG_PEDANTIC
-#  define __glibcxx_check_string(_String) _GLIBCXX_DEBUG_ASSERT(_String != 0)
-#  define __glibcxx_check_string_len(_String,_Len) \
-       _GLIBCXX_DEBUG_ASSERT(_String != 0 || _Len == 0)
-#else
-#  define __glibcxx_check_string(_String)
-#  define __glibcxx_check_string_len(_String,_Len)
-#endif
+#define __glibcxx_check_string(_String) _GLIBCXX_DEBUG_PEDASSERT(_String != 0)
+#define __glibcxx_check_string_len(_String,_Len) \
+  _GLIBCXX_DEBUG_PEDASSERT(_String != 0 || _Len == 0)
 
 // Verify that a predicate is irreflexive
 #define __glibcxx_check_irreflexive(_First,_Last)			\

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

* Re: Do not take address of empty string front
  2015-06-20 10:47 Do not take address of empty string front François Dumont
@ 2015-06-20 20:38 ` Jonathan Wakely
  2015-06-22 15:11   ` Jonathan Wakely
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2015-06-20 20:38 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

On 20/06/15 12:03 +0200, François Dumont wrote:
>Hi
>
>    2 experimental tests are failing in debug mode because
>__do_str_codecvt is sometimes taking address of string front() and
>back() even if empty. It wasn't use so not a big issue but it still
>seems better to avoid. I propose to rather use string begin() to get
>buffer address.

But derefencing begin() is still undefined for an empty string.
Shouldn't that fail for debug mode too? Why change one form of
undefined behaviour that we diagnose to another form that we don't
diagnose?

It would be better if that function didn't do any work when the input
range is empty:

--- a/libstdc++-v3/include/bits/locale_conv.h
+++ b/libstdc++-v3/include/bits/locale_conv.h
@@ -58,6 +58,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
                     _OutStr& __outstr, const _Codecvt& __cvt, _State& __state,
                     size_t& __count, _Fn __fn)
     {
+      if (__first == __last)
+       {
+         __outstr.clear();
+         return true;
+       }
+
       size_t __outchars = 0;
       auto __next = __first;
       const auto __maxlen = __cvt.max_length() + 1;


>I kept operator& as it is what was used, do you think we
>should use addressof ?

I don't expect that function to get used with anything except built-in
character types (char, wchar_t, char16_t, char32_t) which don't use
ADL so can't find an overloaded operator&.

>    At the same time I improve string debug mode cause with front()
>implemented as operator[](0) it is ok even if string is empty while
>back() implemented as operator[](size()-1) is not which is quite
>inconsistent. So I added a number of !empty() checks in several methods
>to detect more easily all those invalid usages.

Nice. These would all be candidates for the "debug mode lite" that I
want.

>    * include/bits/basic_string.h (basic_string<>::front()): Add !empty
>    debug check.
>    (basic_string<>::back()): Likewise.
>    (basic_string<>::pop_back()): Likewise.
>    * include/bits/locale_env.h (__do_std_codecvt): Do not take address of

N.B. include/bits/locale_conv.h not locale_env.h

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

* Re: Do not take address of empty string front
  2015-06-20 20:38 ` Jonathan Wakely
@ 2015-06-22 15:11   ` Jonathan Wakely
  2015-06-23 20:09     ` François Dumont
                       ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Jonathan Wakely @ 2015-06-22 15:11 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

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

On 20/06/15 12:59 +0100, Jonathan Wakely wrote:
>On 20/06/15 12:03 +0200, François Dumont wrote:
>>Hi
>>
>>   2 experimental tests are failing in debug mode because
>>__do_str_codecvt is sometimes taking address of string front() and
>>back() even if empty. It wasn't use so not a big issue but it still
>>seems better to avoid. I propose to rather use string begin() to get
>>buffer address.
>
>But derefencing begin() is still undefined for an empty string.
>Shouldn't that fail for debug mode too? Why change one form of
>undefined behaviour that we diagnose to another form that we don't
>diagnose?
>
>It would be better if that function didn't do any work when the input
>range is empty:
>
>--- a/libstdc++-v3/include/bits/locale_conv.h
>+++ b/libstdc++-v3/include/bits/locale_conv.h
>@@ -58,6 +58,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>                    _OutStr& __outstr, const _Codecvt& __cvt, _State& __state,
>                    size_t& __count, _Fn __fn)
>    {
>+      if (__first == __last)
>+       {
>+         __outstr.clear();
>+         return true;
>+       }
>+
>      size_t __outchars = 0;
>      auto __next = __first;
>      const auto __maxlen = __cvt.max_length() + 1;

This makes that change, and also moves wstring_convert into the
ABI-tagged __cxx11 namespace, and fixes a copy&paste error in the
exception thrown from wbuffer_convert.

Tested powerpc64le-linux, committed to trunk.

François, your changes to add extra checks in std::string are still
useful separately.


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

commit 4ab3f0a76f7e18074c91c4644cbfdf23084e93ba
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Jun 22 13:47:24 2015 +0100

    	* include/bits/locale_conv.h (__do_str_codecvt): Handle empty range.
    	(wstring_convert): Move into __cxx11 namespace.
    	(wbuffer_convert(streambuf*, _Codecvt*, state_type)): Fix exception
    	message.

diff --git a/libstdc++-v3/include/bits/locale_conv.h b/libstdc++-v3/include/bits/locale_conv.h
index 61b535c..fd99499 100644
--- a/libstdc++-v3/include/bits/locale_conv.h
+++ b/libstdc++-v3/include/bits/locale_conv.h
@@ -58,6 +58,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		     _OutStr& __outstr, const _Codecvt& __cvt, _State& __state,
 		     size_t& __count, _Fn __fn)
     {
+      if (__first == __last)
+	{
+	  __outstr.clear();
+	  return true;
+	}
+
       size_t __outchars = 0;
       auto __next = __first;
       const auto __maxlen = __cvt.max_length() + 1;
@@ -150,6 +156,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return __str_codecvt_out(__first, __last, __outstr, __cvt, __state, __n);
     }
 
+_GLIBCXX_BEGIN_NAMESPACE_CXX11
+
   /// String conversions
   template<typename _Codecvt, typename _Elem = wchar_t,
 	   typename _Wide_alloc = allocator<_Elem>,
@@ -301,6 +309,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool			_M_with_strings = false;
     };
 
+_GLIBCXX_END_NAMESPACE_CXX11
+
   /// Buffer conversions
   template<typename _Codecvt, typename _Elem = wchar_t,
 	   typename _Tr = char_traits<_Elem>>
@@ -325,7 +335,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_buf(__bytebuf), _M_cvt(__pcvt), _M_state(__state)
       {
 	if (!_M_cvt)
-	  __throw_logic_error("wstring_convert");
+	  __throw_logic_error("wbuffer_convert");
 
 	_M_always_noconv = _M_cvt->always_noconv();
 

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

* Re: Do not take address of empty string front
  2015-06-22 15:11   ` Jonathan Wakely
@ 2015-06-23 20:09     ` François Dumont
  2015-06-25 18:27       ` Jonathan Wakely
  2015-06-24 20:17     ` François Dumont
  2015-07-02 22:54     ` Jonathan Wakely
  2 siblings, 1 reply; 7+ messages in thread
From: François Dumont @ 2015-06-23 20:09 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

On 22/06/2015 17:10, Jonathan Wakely wrote:
> On 20/06/15 12:59 +0100, Jonathan Wakely wrote:
>> On 20/06/15 12:03 +0200, François Dumont wrote:
>>> Hi
>>>
>>>   2 experimental tests are failing in debug mode because
>>> __do_str_codecvt is sometimes taking address of string front() and
>>> back() even if empty. It wasn't use so not a big issue but it still
>>> seems better to avoid. I propose to rather use string begin() to get
>>> buffer address.
>>
>> But derefencing begin() is still undefined for an empty string.
>> Shouldn't that fail for debug mode too? Why change one form of
>> undefined behaviour that we diagnose to another form that we don't
>> diagnose?
>>
>> It would be better if that function didn't do any work when the input
>> range is empty:
>>
>> --- a/libstdc++-v3/include/bits/locale_conv.h
>> +++ b/libstdc++-v3/include/bits/locale_conv.h
>> @@ -58,6 +58,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>                    _OutStr& __outstr, const _Codecvt& __cvt, _State&
>> __state,
>>                    size_t& __count, _Fn __fn)
>>    {
>> +      if (__first == __last)
>> +       {
>> +         __outstr.clear();
>> +         return true;
>> +       }
>> +

Shouldn't it also set __count to 0 to make clear that nothing has been
converted.

>>      size_t __outchars = 0;
>>      auto __next = __first;
>>      const auto __maxlen = __cvt.max_length() + 1;
>
> This makes that change, and also moves wstring_convert into the
> ABI-tagged __cxx11 namespace, and fixes a copy&paste error in the
> exception thrown from wbuffer_convert.
>
> Tested powerpc64le-linux, committed to trunk.
>
> François, your changes to add extra checks in std::string are still
> useful separately.
>
Ok I will take care of this part.

François

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

* Re: Do not take address of empty string front
  2015-06-22 15:11   ` Jonathan Wakely
  2015-06-23 20:09     ` François Dumont
@ 2015-06-24 20:17     ` François Dumont
  2015-07-02 22:54     ` Jonathan Wakely
  2 siblings, 0 replies; 7+ messages in thread
From: François Dumont @ 2015-06-24 20:17 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

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

On 22/06/2015 17:10, Jonathan Wakely wrote:
> On 20/06/15 12:59 +0100, Jonathan Wakely wrote:
>> On 20/06/15 12:03 +0200, François Dumont wrote:
>>> Hi
>>>
>>>   2 experimental tests are failing in debug mode because
>>> __do_str_codecvt is sometimes taking address of string front() and
>>> back() even if empty. It wasn't use so not a big issue but it still
>>> seems better to avoid. I propose to rather use string begin() to get
>>> buffer address.
>>
>> But derefencing begin() is still undefined for an empty string.
>> Shouldn't that fail for debug mode too?
It would if we were using basic_string debug implementation but we
aren't. We are just using normal implementation with some debug checks
which is not enough to detect invalid deference operations.
>> Why change one form of
>> undefined behaviour that we diagnose to another form that we don't
>> diagnose?
>>
>> It would be better if that function didn't do any work when the input
>> range is empty:
>>
>> --- a/libstdc++-v3/include/bits/locale_conv.h
>> +++ b/libstdc++-v3/include/bits/locale_conv.h
>> @@ -58,6 +58,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>                    _OutStr& __outstr, const _Codecvt& __cvt, _State&
>> __state,
>>                    size_t& __count, _Fn __fn)
>>    {
>> +      if (__first == __last)
>> +       {
>> +         __outstr.clear();
>> +         return true;
>> +       }
>> +
>>      size_t __outchars = 0;
>>      auto __next = __first;
>>      const auto __maxlen = __cvt.max_length() + 1;
>
> This makes that change, and also moves wstring_convert into the
> ABI-tagged __cxx11 namespace, and fixes a copy&paste error in the
> exception thrown from wbuffer_convert.
>
> Tested powerpc64le-linux, committed to trunk.
>
> François, your changes to add extra checks in std::string are still
> useful separately.
>
I just applied attached patch then.

François

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

Index: include/bits/basic_string.h
===================================================================
--- include/bits/basic_string.h	(revision 224914)
+++ include/bits/basic_string.h	(working copy)
@@ -39,6 +39,7 @@
 #include <ext/atomicity.h>
 #include <ext/alloc_traits.h>
 #include <debug/debug.h>
+
 #if __cplusplus >= 201103L
 #include <initializer_list>
 #endif
@@ -903,7 +904,10 @@
        */
       reference
       front() noexcept
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the first
@@ -911,7 +915,10 @@
        */
       const_reference
       front() const noexcept
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read/write reference to the data at the last
@@ -919,7 +926,10 @@
        */
       reference
       back() noexcept
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the
@@ -927,7 +937,10 @@
        */
       const_reference
       back() const noexcept
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 #endif
 
       // Modifiers:
@@ -1506,7 +1519,10 @@
        */
       void
       pop_back() noexcept
-      { _M_erase(size()-1, 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	_M_erase(size() - 1, 1);
+      }
 #endif // C++11
 
       /**
@@ -3308,7 +3324,10 @@
        */
       reference
       front()
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the first
@@ -3316,7 +3335,10 @@
        */
       const_reference
       front() const _GLIBCXX_NOEXCEPT
-      { return operator[](0); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](0);
+      }
 
       /**
        *  Returns a read/write reference to the data at the last
@@ -3324,7 +3346,10 @@
        */
       reference
       back()
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 
       /**
        *  Returns a read-only (constant) reference to the data at the
@@ -3332,7 +3357,10 @@
        */
       const_reference
       back() const _GLIBCXX_NOEXCEPT
-      { return operator[](this->size() - 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	return operator[](this->size() - 1);
+      }
 #endif
 
       // Modifiers:
@@ -3819,7 +3847,10 @@
        */
       void
       pop_back() // FIXME C++11: should be noexcept.
-      { erase(size()-1, 1); }
+      {
+	_GLIBCXX_DEBUG_ASSERT(!empty());
+	erase(size() - 1, 1);
+      }
 #endif // C++11
 
       /**

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

* Re: Do not take address of empty string front
  2015-06-23 20:09     ` François Dumont
@ 2015-06-25 18:27       ` Jonathan Wakely
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Wakely @ 2015-06-25 18:27 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

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

On 23/06/15 22:04 +0200, François Dumont wrote:
>On 22/06/2015 17:10, Jonathan Wakely wrote:
>> On 20/06/15 12:59 +0100, Jonathan Wakely wrote:
>>> On 20/06/15 12:03 +0200, François Dumont wrote:
>>>> Hi
>>>>
>>>>   2 experimental tests are failing in debug mode because
>>>> __do_str_codecvt is sometimes taking address of string front() and
>>>> back() even if empty. It wasn't use so not a big issue but it still
>>>> seems better to avoid. I propose to rather use string begin() to get
>>>> buffer address.
>>>
>>> But derefencing begin() is still undefined for an empty string.
>>> Shouldn't that fail for debug mode too? Why change one form of
>>> undefined behaviour that we diagnose to another form that we don't
>>> diagnose?
>>>
>>> It would be better if that function didn't do any work when the input
>>> range is empty:
>>>
>>> --- a/libstdc++-v3/include/bits/locale_conv.h
>>> +++ b/libstdc++-v3/include/bits/locale_conv.h
>>> @@ -58,6 +58,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>                    _OutStr& __outstr, const _Codecvt& __cvt, _State&
>>> __state,
>>>                    size_t& __count, _Fn __fn)
>>>    {
>>> +      if (__first == __last)
>>> +       {
>>> +         __outstr.clear();
>>> +         return true;
>>> +       }
>>> +
>
>Shouldn't it also set __count to 0 to make clear that nothing has been
>converted.

Yes, thanks. Fixed like so.

Tested powerpc64le-linux, in debug and normal modes, committed to trunk.



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

commit f02e0e6dfd0ffbef53252aa0b739a9b1ba7391ff
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jun 25 14:51:19 2015 +0100

    	* include/bits/locale_conv.h (__do_str_codecvt): Set __count.

diff --git a/libstdc++-v3/include/bits/locale_conv.h b/libstdc++-v3/include/bits/locale_conv.h
index fd99499..146f78b 100644
--- a/libstdc++-v3/include/bits/locale_conv.h
+++ b/libstdc++-v3/include/bits/locale_conv.h
@@ -61,6 +61,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       if (__first == __last)
 	{
 	  __outstr.clear();
+	  __count = 0;
 	  return true;
 	}
 

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

* Re: Do not take address of empty string front
  2015-06-22 15:11   ` Jonathan Wakely
  2015-06-23 20:09     ` François Dumont
  2015-06-24 20:17     ` François Dumont
@ 2015-07-02 22:54     ` Jonathan Wakely
  2 siblings, 0 replies; 7+ messages in thread
From: Jonathan Wakely @ 2015-07-02 22:54 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

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

On 22/06/15 16:10 +0100, Jonathan Wakely wrote:
>On 20/06/15 12:59 +0100, Jonathan Wakely wrote:
>>On 20/06/15 12:03 +0200, François Dumont wrote:
>>>Hi
>>>
>>>  2 experimental tests are failing in debug mode because
>>>__do_str_codecvt is sometimes taking address of string front() and
>>>back() even if empty. It wasn't use so not a big issue but it still
>>>seems better to avoid. I propose to rather use string begin() to get
>>>buffer address.
>>
>>But derefencing begin() is still undefined for an empty string.
>>Shouldn't that fail for debug mode too? Why change one form of
>>undefined behaviour that we diagnose to another form that we don't
>>diagnose?
>>
>>It would be better if that function didn't do any work when the input
>>range is empty:
>>
>>--- a/libstdc++-v3/include/bits/locale_conv.h
>>+++ b/libstdc++-v3/include/bits/locale_conv.h
>>@@ -58,6 +58,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>                   _OutStr& __outstr, const _Codecvt& __cvt, _State& __state,
>>                   size_t& __count, _Fn __fn)
>>   {
>>+      if (__first == __last)
>>+       {
>>+         __outstr.clear();
>>+         return true;
>>+       }
>>+
>>     size_t __outchars = 0;
>>     auto __next = __first;
>>     const auto __maxlen = __cvt.max_length() + 1;
>
>This makes that change, and also moves wstring_convert into the
>ABI-tagged __cxx11 namespace, and fixes a copy&paste error in the
>exception thrown from wbuffer_convert.

This is the equivalent patch for the branch.

Tested powerpc64le-linux, committed to gcc-5-branch.

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

commit ab6011c5ffbd16f7f3f509f6e9fec6dc9f7daf36
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Jul 1 15:41:33 2015 +0100

    	* include/bits/locale_conv.h (wstring_convert): Use __cxx11 inline
    	namespace in new ABI.
    	(wstring_convert::_M_conv): Handle empty range.

diff --git a/libstdc++-v3/include/bits/locale_conv.h b/libstdc++-v3/include/bits/locale_conv.h
index 9be2866..de49dd5 100644
--- a/libstdc++-v3/include/bits/locale_conv.h
+++ b/libstdc++-v3/include/bits/locale_conv.h
@@ -51,6 +51,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
    * @{
    */
 
+_GLIBCXX_BEGIN_NAMESPACE_CXX11
   /// String conversions
   template<typename _Codecvt, typename _Elem = wchar_t,
 	   typename _Wide_alloc = allocator<_Elem>,
@@ -192,10 +193,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	_M_conv(const _InChar* __first, const _InChar* __last,
 		const _OutStr* __err, _MemFn __memfn)
 	{
+	  auto __outstr = __err ? _OutStr(__err->get_allocator()) : _OutStr();
+
+	  if (__first == __last)
+	    {
+	      _M_count = 0;
+	      return __outstr;
+	    }
+
 	  if (!_M_with_cvtstate)
 	    _M_state = state_type();
 
-	  auto __outstr = __err ? _OutStr(__err->get_allocator()) : _OutStr();
 	  size_t __outchars = 0;
 	  auto __next = __first;
 	  const auto __maxlen = _M_cvt->max_length() + 1;
@@ -239,6 +247,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool			_M_with_cvtstate = false;
       bool			_M_with_strings = false;
     };
+_GLIBCXX_END_NAMESPACE_CXX11
 
   /// Buffer conversions
   template<typename _Codecvt, typename _Elem = wchar_t,
@@ -264,7 +273,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_buf(__bytebuf), _M_cvt(__pcvt), _M_state(__state)
       {
 	if (!_M_cvt)
-	  __throw_logic_error("wstring_convert");
+	  __throw_logic_error("wbuffer_convert");
 
 	_M_always_noconv = _M_cvt->always_noconv();
 

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

end of thread, other threads:[~2015-07-02 22:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-20 10:47 Do not take address of empty string front François Dumont
2015-06-20 20:38 ` Jonathan Wakely
2015-06-22 15:11   ` Jonathan Wakely
2015-06-23 20:09     ` François Dumont
2015-06-25 18:27       ` Jonathan Wakely
2015-06-24 20:17     ` François Dumont
2015-07-02 22:54     ` Jonathan Wakely

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).