| Message ID | 1574190388-12605-1-git-send-email-tsimpson@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hexagon: Swap SIGRGMAX-1 and SIGRTMIN+1 | expand |
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
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
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
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
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
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
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
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
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];
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> --- linux-user/signal.c | 8 ++++++++ 1 file changed, 8 insertions(+)