| Message ID | 20210824142730.102421-3-luis.pires@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: DFP instructions using decodetree | expand |
On Tue, Aug 24, 2021 at 11:27:13AM -0300, Luis Pires wrote: > Move abs64 to host-utils so it can be reused elsewhere. > Also made it inline. > > Signed-off-by: Luis Pires <luis.pires@eldorado.org.br> > --- > hw/i386/kvm/i8254.c | 5 ----- > include/qemu/host-utils.h | 8 ++++++++ > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c > index fa68669e8a..761034743b 100644 > --- a/hw/i386/kvm/i8254.c > +++ b/hw/i386/kvm/i8254.c > @@ -59,11 +59,6 @@ struct KVMPITClass { > DeviceRealize parent_realize; > }; > > -static int64_t abs64(int64_t v) > -{ > - return v < 0 ? -v : v; Hrm.. I'm a bit concerned about mkaing this a more widespread function, because it has a nasty edge case... which is basically unavoidable in an abs64() implementation. Specifically: abs64(0x800_0000_0000_00000) == 0x800_0000_0000_0000 < 0 At least in the most likely 2's complement implementation. > -} > - > static void kvm_pit_update_clock_offset(KVMPITState *s) > { > int64_t offset, clock_offset; > diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h > index 711b221704..5fec44a9c4 100644 > --- a/include/qemu/host-utils.h > +++ b/include/qemu/host-utils.h > @@ -357,6 +357,14 @@ static inline uint64_t revbit64(uint64_t x) > #endif > } > > +/** > + * Return the absolute value of a 64-bit integer > + */ > +static inline int64_t abs64(int64_t v) > +{ > + return v < 0 ? -v : v; > +} > + > /** > * sadd32_overflow - addition with overflow indication > * @x, @y: addends
From: David Gibson <david@gibson.dropbear.id.au> > Hrm.. I'm a bit concerned about mkaing this a more widespread function, > because it has a nasty edge case... which is basically unavoidable in an abs64() > implementation. Specifically: > > abs64(0x800_0000_0000_00000) == 0x800_0000_0000_0000 < 0 > > At least in the most likely 2's complement implementation. Right, that's true of any standard implementation of abs(). I thought about making it return uint64_t, but that could make it weird for other uses of abs64(), where callers wouldn't expect a type change from int64_t to uint64_t. Maybe create a separate uabs64() that returns uint64_t? Or is that even weirder? :) -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On Wed, Aug 25, 2021 at 12:48:35PM +0000, Luis Fernando Fujita Pires wrote: > From: David Gibson <david@gibson.dropbear.id.au> > > Hrm.. I'm a bit concerned about mkaing this a more widespread function, > > because it has a nasty edge case... which is basically unavoidable in an abs64() > > implementation. Specifically: > > > > abs64(0x800_0000_0000_00000) == 0x800_0000_0000_0000 < 0 > > > > At least in the most likely 2's complement implementation. > > Right, that's true of any standard implementation of abs(). > I thought about making it return uint64_t, but that could make > it weird for other uses of abs64(), where callers wouldn't > expect a type change from int64_t to uint64_t. Maybe create a > separate uabs64() that returns uint64_t? Or is that even > weirder? :) Which users of abs64 would expect it to return int64_t? kvm_pit_update_clock_offset() doesn't seem to.
From: Eduardo Habkost <ehabkost@redhat.com> > > Right, that's true of any standard implementation of abs(). > > I thought about making it return uint64_t, but that could make it > > weird for other uses of abs64(), where callers wouldn't expect a type > > change from int64_t to uint64_t. Maybe create a separate uabs64() that > > returns uint64_t? Or is that even weirder? :) > > Which users of abs64 would expect it to return int64_t? > kvm_pit_update_clock_offset() doesn't seem to. Oh, I wasn't referring to any specific users. What I meant is that, if we make abs64() generically available from host-utils, callers could expect it to behave the same way as abs() in stdlib, for example. -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On Wed, Aug 25, 2021 at 08:37:17PM +0000, Luis Fernando Fujita Pires wrote: > From: Eduardo Habkost <ehabkost@redhat.com> > > > > Right, that's true of any standard implementation of abs(). > > > I thought about making it return uint64_t, but that could make it > > > weird for other uses of abs64(), where callers wouldn't expect a type > > > change from int64_t to uint64_t. Maybe create a separate uabs64() that > > > returns uint64_t? Or is that even weirder? :) > > > > Which users of abs64 would expect it to return int64_t? > > kvm_pit_update_clock_offset() doesn't seem to. > > Oh, I wasn't referring to any specific users. What I meant is > that, if we make abs64() generically available from host-utils, > callers could expect it to behave the same way as abs() in > stdlib, for example. That would be surprising, but do you think there are cases where that would be a bad surprise? I don't think anybody who is aware of the abs(INT_MIN), labs(LONG_MIN), and llabs(LLONG_MIN) edge cases actually _like_ that behaviour. If you really want to avoid surprises, providing a saner function with a different name seems better than trying to emulate the edge cases of abs()/labs()/llabs().
On 8/25/21 11:18 PM, Eduardo Habkost wrote: > On Wed, Aug 25, 2021 at 08:37:17PM +0000, Luis Fernando Fujita Pires wrote: >> From: Eduardo Habkost <ehabkost@redhat.com> >> >>>> Right, that's true of any standard implementation of abs(). >>>> I thought about making it return uint64_t, but that could make it >>>> weird for other uses of abs64(), where callers wouldn't expect a type >>>> change from int64_t to uint64_t. Maybe create a separate uabs64() that >>>> returns uint64_t? Or is that even weirder? :) >>> >>> Which users of abs64 would expect it to return int64_t? >>> kvm_pit_update_clock_offset() doesn't seem to. >> >> Oh, I wasn't referring to any specific users. What I meant is >> that, if we make abs64() generically available from host-utils, >> callers could expect it to behave the same way as abs() in >> stdlib, for example. > > That would be surprising, but do you think there are cases where > that would be a bad surprise? > > I don't think anybody who is aware of the abs(INT_MIN), > labs(LONG_MIN), and llabs(LLONG_MIN) edge cases actually _like_ > that behaviour. > > If you really want to avoid surprises, providing a saner function > with a different name seems better than trying to emulate the > edge cases of abs()/labs()/llabs(). Agreed. See do_strtosz() for example.
> >> Oh, I wasn't referring to any specific users. What I meant is that, > >> if we make abs64() generically available from host-utils, callers > >> could expect it to behave the same way as abs() in stdlib, for > >> example. > > > > That would be surprising, but do you think there are cases where that > > would be a bad surprise? > > > > I don't think anybody who is aware of the abs(INT_MIN), > > labs(LONG_MIN), and llabs(LLONG_MIN) edge cases actually _like_ that > > behaviour. > > > > If you really want to avoid surprises, providing a saner function with > > a different name seems better than trying to emulate the edge cases of > > abs()/labs()/llabs(). > > Agreed. See do_strtosz() for example. I'll make this change when I submit the next version of this patch series. Thanks! -- Luis Pires Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c index fa68669e8a..761034743b 100644 --- a/hw/i386/kvm/i8254.c +++ b/hw/i386/kvm/i8254.c @@ -59,11 +59,6 @@ struct KVMPITClass { DeviceRealize parent_realize; }; -static int64_t abs64(int64_t v) -{ - return v < 0 ? -v : v; -} - static void kvm_pit_update_clock_offset(KVMPITState *s) { int64_t offset, clock_offset; diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h index 711b221704..5fec44a9c4 100644 --- a/include/qemu/host-utils.h +++ b/include/qemu/host-utils.h @@ -357,6 +357,14 @@ static inline uint64_t revbit64(uint64_t x) #endif } +/** + * Return the absolute value of a 64-bit integer + */ +static inline int64_t abs64(int64_t v) +{ + return v < 0 ? -v : v; +} + /** * sadd32_overflow - addition with overflow indication * @x, @y: addends
Move abs64 to host-utils so it can be reused elsewhere. Also made it inline. Signed-off-by: Luis Pires <luis.pires@eldorado.org.br> --- hw/i386/kvm/i8254.c | 5 ----- include/qemu/host-utils.h | 8 ++++++++ 2 files changed, 8 insertions(+), 5 deletions(-)