diff mbox series

[07/21] linux-user/hexagon: Implement setup_sigtramp

Message ID 20210616011209.1446045-8-richard.henderson@linaro.org (mailing list archive)
State New, archived
Headers show
Series linux-user: Move signal trampolines to new page | expand

Commit Message

Richard Henderson June 16, 2021, 1:11 a.m. UTC
Continue to initialize the words on the stack, as documented.
However, use the off-stack trampoline.

Cc: Taylor Simpson <tsimpson@quicinc.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/hexagon/target_signal.h |  2 ++
 linux-user/hexagon/signal.c        | 19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

Comments

Taylor Simpson June 16, 2021, 8:07 a.m. UTC | #1
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, June 15, 2021 7:12 PM
> To: qemu-devel@nongnu.org
> Cc: laurent@vivier.eu; alex.bennee@linaro.org; Taylor Simpson
> <tsimpson@quicinc.com>
> Subject: [PATCH 07/21] linux-user/hexagon: Implement setup_sigtramp
> 
> Continue to initialize the words on the stack, as documented.
> However, use the off-stack trampoline.
> 
> Cc: Taylor Simpson <tsimpson@quicinc.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> diff --git a/linux-user/hexagon/signal.c b/linux-user/hexagon/signal.c index
> 85eab5e943..bd0f9b1c85 100644
> --- a/linux-user/hexagon/signal.c
> +++ b/linux-user/hexagon/signal.c
> @@ -161,6 +161,11 @@ void setup_rt_frame(int sig, struct target_sigaction
> *ka,
> 
>      setup_ucontext(&frame->uc, env, set);
>      tswap_siginfo(&frame->info, info);
> +    /*
> +     * The on-stack signal trampoline is no longer executed;
> +     * however, the libgcc signal frame unwinding code checks
> +     * for the presence of these two numeric magic values.
> +     */

Hexagon uses musl, not libgcc.  So, I'm not sure if this is needed.  The signals.c test passes for me without this change.  Are you seeing it fail?


> +void setup_sigtramp(abi_ulong sigtramp_page) {
> +    uint32_t *tramp = lock_user(VERIFY_WRITE, sigtramp_page, 4 * 2, 0);
> +    assert(tramp != NULL);
> +
> +    default_rt_sigreturn = sigtramp_page;
> +    install_sigtramp(tramp);
> +
> +    unlock_user(tramp, sigtramp_page, 4 * 2); }

Put the closing curly on a new line.


Thanks,
Taylor
Richard Henderson June 16, 2021, 3:05 p.m. UTC | #2
On 6/16/21 1:07 AM, Taylor Simpson wrote:
>> +    /*
>> +     * The on-stack signal trampoline is no longer executed;
>> +     * however, the libgcc signal frame unwinding code checks
>> +     * for the presence of these two numeric magic values.
>> +     */
> 
> Hexagon uses musl, not libgcc.  So, I'm not sure if this is needed.  The signals.c test passes for me without this change.  Are you seeing it fail?

I copied the comment from the kernel source.


>> +void setup_sigtramp(abi_ulong sigtramp_page) {
>> +    uint32_t *tramp = lock_user(VERIFY_WRITE, sigtramp_page, 4 * 2, 0);
>> +    assert(tramp != NULL);
>> +
>> +    default_rt_sigreturn = sigtramp_page;
>> +    install_sigtramp(tramp);
>> +
>> +    unlock_user(tramp, sigtramp_page, 4 * 2); }
> 
> Put the closing curly on a new line.

That's your mailer.  It's correct in the original.


r~
Richard Henderson June 16, 2021, 3:50 p.m. UTC | #3
On 6/16/21 8:05 AM, Richard Henderson wrote:
> On 6/16/21 1:07 AM, Taylor Simpson wrote:
>>> +    /*
>>> +     * The on-stack signal trampoline is no longer executed;
>>> +     * however, the libgcc signal frame unwinding code checks
>>> +     * for the presence of these two numeric magic values.
>>> +     */
>>
>> Hexagon uses musl, not libgcc.  So, I'm not sure if this is needed.  The signals.c test 
>> passes for me without this change.  Are you seeing it fail?
> 
> I copied the comment from the kernel source.

Also, I think you're confusing libc and libgcc.


r~
Taylor Simpson June 16, 2021, 9:37 p.m. UTC | #4
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Wednesday, June 16, 2021 9:51 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: laurent@vivier.eu; alex.bennee@linaro.org
> Subject: Re: [PATCH 07/21] linux-user/hexagon: Implement setup_sigtramp
> 
> On 6/16/21 8:05 AM, Richard Henderson wrote:
> > On 6/16/21 1:07 AM, Taylor Simpson wrote:
> >>> +    /*
> >>> +     * The on-stack signal trampoline is no longer executed;
> >>> +     * however, the libgcc signal frame unwinding code checks
> >>> +     * for the presence of these two numeric magic values.
> >>> +     */
> >>
> >> Hexagon uses musl, not libgcc.  So, I'm not sure if this is needed.
> >> The signals.c test passes for me without this change.  Are you seeing it
> fail?
> >
> > I copied the comment from the kernel source.
> 
> Also, I think you're confusing libc and libgcc.

Yes, I'm confused.  Why is signal frame unwinding in libgcc?

Also FWIW, we use LLVM's compiler-rt instead of libgcc.
Richard Henderson June 16, 2021, 11:15 p.m. UTC | #5
On 6/16/21 2:37 PM, Taylor Simpson wrote:
> Yes, I'm confused.  Why is signal frame unwinding in libgcc?

Because it's tied to the compiler, and it was a decent solution back in 1997.

I see llvm calls it libunwind, and only has special signal frame support for aarch64.  I 
wonder if ever other target is expecting unwind info in a vdso?


r~
diff mbox series

Patch

diff --git a/linux-user/hexagon/target_signal.h b/linux-user/hexagon/target_signal.h
index 345cf1cbb8..9e0223d322 100644
--- a/linux-user/hexagon/target_signal.h
+++ b/linux-user/hexagon/target_signal.h
@@ -31,4 +31,6 @@  typedef struct target_sigaltstack {
 
 #include "../generic/signal.h"
 
+#define TARGET_ARCH_HAS_SIGTRAMP_PAGE 1
+
 #endif /* TARGET_SIGNAL_H */
diff --git a/linux-user/hexagon/signal.c b/linux-user/hexagon/signal.c
index 85eab5e943..bd0f9b1c85 100644
--- a/linux-user/hexagon/signal.c
+++ b/linux-user/hexagon/signal.c
@@ -161,6 +161,11 @@  void setup_rt_frame(int sig, struct target_sigaction *ka,
 
     setup_ucontext(&frame->uc, env, set);
     tswap_siginfo(&frame->info, info);
+    /*
+     * The on-stack signal trampoline is no longer executed;
+     * however, the libgcc signal frame unwinding code checks
+     * for the presence of these two numeric magic values.
+     */
     install_sigtramp(frame->tramp);
 
     env->gpr[HEX_REG_PC] = ka->_sa_handler;
@@ -170,8 +175,7 @@  void setup_rt_frame(int sig, struct target_sigaction *ka,
         frame_addr + offsetof(struct target_rt_sigframe, info);
     env->gpr[HEX_REG_R02] =
         frame_addr + offsetof(struct target_rt_sigframe, uc);
-    env->gpr[HEX_REG_LR] =
-        frame_addr + offsetof(struct target_rt_sigframe, tramp);
+    env->gpr[HEX_REG_LR] = default_rt_sigreturn;
 
     return;
 
@@ -270,3 +274,14 @@  badframe:
     force_sig(TARGET_SIGSEGV);
     return 0;
 }
+
+void setup_sigtramp(abi_ulong sigtramp_page)
+{
+    uint32_t *tramp = lock_user(VERIFY_WRITE, sigtramp_page, 4 * 2, 0);
+    assert(tramp != NULL);
+
+    default_rt_sigreturn = sigtramp_page;
+    install_sigtramp(tramp);
+
+    unlock_user(tramp, sigtramp_page, 4 * 2);
+}