| Message ID | 20190828032808.242363-1-tamur@google.com |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 91828 invoked by alias); 28 Aug 2019 03:28:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 91796 invoked by uid 89); 28 Aug 2019 03:28:14 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=H*MI:google, HX-HELO:sk:mail-pg, H*r:sk:mail-pg X-HELO: mail-pg1-f201.google.com Received: from mail-pg1-f201.google.com (HELO mail-pg1-f201.google.com) (209.85.215.201) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 28 Aug 2019 03:28:13 +0000 Received: by mail-pg1-f201.google.com with SMTP id z35so813106pgk.10 for <gdb-patches@sourceware.org>; Tue, 27 Aug 2019 20:28:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:message-id:mime-version:subject:from:to:cc; bh=cPfZKvJHTLe7VxqJ4MRrAKBPwzT3nGblPnaRPHRCRIs=; b=DBl2Ujhgrv7W0G0cmozOaIFPB3OKpfcsIDUPzPW58PzhNCWDoqx3jiQa48ZEO253LA 78WB5q5rNMDcgE+/lHyjUsKT1WzooHDDmedoitoPQOx6x4Sqr6PYewm0GA51sG74b0Ay I6Xj2ApOEajHGICgo/oPW1gSkPyd+bOb9Sa73vPA8vYXu6NXE0EfRG0uyOPPdlXqoFdw H+gF324SVbZ+NKwj5TCQpddoOE476QQvEF4iaFHnaLP2dshn3kuzJecsutf07w4lmMIS 8uyse5jPWeP4zjusW6bDiReU9wzTAjo3fCoImym1PzACPVlhGBA6c3OpGPc8JqMGb6hY D1lg== Date: Tue, 27 Aug 2019 20:28:08 -0700 Message-Id: <20190828032808.242363-1-tamur@google.com> Mime-Version: 1.0 Subject: [PATCH] Fix float to LONGEST conversion. From: "Ali Tamur via gdb-patches" <gdb-patches@sourceware.org> Reply-To: Ali Tamur <tamur@google.com> To: gdb-patches@sourceware.org Cc: Ali Tamur <tamur@google.com> Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes |
Commit Message
Terekhov, Mikhail via Gdb-patches
Aug. 28, 2019, 3:28 a.m. UTC
The code used to have undefined behaviour. gdb/ChangeLog: *gdb/target-float.c (host_float_ops<T>::to_longest): Update implementation. --- gdb/target-float.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
Comments
* Ali Tamur via gdb-patches <gdb-patches@sourceware.org> [2019-08-27 20:28:08 -0700]: > The code used to have undefined behaviour. I would much prefer to see a more detailed explanation for _why_ the previous behaviour is undefined. I ran my eye over the old code but didn't see anything obvious. If at a later date I come back and want to figure out why this patch went in it would be nice if the commit message could tell me everything I need to know. Thanks, Andrew > > > gdb/ChangeLog: > *gdb/target-float.c (host_float_ops<T>::to_longest): Update > implementation. > --- > gdb/target-float.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/gdb/target-float.c b/gdb/target-float.c > index 39abb12696..0fd71c0dc3 100644 > --- a/gdb/target-float.c > +++ b/gdb/target-float.c > @@ -1007,13 +1007,18 @@ host_float_ops<T>::to_longest (const gdb_byte *addr, > { > T host_float; > from_target (type, addr, &host_float); > - /* Converting an out-of-range value is undefined behavior in C, but we > - prefer to return a defined value here. */ > - if (host_float > std::numeric_limits<LONGEST>::max()) > - return std::numeric_limits<LONGEST>::max(); > - if (host_float < std::numeric_limits<LONGEST>::min()) > + T min_possible_range = static_cast<T>(std::numeric_limits<LONGEST>::min()); > + T max_possible_range = -min_possible_range; > + /* host_float can be converted to an integer as long as it's in > + the range [min_possible_range, max_possible_range). If not, it is either > + too large, or too small, or is NaN; in this case return the maximum or > + minimum possible value. */ > + if (host_float < max_possible_range && host_float >= min_possible_range) > + return static_cast<LONGEST> (host_float); > + if (host_float < min_possible_range) > return std::numeric_limits<LONGEST>::min(); > - return (LONGEST) host_float; > + /* This line will be executed if host_float is NaN. */ > + return std::numeric_limits<LONGEST>::max(); > } > > /* Convert signed integer VAL to a target floating-number of type TYPE > -- > 2.23.0.187.g17f5b7556c-goog >
Updated the commit message: > Fix float to LONGEST conversion. The code used to have undefined behaviour when template parameter is float and host_float is NaN, because it attempted to convert NaN value to LONGEST at the last statement. This frequently caused crashes on tests that checked "info all-registers" (at least when the code is compiled with clang; I didn't test with gdb). On Wed, Aug 28, 2019 at 1:29 AM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > * Ali Tamur via gdb-patches <gdb-patches@sourceware.org> [2019-08-27 > 20:28:08 -0700]: > > > The code used to have undefined behaviour. > > I would much prefer to see a more detailed explanation for _why_ the > previous behaviour is undefined. I ran my eye over the old code but > didn't see anything obvious. > > If at a later date I come back and want to figure out why this patch > went in it would be nice if the commit message could tell me > everything I need to know. > > Thanks, > Andrew > > > > > > > > > gdb/ChangeLog: > > *gdb/target-float.c (host_float_ops<T>::to_longest): Update > > implementation. > > --- > > gdb/target-float.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/gdb/target-float.c b/gdb/target-float.c > > index 39abb12696..0fd71c0dc3 100644 > > --- a/gdb/target-float.c > > +++ b/gdb/target-float.c > > @@ -1007,13 +1007,18 @@ host_float_ops<T>::to_longest (const gdb_byte > *addr, > > { > > T host_float; > > from_target (type, addr, &host_float); > > - /* Converting an out-of-range value is undefined behavior in C, but we > > - prefer to return a defined value here. */ > > - if (host_float > std::numeric_limits<LONGEST>::max()) > > - return std::numeric_limits<LONGEST>::max(); > > - if (host_float < std::numeric_limits<LONGEST>::min()) > > + T min_possible_range = > static_cast<T>(std::numeric_limits<LONGEST>::min()); > > + T max_possible_range = -min_possible_range; > > + /* host_float can be converted to an integer as long as it's in > > + the range [min_possible_range, max_possible_range). If not, it is > either > > + too large, or too small, or is NaN; in this case return the > maximum or > > + minimum possible value. */ > > + if (host_float < max_possible_range && host_float >= > min_possible_range) > > + return static_cast<LONGEST> (host_float); > > + if (host_float < min_possible_range) > > return std::numeric_limits<LONGEST>::min(); > > - return (LONGEST) host_float; > > + /* This line will be executed if host_float is NaN. */ > > + return std::numeric_limits<LONGEST>::max(); > > } > > > > /* Convert signed integer VAL to a target floating-number of type TYPE > > -- > > 2.23.0.187.g17f5b7556c-goog > > >
Hi, Can I submit if there aren't any other concerns? Thanks, Ali On Wed, Sep 4, 2019 at 2:28 PM Ali Tamur <tamur@google.com> wrote: > > Updated the commit message: > > > Fix float to LONGEST conversion. > > The code used to have undefined behaviour when template parameter is float and > host_float is NaN, because it attempted to convert NaN value to LONGEST at the > last statement. This frequently caused crashes on tests that checked "info > all-registers" (at least when the code is compiled with clang; I didn't test > with gdb). > > On Wed, Aug 28, 2019 at 1:29 AM Andrew Burgess <andrew.burgess@embecosm.com> wrote: >> >> * Ali Tamur via gdb-patches <gdb-patches@sourceware.org> [2019-08-27 20:28:08 -0700]: >> >> > The code used to have undefined behaviour. >> >> I would much prefer to see a more detailed explanation for _why_ the >> previous behaviour is undefined. I ran my eye over the old code but >> didn't see anything obvious. >> >> If at a later date I come back and want to figure out why this patch >> went in it would be nice if the commit message could tell me >> everything I need to know. >> >> Thanks, >> Andrew >> >> >> >> > >> > >> > gdb/ChangeLog: >> > *gdb/target-float.c (host_float_ops<T>::to_longest): Update >> > implementation. >> > --- >> > gdb/target-float.c | 17 +++++++++++------ >> > 1 file changed, 11 insertions(+), 6 deletions(-) >> > >> > diff --git a/gdb/target-float.c b/gdb/target-float.c >> > index 39abb12696..0fd71c0dc3 100644 >> > --- a/gdb/target-float.c >> > +++ b/gdb/target-float.c >> > @@ -1007,13 +1007,18 @@ host_float_ops<T>::to_longest (const gdb_byte *addr, >> > { >> > T host_float; >> > from_target (type, addr, &host_float); >> > - /* Converting an out-of-range value is undefined behavior in C, but we >> > - prefer to return a defined value here. */ >> > - if (host_float > std::numeric_limits<LONGEST>::max()) >> > - return std::numeric_limits<LONGEST>::max(); >> > - if (host_float < std::numeric_limits<LONGEST>::min()) >> > + T min_possible_range = static_cast<T>(std::numeric_limits<LONGEST>::min()); >> > + T max_possible_range = -min_possible_range; >> > + /* host_float can be converted to an integer as long as it's in >> > + the range [min_possible_range, max_possible_range). If not, it is either >> > + too large, or too small, or is NaN; in this case return the maximum or >> > + minimum possible value. */ >> > + if (host_float < max_possible_range && host_float >= min_possible_range) >> > + return static_cast<LONGEST> (host_float); >> > + if (host_float < min_possible_range) >> > return std::numeric_limits<LONGEST>::min(); >> > - return (LONGEST) host_float; >> > + /* This line will be executed if host_float is NaN. */ >> > + return std::numeric_limits<LONGEST>::max(); >> > } >> > >> > /* Convert signed integer VAL to a target floating-number of type TYPE >> > -- >> > 2.23.0.187.g17f5b7556c-goog >> >
Hi, If there are no other concerns I am going to submit this tomorrow morning. Thanks. On Sun, Sep 8, 2019 at 7:36 PM Ali Tamur <tamur@google.com> wrote: > > Hi, > Can I submit if there aren't any other concerns? > Thanks, > Ali > > On Wed, Sep 4, 2019 at 2:28 PM Ali Tamur <tamur@google.com> wrote: > > > > Updated the commit message: > > > > > Fix float to LONGEST conversion. > > > > The code used to have undefined behaviour when template parameter is float and > > host_float is NaN, because it attempted to convert NaN value to LONGEST at the > > last statement. This frequently caused crashes on tests that checked "info > > all-registers" (at least when the code is compiled with clang; I didn't test > > with gdb). > > > > On Wed, Aug 28, 2019 at 1:29 AM Andrew Burgess <andrew.burgess@embecosm.com> wrote: > >> > >> * Ali Tamur via gdb-patches <gdb-patches@sourceware.org> [2019-08-27 20:28:08 -0700]: > >> > >> > The code used to have undefined behaviour. > >> > >> I would much prefer to see a more detailed explanation for _why_ the > >> previous behaviour is undefined. I ran my eye over the old code but > >> didn't see anything obvious. > >> > >> If at a later date I come back and want to figure out why this patch > >> went in it would be nice if the commit message could tell me > >> everything I need to know. > >> > >> Thanks, > >> Andrew > >> > >> > >> > >> > > >> > > >> > gdb/ChangeLog: > >> > *gdb/target-float.c (host_float_ops<T>::to_longest): Update > >> > implementation. > >> > --- > >> > gdb/target-float.c | 17 +++++++++++------ > >> > 1 file changed, 11 insertions(+), 6 deletions(-) > >> > > >> > diff --git a/gdb/target-float.c b/gdb/target-float.c > >> > index 39abb12696..0fd71c0dc3 100644 > >> > --- a/gdb/target-float.c > >> > +++ b/gdb/target-float.c > >> > @@ -1007,13 +1007,18 @@ host_float_ops<T>::to_longest (const gdb_byte *addr, > >> > { > >> > T host_float; > >> > from_target (type, addr, &host_float); > >> > - /* Converting an out-of-range value is undefined behavior in C, but we > >> > - prefer to return a defined value here. */ > >> > - if (host_float > std::numeric_limits<LONGEST>::max()) > >> > - return std::numeric_limits<LONGEST>::max(); > >> > - if (host_float < std::numeric_limits<LONGEST>::min()) > >> > + T min_possible_range = static_cast<T>(std::numeric_limits<LONGEST>::min()); > >> > + T max_possible_range = -min_possible_range; > >> > + /* host_float can be converted to an integer as long as it's in > >> > + the range [min_possible_range, max_possible_range). If not, it is either > >> > + too large, or too small, or is NaN; in this case return the maximum or > >> > + minimum possible value. */ > >> > + if (host_float < max_possible_range && host_float >= min_possible_range) > >> > + return static_cast<LONGEST> (host_float); > >> > + if (host_float < min_possible_range) > >> > return std::numeric_limits<LONGEST>::min(); > >> > - return (LONGEST) host_float; > >> > + /* This line will be executed if host_float is NaN. */ > >> > + return std::numeric_limits<LONGEST>::max(); > >> > } > >> > > >> > /* Convert signed integer VAL to a target floating-number of type TYPE > >> > -- > >> > 2.23.0.187.g17f5b7556c-goog > >> >
diff --git a/gdb/target-float.c b/gdb/target-float.c index 39abb12696..0fd71c0dc3 100644 --- a/gdb/target-float.c +++ b/gdb/target-float.c @@ -1007,13 +1007,18 @@ host_float_ops<T>::to_longest (const gdb_byte *addr, { T host_float; from_target (type, addr, &host_float); - /* Converting an out-of-range value is undefined behavior in C, but we - prefer to return a defined value here. */ - if (host_float > std::numeric_limits<LONGEST>::max()) - return std::numeric_limits<LONGEST>::max(); - if (host_float < std::numeric_limits<LONGEST>::min()) + T min_possible_range = static_cast<T>(std::numeric_limits<LONGEST>::min()); + T max_possible_range = -min_possible_range; + /* host_float can be converted to an integer as long as it's in + the range [min_possible_range, max_possible_range). If not, it is either + too large, or too small, or is NaN; in this case return the maximum or + minimum possible value. */ + if (host_float < max_possible_range && host_float >= min_possible_range) + return static_cast<LONGEST> (host_float); + if (host_float < min_possible_range) return std::numeric_limits<LONGEST>::min(); - return (LONGEST) host_float; + /* This line will be executed if host_float is NaN. */ + return std::numeric_limits<LONGEST>::max(); } /* Convert signed integer VAL to a target floating-number of type TYPE