When op raises an exception, it may not have initialized the output
temps that would be written back by wout or cout.

Cc: qemu-s390x@nongnu.org
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/translate.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 7363aabf3a..7fad3ad8e9 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
     if (insn->help_op) {
         ret = insn->help_op(s, &o);
     }
-    if (insn->help_wout) {
-        insn->help_wout(s, &f, &o);
-    }
-    if (insn->help_cout) {
-        insn->help_cout(s, &o);
+    if (ret != DISAS_NORETURN) {
+        if (insn->help_wout) {
+            insn->help_wout(s, &f, &o);
+        }
+        if (insn->help_cout) {
+            insn->help_cout(s, &o);
+        }
     }
 
     /* Free any temporaries created by the helpers.  */
-- 
2.17.1

On 03/10/2018 21:39, Richard Henderson wrote:
> When op raises an exception, it may not have initialized the output
> temps that would be written back by wout or cout.
> 
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/translate.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 7363aabf3a..7fad3ad8e9 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>      if (insn->help_op) {
>          ret = insn->help_op(s, &o);
>      }
> -    if (insn->help_wout) {
> -        insn->help_wout(s, &f, &o);
> -    }
> -    if (insn->help_cout) {
> -        insn->help_cout(s, &o);
> +    if (ret != DISAS_NORETURN) {
> +        if (insn->help_wout) {
> +            insn->help_wout(s, &f, &o);
> +        }
> +        if (insn->help_cout) {
> +            insn->help_cout(s, &o);
> +        }
>      }
>  
>      /* Free any temporaries created by the helpers.  */
> 

What about things like LPSW/LPWSE ? They certainly don't imply that we
had an exception.

(these two don't use wout/cout, so it is still fine, but I would prefer
a comment somewhere because otherwise it is really easy to miss that
DISAS_NORETURN makes us skip these handlers)

Apart from that

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

-- 

Thanks,

David / dhildenb

On 10/11/18 1:06 AM, David Hildenbrand wrote:
> On 03/10/2018 21:39, Richard Henderson wrote:
>> When op raises an exception, it may not have initialized the output
>> temps that would be written back by wout or cout.
>>
>> Cc: qemu-s390x@nongnu.org
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/s390x/translate.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
>> index 7363aabf3a..7fad3ad8e9 100644
>> --- a/target/s390x/translate.c
>> +++ b/target/s390x/translate.c
>> @@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>>      if (insn->help_op) {
>>          ret = insn->help_op(s, &o);
>>      }
>> -    if (insn->help_wout) {
>> -        insn->help_wout(s, &f, &o);
>> -    }
>> -    if (insn->help_cout) {
>> -        insn->help_cout(s, &o);
>> +    if (ret != DISAS_NORETURN) {
>> +        if (insn->help_wout) {
>> +            insn->help_wout(s, &f, &o);
>> +        }
>> +        if (insn->help_cout) {
>> +            insn->help_cout(s, &o);
>> +        }
>>      }
>>  
>>      /* Free any temporaries created by the helpers.  */
>>
> 
> What about things like LPSW/LPWSE ? They certainly don't imply that we
> had an exception.

Exception in the tcg sense, not the guest architectural sense, in that we call
cpu_loop_exit from the helper, which performs a longjmp.  (Incidentally,
there's no reason to do that for load_psw -- we could just exit the tb normally.)

> (these two don't use wout/cout, so it is still fine, but I would prefer
> a comment somewhere because otherwise it is really easy to miss that
> DISAS_NORETURN makes us skip these handlers)

Where would you like me to place that comment?  In the DisasInsn definition?


r~

On 11/10/2018 18:47, Richard Henderson wrote:
> On 10/11/18 1:06 AM, David Hildenbrand wrote:
>> On 03/10/2018 21:39, Richard Henderson wrote:
>>> When op raises an exception, it may not have initialized the output
>>> temps that would be written back by wout or cout.
>>>
>>> Cc: qemu-s390x@nongnu.org
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>  target/s390x/translate.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
>>> index 7363aabf3a..7fad3ad8e9 100644
>>> --- a/target/s390x/translate.c
>>> +++ b/target/s390x/translate.c
>>> @@ -6164,11 +6164,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>>>      if (insn->help_op) {
>>>          ret = insn->help_op(s, &o);
>>>      }
>>> -    if (insn->help_wout) {
>>> -        insn->help_wout(s, &f, &o);
>>> -    }
>>> -    if (insn->help_cout) {
>>> -        insn->help_cout(s, &o);
>>> +    if (ret != DISAS_NORETURN) {
>>> +        if (insn->help_wout) {
>>> +            insn->help_wout(s, &f, &o);
>>> +        }
>>> +        if (insn->help_cout) {
>>> +            insn->help_cout(s, &o);
>>> +        }
>>>      }
>>>  
>>>      /* Free any temporaries created by the helpers.  */
>>>
>>
>> What about things like LPSW/LPWSE ? They certainly don't imply that we
>> had an exception.
> 
> Exception in the tcg sense, not the guest architectural sense, in that we call
> cpu_loop_exit from the helper, which performs a longjmp.  (Incidentally,
> there's no reason to do that for load_psw -- we could just exit the tb normally.)
> 
>> (these two don't use wout/cout, so it is still fine, but I would prefer
>> a comment somewhere because otherwise it is really easy to miss that
>> DISAS_NORETURN makes us skip these handlers)
> 
> Where would you like me to place that comment?  In the DisasInsn definition?

That probably makes sense!

> 
> 
> r~
> 


-- 

Thanks,

David / dhildenb