From patchwork Wed Jun 24 20:13:49 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Fran=C3=A7ois_Dumont?= X-Patchwork-Id: 488196 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id E962E140309 for ; Thu, 25 Jun 2015 06:14:09 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=ZAxG99lk; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=cHfWlkwqgeadjN/dD jR90xVAuIAB7xYaB7KKJTLy7G/M7V/wRixleIgAIgcqErOYxEbOLPiuE3hZaYrUj J30+o1kFRIiEHn3G7Gqv+bTfOvkfzW18/1hbL4chSrROUFzpZD0TH6E3grBpi+fK omJuEGCtTW5PZ5sqwH7OdA7+4g= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=UFI/rMhHdVRw9xVdEEjrT0F WhOA=; b=ZAxG99lkM7Wi42qiOiaOHrFZfi1pxSY2SUrwrHI37hYfhKauoTjSQb7 occeVm5/qEf+W9+Obw6k07G8lgefAa10+T7mHfKhPX/ST57ObJ8nrV6Mn+/ZDUrE RmrZQMzYjuyyVTLBfdLIIhPLm4jj3efms471x6sHce5jJrGC46F4= Received: (qmail 46130 invoked by alias); 24 Jun 2015 20:14:02 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 46059 invoked by uid 89); 24 Jun 2015 20:14:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-wi0-f180.google.com Received: from mail-wi0-f180.google.com (HELO mail-wi0-f180.google.com) (209.85.212.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 24 Jun 2015 20:14:00 +0000 Received: by wibdq8 with SMTP id dq8so56941462wib.1; Wed, 24 Jun 2015 13:13:57 -0700 (PDT) X-Received: by 10.194.23.225 with SMTP id p1mr75105264wjf.155.1435176837298; Wed, 24 Jun 2015 13:13:57 -0700 (PDT) Received: from [192.168.0.22] (arf62-1-82-237-250-248.fbx.proxad.net. [82.237.250.248]) by mx.google.com with ESMTPSA id d3sm42276103wjs.21.2015.06.24.13.13.55 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 24 Jun 2015 13:13:56 -0700 (PDT) Message-ID: <558B0F7D.6010405@gmail.com> Date: Wed, 24 Jun 2015 22:13:49 +0200 From: =?windows-1252?Q?Fran=E7ois_Dumont?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Jonathan Wakely CC: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: Do not take address of empty string front References: <55853A70.7030607@gmail.com> <20150620115928.GG2856@redhat.com> <20150622151000.GI2856@redhat.com> In-Reply-To: <20150622151000.GI2856@redhat.com> 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 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 #include #include + #if __cplusplus >= 201103L #include #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 /**