Re: [PATCH 1/3] user-mode: Use QEMU_ALIGNED() on TaskState structure

[Peter Maydell]

On Thu, 4 Mar 2021 at 15:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The rest of the codebase uses the QEMU_ALIGNED() introduced in
> commit 911a4d2215b ("compiler.h: add QEMU_ALIGNED() to enforce
> struct alignment"). Use it for the TaskState structure too.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  bsd-user/qemu.h   | 2 +-
>  linux-user/qemu.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index d2bcaab7413..1ec14010216 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -96,7 +96,7 @@ typedef struct TaskState {
>      int signal_pending; /* non zero if a signal may be pending */
>
>      uint8_t stack[];
> -} __attribute__((aligned(16))) TaskState;
> +} QEMU_ALIGNED(16) TaskState;
>
>  void init_task_state(TaskState *ts);
>  extern const char *qemu_uname_release;
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 52c981710b4..d7815bfb845 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -157,7 +157,7 @@ typedef struct TaskState {
>
>      /* This thread's sigaltstack, if it has one */
>      struct target_sigaltstack sigaltstack_used;
> -} __attribute__((aligned(16))) TaskState;
> +} QEMU_ALIGNED(16) TaskState;

Does this struct need to be 16-aligned these days? When it 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.

So I think that at least for linux-user the alignment attribute
is no longer needed.

bsd-user's struct still has the 'stack' field but as far
as I can tell it is never used, so it and the alignment
could be deleted there too. (bsd-user is missing the changes
that 48e15fc2d makes for linux-user, but less harmfully since
it appears not to implement either threads or fork (?!). In
any case the plan at the moment is to blow away bsd-user/
entirely so spending much thought on it is wasted effort.)

thanks
-- PMM