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

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