[PATCH] Fix float to LONGEST conversion.

[PATCH] Fix float to LONGEST conversion.

[Ali Tamur via gdb-patches]

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

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

Re: [PATCH] Fix float to LONGEST conversion.

[Andrew Burgess]

* 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
> 

Re: [PATCH] Fix float to LONGEST conversion.

[Ali Tamur via gdb-patches]

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

Re: [PATCH] Fix float to LONGEST conversion.

[Ali Tamur via gdb-patches]

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

Re: [PATCH] Fix float to LONGEST conversion.

[Ali Tamur via gdb-patches]

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