[PATCH 0/3] linux-user: Fix some minor nits

[PATCH 0/3] linux-user: Fix some minor nits

[Peter Maydell]

This patchset fixes up some minor nits in the linux-user code that I
noticed while I was reading code to assist with reviewing the
bsd-user signal handling.

thanks
-- PMM

Peter Maydell (3):
  linux-user: Remove unnecessary 'aligned' attribute from TaskState
  linux-user: Rename user_force_sig tracepoint to match function name
  linux-user: Return void from queue_signal()

 linux-user/qemu.h          | 6 +-----
 linux-user/signal-common.h | 4 ++--
 linux-user/signal.c        | 7 +++----
 linux-user/trace-events    | 2 +-
 4 files changed, 7 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState

[Peter Maydell]

The linux-user struct TaskState has an 'aligned(16)' attribute.  When
the struct was first added in commit 851e67a1b46f in 2003, there was
a justification in a comment (still present in the source today):

/* NOTE: we force a big alignment so that the stack stored after is
   aligned too */

because the final field in the struct was "uint8_t stack[0];"
But that field was removed in commit 48e15fc2d in 2010 which
switched us to allocating the stack and the TaskState separately.
Because we allocate the structure with g_new0() rather than as
a local variable, the attribute made no difference to the alignment
of the structure anyway.

Remove the unnecessary attribute, and the corresponding comment.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/qemu.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 5c713fa8ab2..bd0559759ae 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -96,10 +96,6 @@ struct emulated_sigtable {
     target_siginfo_t info;
 };
 
-/*
- * NOTE: we force a big alignment so that the stack stored after is
- * aligned too
- */
 typedef struct TaskState {
     pid_t ts_tid;     /* tid (or pid) of this task */
 #ifdef TARGET_ARM
@@ -160,7 +156,7 @@ typedef struct TaskState {
 
     /* This thread's sigaltstack, if it has one */
     struct target_sigaltstack sigaltstack_used;
-} __attribute__((aligned(16))) TaskState;
+} TaskState;
 
 abi_long do_brk(abi_ulong new_brk);
 
-- 
2.25.1

