[PATCH] tcg/s390: Fix compare instruction from extended-immediate facility

[PATCH] tcg/s390: Fix compare instruction from extended-immediate facility

[Philippe Mathieu-Daudé]

The code is currently comparing c2 to the type promotion of
uint32_t and int32_t. That is, the conversion rules are as:

  (common_type) c2 == (common_type) (uint32_t)
                        (is_unsigned
                        ? (uint32_t)c2
                        : (uint32_t)(int32_t)c2)

In the signed case we lose the desired sign extensions because
of the argument promotion rules of the ternary operator.

Solve the problem by doing the round-trip parsing through the
intermediate type and back to the desired common type (all at
one expression).

Fixes: a534bb15f30 ("tcg/s390: Use constant pool for cmpi")
Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
Reported-by: Richard W.M. Jones <rjones@redhat.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tcg/s390/tcg-target.c.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc
index b67470137c4..695d7ee6523 100644
--- a/tcg/s390/tcg-target.c.inc
+++ b/tcg/s390/tcg-target.c.inc
@@ -1067,7 +1067,7 @@ static int tgen_cmp(TCGContext *s, TCGType type, TCGCond c, TCGReg r1,
                 op = (is_unsigned ? RIL_CLFI : RIL_CFI);
                 tcg_out_insn_RIL(s, op, r1, c2);
                 goto exit;
-            } else if (c2 == (is_unsigned ? (uint32_t)c2 : (int32_t)c2)) {
+            } else if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : (TCGArg)(int32_t)c2)) {
                 op = (is_unsigned ? RIL_CLGFI : RIL_CGFI);
                 tcg_out_insn_RIL(s, op, r1, c2);
                 goto exit;
-- 
2.26.2

Re: [PATCH] tcg/s390: Fix compare instruction from extended-immediate facility

[Richard Henderson]

On 2/4/21 8:29 AM, Philippe Mathieu-Daudé wrote:
> The code is currently comparing c2 to the type promotion of
> uint32_t and int32_t. That is, the conversion rules are as:
> 
>   (common_type) c2 == (common_type) (uint32_t)
>                         (is_unsigned
>                         ? (uint32_t)c2
>                         : (uint32_t)(int32_t)c2)
> 
> In the signed case we lose the desired sign extensions because
> of the argument promotion rules of the ternary operator.
> 
> Solve the problem by doing the round-trip parsing through the
> intermediate type and back to the desired common type (all at
> one expression).
> 
> Fixes: a534bb15f30 ("tcg/s390: Use constant pool for cmpi")
> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tcg/s390/tcg-target.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

A most excellent catch, thanks.  Queued.


r~

Re: [PATCH] tcg/s390: Fix compare instruction from extended-immediate facility

[David Hildenbrand]

On 04.02.21 19:29, Philippe Mathieu-Daudé wrote:
> The code is currently comparing c2 to the type promotion of
> uint32_t and int32_t. That is, the conversion rules are as:
> 
>    (common_type) c2 == (common_type) (uint32_t)
>                          (is_unsigned
>                          ? (uint32_t)c2
>                          : (uint32_t)(int32_t)c2)
> 
> In the signed case we lose the desired sign extensions because
> of the argument promotion rules of the ternary operator.
> 
> Solve the problem by doing the round-trip parsing through the
> intermediate type and back to the desired common type (all at
> one expression).
> 
> Fixes: a534bb15f30 ("tcg/s390: Use constant pool for cmpi")
> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   tcg/s390/tcg-target.c.inc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc
> index b67470137c4..695d7ee6523 100644
> --- a/tcg/s390/tcg-target.c.inc
> +++ b/tcg/s390/tcg-target.c.inc
> @@ -1067,7 +1067,7 @@ static int tgen_cmp(TCGContext *s, TCGType type, TCGCond c, TCGReg r1,
>                   op = (is_unsigned ? RIL_CLFI : RIL_CFI);
>                   tcg_out_insn_RIL(s, op, r1, c2);
>                   goto exit;
> -            } else if (c2 == (is_unsigned ? (uint32_t)c2 : (int32_t)c2)) {
> +            } else if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : (TCGArg)(int32_t)c2)) {
>                   op = (is_unsigned ? RIL_CLGFI : RIL_CGFI);
>                   tcg_out_insn_RIL(s, op, r1, c2);
>                   goto exit;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH] tcg/s390: Fix compare instruction from extended-immediate facility

[Richard W.M. Jones]

On Thu, Feb 04, 2021 at 07:29:02PM +0100, Philippe Mathieu-Daudé wrote:
> The code is currently comparing c2 to the type promotion of
> uint32_t and int32_t. That is, the conversion rules are as:
> 
>   (common_type) c2 == (common_type) (uint32_t)
>                         (is_unsigned
>                         ? (uint32_t)c2
>                         : (uint32_t)(int32_t)c2)
> 
> In the signed case we lose the desired sign extensions because
> of the argument promotion rules of the ternary operator.
> 
> Solve the problem by doing the round-trip parsing through the
> intermediate type and back to the desired common type (all at
> one expression).
> 
> Fixes: a534bb15f30 ("tcg/s390: Use constant pool for cmpi")
> Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tcg/s390/tcg-target.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc
> index b67470137c4..695d7ee6523 100644
> --- a/tcg/s390/tcg-target.c.inc
> +++ b/tcg/s390/tcg-target.c.inc
> @@ -1067,7 +1067,7 @@ static int tgen_cmp(TCGContext *s, TCGType type, TCGCond c, TCGReg r1,
>                  op = (is_unsigned ? RIL_CLFI : RIL_CFI);
>                  tcg_out_insn_RIL(s, op, r1, c2);
>                  goto exit;
> -            } else if (c2 == (is_unsigned ? (uint32_t)c2 : (int32_t)c2)) {
> +            } else if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : (TCGArg)(int32_t)c2)) {
>                  op = (is_unsigned ? RIL_CLGFI : RIL_CGFI);
>                  tcg_out_insn_RIL(s, op, r1, c2);
>                  goto exit;

I have tested this patch on an s390x host on top of 1ed9228f63e "Merge
remote-tracking branch 'remotes/ericb/tags/pull-nbd-2021-02-02-v2'
into staging" and I can confirm that it fixes both previously reported
problems.  Therefore you can add:

Tested-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org