[PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Taylor Simpson]

Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 linux-user/signal.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5ca6d62..ce3d27f 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -72,6 +72,14 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
        over a single host signal.  */
     [__SIGRTMIN] = __SIGRTMAX,
     [__SIGRTMAX] = __SIGRTMIN,
+#ifdef TARGET_HEXAGON
+    /*
+     * Hexagon uses the same signal for pthread cancel as the host pthreads,
+     * so cannot be overridden.
+     * Therefore, we map Hexagon signal to a different host signal.
+     */
+    [__SIGRTMAX - 1] = __SIGRTMIN + 1,
+#endif
 };
 static uint8_t target_to_host_signal_table[_NSIG];
 
-- 
2.7.4

Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Peter Maydell]

On Tue, 19 Nov 2019 at 19:07, Taylor Simpson <tsimpson@quicinc.com> wrote:
>
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>  linux-user/signal.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5ca6d62..ce3d27f 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -72,6 +72,14 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>         over a single host signal.  */
>      [__SIGRTMIN] = __SIGRTMAX,
>      [__SIGRTMAX] = __SIGRTMIN,
> +#ifdef TARGET_HEXAGON
> +    /*
> +     * Hexagon uses the same signal for pthread cancel as the host pthreads,
> +     * so cannot be overridden.
> +     * Therefore, we map Hexagon signal to a different host signal.
> +     */
> +    [__SIGRTMAX - 1] = __SIGRTMIN + 1,
> +#endif
>  };

This breaks other stuff, unfortunately, like Go binaries.
(Also, you now have two host signals mapped to the same
target signal; notice that the existing RTMAX/RTMIN
is a swap of the two slots.)

We need a generic solution for this, Hexagon is not the
only one with the problem. There's a patchset on list
from ages back that had a suggested approach, but
it needed review and work.

thanks
-- PMM

RE: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Taylor Simpson]

Peter,

Yeah, I was surprised not to see other targets encountering this problem.  However, I don't understand how something under #ifdef TARGET_HEXAGON can break anything else.

Could you point me to the patch set you mention?

One generic solution I can think of is to reference a target-defined macro in linux-user/signal.c and let targets optionally define it in their target_signal.h.  So, it would look something like this
> @@ -72,6 +72,14 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>         over a single host signal.  */
>      [__SIGRTMIN] = __SIGRTMAX,
>      [__SIGRTMAX] = __SIGRTMIN,
> +#ifdef TARGET_SIGNAL_TABLE_MODIFY
> +    TARGET_SIGNAL_TABLE_MODIFY

Thanks,
Taylor


-----Original Message-----
From: Peter Maydell <peter.maydell@linaro.org>
Sent: Tuesday, November 19, 2019 1:31 PM
To: Taylor Simpson <tsimpson@quicinc.com>
Cc: Riku Voipio <riku.voipio@iki.fi>; Laurent Vivier <laurent@vivier.eu>; QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1


On Tue, 19 Nov 2019 at 19:07, Taylor Simpson <tsimpson@quicinc.com> wrote:
>
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>  linux-user/signal.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c index
> 5ca6d62..ce3d27f 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -72,6 +72,14 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>         over a single host signal.  */
>      [__SIGRTMIN] = __SIGRTMAX,
>      [__SIGRTMAX] = __SIGRTMIN,
> +#ifdef TARGET_HEXAGON
> +    /*
> +     * Hexagon uses the same signal for pthread cancel as the host pthreads,
> +     * so cannot be overridden.
> +     * Therefore, we map Hexagon signal to a different host signal.
> +     */
> +    [__SIGRTMAX - 1] = __SIGRTMIN + 1, #endif
>  };

This breaks other stuff, unfortunately, like Go binaries.
(Also, you now have two host signals mapped to the same target signal; notice that the existing RTMAX/RTMIN is a swap of the two slots.)

We need a generic solution for this, Hexagon is not the only one with the problem. There's a patchset on list from ages back that had a suggested approach, but it needed review and work.

thanks
-- PMM

Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Laurent Vivier]

Le 19/11/2019 à 20:31, Peter Maydell a écrit :
> On Tue, 19 Nov 2019 at 19:07, Taylor Simpson <tsimpson@quicinc.com> wrote:
>>
>> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
>> ---
>>  linux-user/signal.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index 5ca6d62..ce3d27f 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -72,6 +72,14 @@ static uint8_t host_to_target_signal_table[_NSIG] = {
>>         over a single host signal.  */
>>      [__SIGRTMIN] = __SIGRTMAX,
>>      [__SIGRTMAX] = __SIGRTMIN,
>> +#ifdef TARGET_HEXAGON
>> +    /*
>> +     * Hexagon uses the same signal for pthread cancel as the host pthreads,
>> +     * so cannot be overridden.
>> +     * Therefore, we map Hexagon signal to a different host signal.
>> +     */
>> +    [__SIGRTMAX - 1] = __SIGRTMIN + 1,
>> +#endif
>>  };
> 
> This breaks other stuff, unfortunately, like Go binaries.
> (Also, you now have two host signals mapped to the same
> target signal; notice that the existing RTMAX/RTMIN
> is a swap of the two slots.)
> 
> We need a generic solution for this, Hexagon is not the
> only one with the problem. There's a patchset on list
> from ages back that had a suggested approach, but
> it needed review and work.

