Re: [PATCH] tests/tcg/s390x: Test overflow conditions

[Thomas Huth]

 Hi!

On 30/05/2022 11.50, David Hildenbrand wrote:
> On 27.05.22 12:11, Gautam Agrawal wrote:
> > Add a test to check for overflow conditions in s390x.
> > This patch is based on the following patches :
> > * https://git.qemu.org/?p=qemu.git;a=commitdiff;h=5a2e67a691501
> > * https://git.qemu.org/?p=qemu.git;a=commitdiff;h=fc6e0d0f2db51
> >   
> > Signed-off-by: Gautam Agrawal <gautamnagrawal@gmail.com>
> > ---
> >   tests/tcg/s390x/Makefile.target |  1 +
> >   tests/tcg/s390x/overflow.c      | 58 +++++++++++++++++++++++++++++++++
> >   2 files changed, 59 insertions(+)
> >   create mode 100644 tests/tcg/s390x/overflow.c
> >
> > diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> > index 3124172736..7f86de85b9 100644
> > --- a/tests/tcg/s390x/Makefile.target
> > +++ b/tests/tcg/s390x/Makefile.target
> > @@ -16,6 +16,7 @@ TESTS+=shift
> >   TESTS+=trap
> >   TESTS+=signals-s390x
> >   TESTS+=branch-relative-long
> > +TESTS+=overflow
> >   
> >   VECTOR_TESTS=vxeh2_vs
> >   VECTOR_TESTS+=vxeh2_vcvt
> > diff --git a/tests/tcg/s390x/overflow.c b/tests/tcg/s390x/overflow.c
> > new file mode 100644
> > index 0000000000..ea8a410b1a
> > --- /dev/null
> > +++ b/tests/tcg/s390x/overflow.c
> > @@ -0,0 +1,58 @@
> > +#include <stdio.h>
> > +
> > +int overflow_add_32(int x, int y)
> > +{
> > +    int sum;
> > +    return __builtin_add_overflow(x, y, &sum);
> > +}
> > +
> > +int overflow_add_64(long long x, long long y)
> > +{
> > +    long sum;
>
> Just wondering, why "long long" in input and "long" in output?

It's been like this in the original test program that has been supplied in 
https://gitlab.com/qemu-project/qemu/-/issues/616 and .../618 - but I agree 
it likely makes more sense to use the same type everywhere (i.e. switch sum 
from long to long long).

> > +    return __builtin_add_overflow(x, y, &sum);
> > +}
> > +
> > +int overflow_sub_32(int x, int y)
> > +{
> > +    int sum;
> > +    return __builtin_sub_overflow(x, y, &sum);
> > +}
> > +
> > +int overflow_sub_64(long long x, long long y)
> > +{
> > +    long sum;
> > +    return __builtin_sub_overflow(x, y, &sum);
>
> nit: I'd call all local variables "ret" or "res".

Well, "sum" is not the return value here, so "ret" could be confusing, too. 
"res" or "diff" might be a good choice here, though. Gautam, what do you think?

 Thomas