Re: [Qemu-devel] [PATCH 1/2] linux-user: Implement signals for openrisc

[Richard Henderson <richard.henderson@linaro.org>]

On 06/01/2018 01:45 PM, Laurent Vivier wrote:
> Le 31/05/2018 à 06:18, Richard Henderson a écrit :
>> All of the existing code was boilerplate from elsewhere,
>> and would crash the guest upon the first signal.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  linux-user/openrisc/target_syscall.h |  23 +--
>>  linux-user/openrisc/signal.c         | 210 +++++++++++----------------
>>  linux-user/signal.c                  |   2 +-
>>  target/openrisc/cpu.c                |   1 +
>>  4 files changed, 87 insertions(+), 149 deletions(-)
>>
> ...
>> @@ -148,11 +102,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>>                      target_siginfo_t *info,
>>                      target_sigset_t *set, CPUOpenRISCState *env)
>>  {
> ...
>> @@ -160,47 +111,32 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>>          goto give_sigsegv;
>>      }
>>  
>> -    info_addr = frame_addr + offsetof(struct target_rt_sigframe, info);
>> -    __put_user(info_addr, &frame->pinfo);
>> -    uc_addr = frame_addr + offsetof(struct target_rt_sigframe, uc);
>> -    __put_user(uc_addr, &frame->puc);
>> +    tswap_siginfo(&frame->info, info);
>>  
>> -    if (ka->sa_flags & SA_SIGINFO) {
>> -        tswap_siginfo(&frame->info, info);
>> -    }
> 
> Why do you remove the "ka->sa_flags & SA_SIGINFO"?

I thought it was unconditional for rt signals, which are the only ones that
openrisc has.  It's definitely odd, since the kernel also checks that flag, but
unconditionally points r4 to the (possibly uninitialized) siginfo_t.

>> -    /*err |= copy_to_user(frame->uc.tuc_sigmask, set, sizeof(*set));*/
> 
> other targets have something like:
> 
>     for (i = 0; i < TARGET_NSIG_WORDS; i++) {
>         __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
>     }
> 
> to match kernel
> 
>         err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
> 
> Do we need it?

Yes, this is my mistake.


r~