This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix float to LONGEST conversion.
- From: "Ali Tamur via gdb-patches" <gdb-patches at sourceware dot org>
- To: Andrew Burgess <andrew dot burgess at embecosm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 10 Sep 2019 11:46:37 -0700
- Subject: Re: [PATCH] Fix float to LONGEST conversion.
- References: <20190828032808.242363-1-tamur@google.com> <20190828082935.GQ6076@embecosm.com> <CAH=Am=5gK61MgNBFD4gZpweLd72u8qY340t4F6X-=V3DWP3b8A@mail.gmail.com> <CAH=Am=5uk1XQqBa9pTOvP6mF39_b5UPBFK7=TXy7OQdH1jn4UA@mail.gmail.com>
- Reply-to: Ali Tamur <tamur at google dot com>
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
> >> >