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

[Laurent Vivier]

Thank you Peter,

I will address you comments and send a new version of the series.

Laurent

Le 11/02/2020 à 18:05, Peter Maydell a écrit :
> On Tue, 4 Feb 2020 at 17:11, Laurent Vivier <address@hidden> wrote:
>>
>> 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>
>> ---
> 
> In general I think this is a good approach to trying to deal
> with this long-standing issue in a pragmatic and not too
> complicated way, so thanks for writing this patchset. I have
> some fairly minor comments on the code below.
> 
>>
>> Notes:
>>     v2: ignore error when target sig <= TARGET_NSIG but host sig > SIGRTMAX
>>         replace i, j by target_sig, host_sig
>>         update signal_table_init() trace message
>>
>>  linux-user/signal.c     | 37 ++++++++++++++++++++++++++++++-------
>>  linux-user/trace-events |  3 +++
>>  2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index c1e664f97a7c..e7e5581a016f 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -498,18 +498,23 @@ static int core_dump_signal(int sig)
>>
>>  static void signal_table_init(void)
>>  {
>> -    int host_sig, target_sig;
>> +    int host_sig, target_sig, count;
>>
>>      /*
>> -     * 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 (host_sig = SIGRTMIN; host_sig <= SIGRTMAX; host_sig++) {
>> +        target_sig = host_sig - SIGRTMIN + TARGET_SIGRTMIN;
>> +        if (target_sig <= TARGET_NSIG) {
>> +            host_to_target_signal_table[host_sig] = target_sig;
>> +        }
>> +    }
> 
> So the effect of this is that we now support target signals
> starting from TARGET_SIGRTMIN and going up until we run out
> of host realtime signals that the host libc hasn't reserved ?
> That seems reasonable, since glibc at least uses only the
> lower 2 rt signals and probably nobody's using the upper ones.
> But this would be a good place to have a comment explaining
> the limitation (and that if it needed to be fixed we'd have
> to multiplex guest signals onto a single host signal). You
> could also mention that attempts to configure the "missing"
> signals via sigaction will be silently ignored.
> 
>>      /* generate signal conversion tables */
>> +    for (target_sig = 1; target_sig <= TARGET_NSIG; target_sig++) {
>> +        target_to_host_signal_table[target_sig] = _NSIG; /* poison */
>> +    }
>>      for (host_sig = 1; host_sig < _NSIG; host_sig++) {
>>          if (host_to_target_signal_table[host_sig] == 0) {
>>              host_to_target_signal_table[host_sig] = host_sig;
>> @@ -519,6 +524,15 @@ static void signal_table_init(void)
>>              target_to_host_signal_table[target_sig] = host_sig;
>>          }
>>      }
>> +
>> +    if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) {
> 
> This isn't the right way to conditionalize expensive stuff
> that's only used in trace events. You want to use
> trace_event_get_state_backends() (see docs/devel/tracing.txt
> for details).
> 
>> +        for (target_sig = 1, count = 0; target_sig <= TARGET_NSIG; 
>> target_sig++) {
>> +            if (target_to_host_signal_table[target_sig] == _NSIG) {
>> +                count++;
>> +            }
>> +        }
>> +        trace_signal_table_init(count);
>> +    }
>>  }
>>
>>  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);
>> +
>>      if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == 
>> TARGET_SIGSTOP) {
>>          return -TARGET_EINVAL;
>>      }
>> @@ -847,6 +863,13 @@ int do_sigaction(int sig, const struct target_sigaction 
>> *act,
>>
>>          /* we update the host linux signal state */
>>          host_sig = target_to_host_signal(sig);
>> +        trace_signal_do_sigaction_host(host_sig, TARGET_NSIG);
>> +        if (host_sig > SIGRTMAX) {
>> +            /* we don't have enough host signals to map all target signals 
>> */
>> +            qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d, 
>> ignored\n",
>> +                          sig);
>> +            return 0;
> 
> We should have a comment here mentioning why we don't return
> an error code here (and explicitly noting that the Go runtime
> is the major one which we don't want to upset).
> 
>> +        }
>>          if (host_sig != SIGSEGV && host_sig != SIGBUS) {
>>              sigfillset(&act1.sa_mask);
>>              act1.sa_flags = SA_SIGINFO;
>> diff --git a/linux-user/trace-events b/linux-user/trace-events
>> index f6de1b8befc0..0296133daeb6 100644
>> --- a/linux-user/trace-events
>> +++ b/linux-user/trace-events
>> @@ -1,6 +1,9 @@
>>  # See docs/devel/tracing.txt for syntax documentation.
>>
>>  # signal.c
>> +signal_table_init(int i) "number of unavailable signals: %d"
>> +signal_do_sigaction_guest(int sig, int max) "target signal %d (MAX %d)"
>> +signal_do_sigaction_host(int sig, int max) "host signal %d (MAX %d)"
>>  # */signal.c
>>  user_setup_frame(void *env, uint64_t frame_addr) "env=%p 
>> frame_addr=0x%"PRIx64
>>  user_setup_rt_frame(void *env, uint64_t frame_addr) "env=%p 
>> frame_addr=0x%"PRIx64
> 
> thanks
> -- PMM
>