For the moment we don't have a better solution, Josh Kunz tried to
rework [1] patches from Miloš Stojanović [2] but it doesn't seem to be
an easy task.

So I agree we need a generic solution to fix this problem, but in the
meantime I told to Taylor if he doesn't care about Go on hexagon he can
do this change specifically to his target (perhaps we can have cleaner
approach by containing this change into linux-user/hexagon). And what I
understand is hexagon libc (musl) doesn't work without this.

Thanks,
Laurent

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg00738.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg05259.html

Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Peter Maydell]

On Wed, 20 Nov 2019 at 08:10, Laurent Vivier <laurent@vivier.eu> wrote:
> For the moment we don't have a better solution, Josh Kunz tried to
> rework [1] patches from Miloš Stojanović [2] but it doesn't seem to be
> an easy task.
>
> So I agree we need a generic solution to fix this problem, but in the
> meantime I told to Taylor if he doesn't care about Go on hexagon he can
> do this change specifically to his target (perhaps we can have cleaner
> approach by containing this change into linux-user/hexagon). And what I
> understand is hexagon libc (musl) doesn't work without this.

I really don't like target-specific ifdeffery that changes behaviour
that shouldn't be target specific. They reduce the chances we ever
try to go back and actually fix the underlying problem correctly,
and they can be awkward to undo without breaking things.
As far as I can tell there is nothing special about Hexagon here.

thanks
-- PMM

Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Laurent Vivier]

Le 20/11/2019 à 11:45, Peter Maydell a écrit :
> On Wed, 20 Nov 2019 at 08:10, Laurent Vivier <laurent@vivier.eu> wrote:
>> For the moment we don't have a better solution, Josh Kunz tried to
>> rework [1] patches from Miloš Stojanović [2] but it doesn't seem to be
>> an easy task.
>>
>> So I agree we need a generic solution to fix this problem, but in the
>> meantime I told to Taylor if he doesn't care about Go on hexagon he can
>> do this change specifically to his target (perhaps we can have cleaner
>> approach by containing this change into linux-user/hexagon). And what I
>> understand is hexagon libc (musl) doesn't work without this.
> 
> I really don't like target-specific ifdeffery that changes behaviour
> that shouldn't be target specific. They reduce the chances we ever
> try to go back and actually fix the underlying problem correctly,
> and they can be awkward to undo without breaking things.
> As far as I can tell there is nothing special about Hexagon here.

I understand your point, and I agree, but not allowing this will block
the merge of the hexagon target, and I don't see any fix for the
underlying problem coming soon.

Other targets work without this change, and adding this change breaks
some user space applications (like go), whereas adding this change for
hexagon target only will improve the situation for it (with no
regression, as it doesn't work at all for the moment)

But, yes, we must fix the underlying problem...

Thanks,
Laurent

Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Peter Maydell]

On Wed, 20 Nov 2019 at 10:54, Laurent Vivier <laurent@vivier.eu> wrote:
> I understand your point, and I agree, but not allowing this will block
> the merge of the hexagon target, and I don't see any fix for the
> underlying problem coming soon.
>
> Other targets work without this change, and adding this change breaks
> some user space applications (like go), whereas adding this change for
> hexagon target only will improve the situation for it (with no
> regression, as it doesn't work at all for the moment)

I care more that we should fix things correctly and maintain the
consistency of how our architectures behave than that we are
able to quickly land a target for a fairly minor architecture,
to be honest. If we land hexagon with hacks and workarounds
then we're potentially stuck with that behaviour.

thanks
-- PMM

RE: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Taylor Simpson]

How was this solved for other targets?

-----Original Message-----
From: Peter Maydell <peter.maydell@linaro.org>
Sent: Wednesday, November 20, 2019 5:01 AM
To: Laurent Vivier <laurent@vivier.eu>
Cc: Taylor Simpson <tsimpson@quicinc.com>; Riku Voipio <riku.voipio@iki.fi>; QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1


On Wed, 20 Nov 2019 at 10:54, Laurent Vivier <laurent@vivier.eu> wrote:
> I understand your point, and I agree, but not allowing this will block
> the merge of the hexagon target, and I don't see any fix for the
> underlying problem coming soon.
>
> Other targets work without this change, and adding this change breaks
> some user space applications (like go), whereas adding this change for
> hexagon target only will improve the situation for it (with no
> regression, as it doesn't work at all for the moment)

I care more that we should fix things correctly and maintain the consistency of how our architectures behave than that we are able to quickly land a target for a fairly minor architecture, to be honest. If we land hexagon with hacks and workarounds then we're potentially stuck with that behaviour.

thanks
-- PMM

Re: [PATCH] Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1

[Peter Maydell]

On Wed, 20 Nov 2019 at 12:54, Taylor Simpson <tsimpson@quicinc.com> wrote:
>
> How was this solved for other targets?

It hasn't been, yet. Other targets only run guest
code that doesn't care about this signal number
being unavailable.

thanks
-- PMM