* [PATCH 0/3] linux-user: Fix some minor nits
@ 2022-01-14 15:37 Peter Maydell
2022-01-14 15:37 ` [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState Peter Maydell
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Peter Maydell @ 2022-01-14 15:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState
2022-01-14 15:37 [PATCH 0/3] linux-user: Fix some minor nits Peter Maydell
@ 2022-01-14 15:37 ` Peter Maydell
2022-01-27 13:20 ` Laurent Vivier
2022-01-14 15:37 ` [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name Peter Maydell
2022-01-14 15:37 ` [PATCH 3/3] linux-user: Return void from queue_signal() Peter Maydell
2 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2022-01-14 15:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name
2022-01-14 15:37 [PATCH 0/3] linux-user: Fix some minor nits Peter Maydell
2022-01-14 15:37 ` [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState Peter Maydell
@ 2022-01-14 15:37 ` Peter Maydell
2022-01-14 15:46 ` Philippe Mathieu-Daudé via
2022-01-18 11:41 ` Laurent Vivier
2022-01-14 15:37 ` [PATCH 3/3] linux-user: Return void from queue_signal() Peter Maydell
2 siblings, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2022-01-14 15:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] linux-user: Return void from queue_signal()
2022-01-14 15:37 [PATCH 0/3] linux-user: Fix some minor nits Peter Maydell
2022-01-14 15:37 ` [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState Peter Maydell
2022-01-14 15:37 ` [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name Peter Maydell
@ 2022-01-14 15:37 ` Peter Maydell
2022-01-14 15:57 ` Philippe Mathieu-Daudé via
2022-01-18 11:42 ` Laurent Vivier
2 siblings, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2022-01-14 15:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name
2022-01-14 15:37 ` [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name Peter Maydell
@ 2022-01-14 15:46 ` Philippe Mathieu-Daudé via
2022-01-18 11:41 ` Laurent Vivier
1 sibling, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-01-14 15:46 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Laurent Vivier
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] linux-user: Return void from queue_signal()
2022-01-14 15:37 ` [PATCH 3/3] linux-user: Return void from queue_signal() Peter Maydell
@ 2022-01-14 15:57 ` Philippe Mathieu-Daudé via
2022-01-18 11:42 ` Laurent Vivier
1 sibling, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-01-14 15:57 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: Laurent Vivier
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name
2022-01-14 15:37 ` [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name Peter Maydell
2022-01-14 15:46 ` Philippe Mathieu-Daudé via
@ 2022-01-18 11:41 ` Laurent Vivier
1 sibling, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2022-01-18 11:41 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] linux-user: Return void from queue_signal()
2022-01-14 15:37 ` [PATCH 3/3] linux-user: Return void from queue_signal() Peter Maydell
2022-01-14 15:57 ` Philippe Mathieu-Daudé via
@ 2022-01-18 11:42 ` Laurent Vivier
1 sibling, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2022-01-18 11:42 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState
2022-01-14 15:37 ` [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState Peter Maydell
@ 2022-01-27 13:20 ` Laurent Vivier
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2022-01-27 13:20 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-27 13:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-14 15:37 [PATCH 0/3] linux-user: Fix some minor nits Peter Maydell
2022-01-14 15:37 ` [PATCH 1/3] linux-user: Remove unnecessary 'aligned' attribute from TaskState Peter Maydell
2022-01-27 13:20 ` Laurent Vivier
2022-01-14 15:37 ` [PATCH 2/3] linux-user: Rename user_force_sig tracepoint to match function name Peter Maydell
2022-01-14 15:46 ` Philippe Mathieu-Daudé via
2022-01-18 11:41 ` Laurent Vivier
2022-01-14 15:37 ` [PATCH 3/3] linux-user: Return void from queue_signal() Peter Maydell
2022-01-14 15:57 ` Philippe Mathieu-Daudé via
2022-01-18 11:42 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.