Re: [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState

[Laurent Vivier]

Le 14/01/2022 à 16:37, Peter Maydell a écrit :
> The linux-user struct TaskState has an 'aligned(16)' attribute.  When
> the struct was first added in commit 851e67a1b46f in 2003, there was
> a justification in a comment (still present in the source today):
> 
> /* NOTE: we force a big alignment so that the stack stored after is
>     aligned too */
> 
> because the final field in the struct was "uint8_t stack[0];"
> But that field was removed in commit 48e15fc2d in 2010 which
> switched us to allocating the stack and the TaskState separately.
> Because we allocate the structure with g_new0() rather than as
> a local variable, the attribute made no difference to the alignment
> of the structure anyway.
> 
> Remove the unnecessary attribute, and the corresponding comment.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   linux-user/qemu.h | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 5c713fa8ab2..bd0559759ae 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -96,10 +96,6 @@ struct emulated_sigtable {
>       target_siginfo_t info;
>   };
>   
> -/*
> - * NOTE: we force a big alignment so that the stack stored after is
> - * aligned too
> - */
>   typedef struct TaskState {
>       pid_t ts_tid;     /* tid (or pid) of this task */
>   #ifdef TARGET_ARM
> @@ -160,7 +156,7 @@ typedef struct TaskState {
>   
>       /* This thread's sigaltstack, if it has one */
>       struct target_sigaltstack sigaltstack_used;
> -} __attribute__((aligned(16))) TaskState;
> +} TaskState;
>   
>   abi_long do_brk(abi_ulong new_brk);
>   

Applied to my linux-user-for-7.0 branch.

Thanks,
Laurent

[PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name

[Peter Maydell]

In commit c599d4d6d6e9bfdb64 in 2016 we renamed the old force_sig()
function to dump_core_and_abort(), but we forgot to rename the
associated tracepoint.  Rename the tracepoint to to match the
function it's called from.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c     | 2 +-
 linux-user/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index f813b4f18e4..bfbbeab9ad2 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -734,7 +734,7 @@ static void QEMU_NORETURN dump_core_and_abort(int target_sig)
     struct sigaction act;
 
     host_sig = target_to_host_signal(target_sig);
-    trace_user_force_sig(env, target_sig, host_sig);
+    trace_user_dump_core_and_abort(env, target_sig, host_sig);
     gdb_signalled(env, target_sig);
 
     /* dump core if supported by target binary format */
diff --git a/linux-user/trace-events b/linux-user/trace-events
index e7d2f54e940..f33717f248a 100644
--- a/linux-user/trace-events
+++ b/linux-user/trace-events
@@ -9,7 +9,7 @@ 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
 user_do_rt_sigreturn(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64
 user_do_sigreturn(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64
-user_force_sig(void *env, int target_sig, int host_sig) "env=%p signal %d (host %d)"
+user_dump_core_and_abort(void *env, int target_sig, int host_sig) "env=%p signal %d (host %d)"
 user_handle_signal(void *env, int target_sig) "env=%p signal %d"
 user_host_signal(void *env, int host_sig, int target_sig) "env=%p signal %d (target %d)"
 user_queue_signal(void *env, int target_sig) "env=%p signal %d"
-- 
2.25.1

Re: [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name

[Philippe Mathieu-Daudé via]

On 14/1/22 16:37, Peter Maydell wrote:
> In commit c599d4d6d6e9bfdb64 in 2016 we renamed the old force_sig()
> function to dump_core_and_abort(), but we forgot to rename the
> associated tracepoint.  Rename the tracepoint to to match the
> function it's called from.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   linux-user/signal.c     | 2 +-
>   linux-user/trace-events | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name

[Laurent Vivier]

Le 14/01/2022 à 16:37, Peter Maydell a écrit :
> In commit c599d4d6d6e9bfdb64 in 2016 we renamed the old force_sig()
> function to dump_core_and_abort(), but we forgot to rename the
> associated tracepoint.  Rename the tracepoint to to match the
> function it's called from.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   linux-user/signal.c     | 2 +-
>   linux-user/trace-events | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index f813b4f18e4..bfbbeab9ad2 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -734,7 +734,7 @@ static void QEMU_NORETURN dump_core_and_abort(int target_sig)
>       struct sigaction act;
>   
>       host_sig = target_to_host_signal(target_sig);
> -    trace_user_force_sig(env, target_sig, host_sig);
> +    trace_user_dump_core_and_abort(env, target_sig, host_sig);
>       gdb_signalled(env, target_sig);
>   
>       /* dump core if supported by target binary format */
> diff --git a/linux-user/trace-events b/linux-user/trace-events
> index e7d2f54e940..f33717f248a 100644
> --- a/linux-user/trace-events
> +++ b/linux-user/trace-events
> @@ -9,7 +9,7 @@ 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
>   user_do_rt_sigreturn(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64
>   user_do_sigreturn(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64
> -user_force_sig(void *env, int target_sig, int host_sig) "env=%p signal %d (host %d)"
> +user_dump_core_and_abort(void *env, int target_sig, int host_sig) "env=%p signal %d (host %d)"
>   user_handle_signal(void *env, int target_sig) "env=%p signal %d"
>   user_host_signal(void *env, int host_sig, int target_sig) "env=%p signal %d (target %d)"
>   user_queue_signal(void *env, int target_sig) "env=%p signal %d"

Applied to my trivial-patches branch.

Thanks,
Laurent

[PATCH 3/3] linux-user: Return void from queue_signal()

[Peter Maydell]

The linux-user queue_signal() function always returns 1, and none of
its callers check the return value.  Give it a void return type
instead.

The return value is a leftover from the old pre-2016 linux-user
signal handling code, which really did have a queue of signals and so
might return a failure indication if too many signals were queued at
once.  The current design avoids having to ever have more than one
signal queued via queue_signal() at once, so it can never fail.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal-common.h | 4 ++--
 linux-user/signal.c        | 5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
index 42aa479080b..2113165a758 100644
--- a/linux-user/signal-common.h
+++ b/linux-user/signal-common.h
@@ -59,8 +59,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
 
 void process_pending_signals(CPUArchState *cpu_env);
 void signal_init(void);
-int queue_signal(CPUArchState *env, int sig, int si_type,
-                 target_siginfo_t *info);
+void queue_signal(CPUArchState *env, int sig, int si_type,
+                  target_siginfo_t *info);
 void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info);
 void target_to_host_siginfo(siginfo_t *info, const target_siginfo_t *tinfo);
 int target_to_host_signal(int sig);
diff --git a/linux-user/signal.c b/linux-user/signal.c
index bfbbeab9ad2..32854bb3752 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -780,8 +780,8 @@ static void QEMU_NORETURN dump_core_and_abort(int target_sig)
 
 /* queue a signal so that it will be send to the virtual CPU as soon
    as possible */
-int queue_signal(CPUArchState *env, int sig, int si_type,
-                 target_siginfo_t *info)
+void queue_signal(CPUArchState *env, int sig, int si_type,
+                  target_siginfo_t *info)
 {
     CPUState *cpu = env_cpu(env);
     TaskState *ts = cpu->opaque;
@@ -794,7 +794,6 @@ int queue_signal(CPUArchState *env, int sig, int si_type,
     ts->sync_signal.pending = sig;
     /* signal that a new signal is pending */
     qatomic_set(&ts->signal_pending, 1);
-    return 1; /* indicates that the signal was queued */
 }
 
 
-- 
2.25.1

Re: [PATCH 3/3] linux-user: Return void from queue_signal()

[Philippe Mathieu-Daudé via]

On 14/1/22 16:37, Peter Maydell wrote:
> The linux-user queue_signal() function always returns 1, and none of
> its callers check the return value.  Give it a void return type
> instead.
> 
> The return value is a leftover from the old pre-2016 linux-user
> signal handling code, which really did have a queue of signals and so
> might return a failure indication if too many signals were queued at
> once.  The current design avoids having to ever have more than one
> signal queued via queue_signal() at once, so it can never fail.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   linux-user/signal-common.h | 4 ++--
>   linux-user/signal.c        | 5 ++---
>   2 files changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 3/3] linux-user: Return void from queue_signal()

[Laurent Vivier]

Le 14/01/2022 à 16:37, Peter Maydell a écrit :
> The linux-user queue_signal() function always returns 1, and none of
> its callers check the return value.  Give it a void return type
> instead.
> 
> The return value is a leftover from the old pre-2016 linux-user
> signal handling code, which really did have a queue of signals and so
> might return a failure indication if too many signals were queued at
> once.  The current design avoids having to ever have more than one
> signal queued via queue_signal() at once, so it can never fail.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   linux-user/signal-common.h | 4 ++--
>   linux-user/signal.c        | 5 ++---
>   2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
> index 42aa479080b..2113165a758 100644
> --- a/linux-user/signal-common.h
> +++ b/linux-user/signal-common.h
> @@ -59,8 +59,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>   
>   void process_pending_signals(CPUArchState *cpu_env);
>   void signal_init(void);
> -int queue_signal(CPUArchState *env, int sig, int si_type,
> -                 target_siginfo_t *info);
> +void queue_signal(CPUArchState *env, int sig, int si_type,
> +                  target_siginfo_t *info);
>   void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info);
>   void target_to_host_siginfo(siginfo_t *info, const target_siginfo_t *tinfo);
>   int target_to_host_signal(int sig);
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index bfbbeab9ad2..32854bb3752 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -780,8 +780,8 @@ static void QEMU_NORETURN dump_core_and_abort(int target_sig)
>   
>   /* queue a signal so that it will be send to the virtual CPU as soon
>      as possible */
> -int queue_signal(CPUArchState *env, int sig, int si_type,
> -                 target_siginfo_t *info)
> +void queue_signal(CPUArchState *env, int sig, int si_type,
> +                  target_siginfo_t *info)
>   {
>       CPUState *cpu = env_cpu(env);
>       TaskState *ts = cpu->opaque;
> @@ -794,7 +794,6 @@ int queue_signal(CPUArchState *env, int sig, int si_type,
>       ts->sync_signal.pending = sig;
>       /* signal that a new signal is pending */
>       qatomic_set(&ts->signal_pending, 1);
> -    return 1; /* indicates that the signal was queued */
>   }
>   
>   

Applied to my trivial-patches branch.

Thanks,
Laurent