Re: [PATCH 4/4] linux-user: fix use of SIGRTMIN

[Laurent Vivier]

Le 04/02/2020 à 00:15, Taylor Simpson a écrit :
> 
> 
>> -----Original Message-----
>> From: Laurent Vivier <address@hidden>
>> Sent: Saturday, February 1, 2020 6:28 AM
>> To: address@hidden
>> Cc: Josh Kunz <address@hidden>; address@hidden; Matus Kysel
>> <address@hidden>; Aleksandar Markovic <aleksandar.markovic@rt-
>> rk.com>; Marlies Ruck <address@hidden>; Laurent Vivier
>> <address@hidden>; Peter Maydell <address@hidden>; Taylor
>> Simpson <address@hidden>; Riku Voipio <address@hidden>
>> Subject: [PATCH 4/4] linux-user: fix use of SIGRTMIN
>>
>> Some RT signals can be in use by glibc,
>> it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32).
>>
>> So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN.
>>
>> Instead of swapping only SIGRTMIN and SIGRTMAX, map all the range
>> [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to
>>       [__SIGRTMIN + X ... SIGRTMAX ]
>> (SIGRTMIN is __SIGRTMIN + X).
>>
>> Signed-off-by: Laurent Vivier <address@hidden>
>> ---
>>  linux-user/signal.c     | 34 ++++++++++++++++++++++++++++------
>>  linux-user/trace-events |  3 +++
>>  2 files changed, 31 insertions(+), 6 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c index
>> 3491f0a7ecb1..c4abacde49a0 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -501,15 +501,20 @@ static void signal_table_init(void)
>>      int i, j;
>>
>>      /*
>> -     * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with
>> -     * host libpthread signals.  This assumes no one actually uses SIGRTMAX 
>> :-/
>> -     * To fix this properly we need to do manual signal delivery multiplexed
>> -     * over a single host signal.
>> +     * some RT signals can be in use by glibc,
>> +     * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32)
>>       */
>> -    host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX;
>> -    host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN;
>> +    for (i = SIGRTMIN; i <= SIGRTMAX; i++) {
>> +        j = i - SIGRTMIN + TARGET_SIGRTMIN;
>> +        if (j <= TARGET_NSIG) {
>> +            host_to_target_signal_table[i] = j;
>> +        }
>> +    }
>>
>>      /* generate signal conversion tables */
>> +    for (i = 1; i <= TARGET_NSIG; i++) {
>> +        target_to_host_signal_table[i] = _NSIG; /* poison */
>> +    }
>>      for (i = 1; i < _NSIG; i++) {
>>          if (host_to_target_signal_table[i] == 0) {
>>              host_to_target_signal_table[i] = i; @@ -519,6 +524,15 @@ static 
>> void
>> signal_table_init(void)
>>              target_to_host_signal_table[j] = i;
>>          }
>>      }
>> +
>> +    if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) {
>> +        for (i = 1, j = 0; i <= TARGET_NSIG; i++) {
>> +            if (target_to_host_signal_table[i] == _NSIG) {
>> +                j++;
>> +            }
>> +        }
>> +        trace_signal_table_init(j);
> 
> It looks like j will have a count of the number of poison entries, but the 
> message in trace_signal_table_init is "missing signal number".  Is that what 
> you intend?

Yes, poison entries are the entries that cannot be used for the guest.
Perhaps it would be more correct as "Number of missing signals:"?

> 
>> +    }
>>  }
>>
>>  void signal_init(void)
>> @@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction
>> *act,
>>      int host_sig;
>>      int ret = 0;
>>
>> +    trace_signal_do_sigaction_guest(sig, TARGET_NSIG);
> 
> Shouldn't this be _NSIG, not TARGET_NSIG?

No: do_sigaction() takes a number from the guest, so "sig" is a target
signal number, and this trace displays also the maximum value for the
target.

Thanks,
Laurent