Re: [PATCH v3 10/16] [gdb/aarch64] sme: Fixup sigframe gdbarch when vg/svg changes

Thiago Jung Bauermann via Gdb-patches
2023-07-28T01:02:18Z

Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> case AARCH64_SVE_MAGIC:
> {
> /* Check if the section is followed by a full SVE dump, and set
> sve_regs if it is. */
> gdb_byte buf[4];
> - uint16_t flags;
> -
> - if (!tdep->has_sve ())
> - break;
>
> + /* Extract the vector length. */
> if (target_read_memory (section + AARCH64_SVE_CONTEXT_VL_OFFSET,
> buf, 2) != 0)
> {
> + warning (_("Failed to read the vector length from the SVE "
> + " signal frame context."));

Extraneous space between "SVE" and "signal".

> section += size;
> break;
> }

> case AARCH64_ZA_MAGIC:
> {
> - if (!tdep->has_sme ())
> - {
> - section += size;
> - break;
> - }
> -
> /* Check if the section is followed by a full ZA dump, and set
> za_state if it is. */
> gdb_byte buf[2];
>
> + /* Extract the streaming vector length. */
> if (target_read_memory (section + AARCH64_SVE_CONTEXT_VL_OFFSET,

Same comment from the previous patch about using the SME macro instead
of the SVE one.

> buf, 2) != 0)
> {
> + warning (_("Failed to read the streaming vector length from "
> + " ZA signal frame context."));

Extraneous space between "SVE" and "signal".

> section += size;
> break;
> }

> @@ -477,11 +499,49 @@ aarch64_linux_sigframe_init (const struct tramp_frame *self,
> if (!extra_found && section > section_end)
> break;
> }
> +}
> +
> +/* Implement the "init" method of struct tramp_frame. */
> +
> +static void
> +aarch64_linux_sigframe_init (const struct tramp_frame *self,
> + frame_info_ptr this_frame,
> + struct trad_frame_cache *this_cache,
> + CORE_ADDR func)
> +{
> + /* Read the signal context information. */
> + struct aarch64_linux_sigframe signal_frame;
> + aarch64_linux_read_signal_frame_info (this_frame, signal_frame);
> +
> + /* Now we have all the data required to restore the registers from the
> + signal frame. */
> +
> + /* Restore the general purpose registers. */
> + CORE_ADDR offset
> + = signal_frame.sigcontext_address + AARCH64_SIGCONTEXT_XO_OFFSET;

You can simplify the above to

CORE_ADDR offset = signal_frame.gpr_section;

> + for (int i = 0; i < 31; i++)
> + {
> + trad_frame_set_reg_addr (this_cache, AARCH64_X0_REGNUM + i, offset);
> + offset += AARCH64_SIGCONTEXT_REG_SIZE;
> + }
> + trad_frame_set_reg_addr (this_cache, AARCH64_SP_REGNUM, offset);
> + offset += AARCH64_SIGCONTEXT_REG_SIZE;
> + trad_frame_set_reg_addr (this_cache, AARCH64_PC_REGNUM, offset);

> diff --git a/gdb/tramp-frame.h b/gdb/tramp-frame.h
> index fa0241acb2d..563c34ae595 100644
> --- a/gdb/tramp-frame.h
> +++ b/gdb/tramp-frame.h
> @@ -42,6 +42,13 @@ struct trad_frame_cache;
> instruction sequence. */
> #define TRAMP_SENTINEL_INSN ULONGEST_MAX
>
> +/* Assuming the frame chain: (outer) prev <-> this <-> next (inner);
> + use THIS frame, and implicitly the NEXT frame's register unwind
> + method, return PREV frame's architecture. */

Maybe s/return/to return/ ?

> +
> +typedef struct gdbarch *(frame_prev_arch_ftype) (frame_info_ptr this_frame,
> + void **this_prologue_cache);
> +
> struct tramp_frame
> {
> /* The trampoline's type, some a signal trampolines, some are normal
> @@ -75,6 +82,11 @@ struct tramp_frame
> int (*validate) (const struct tramp_frame *self,
> frame_info_ptr this_frame,
> CORE_ADDR *pc);
> +
> + /* Given the current frame in THIS_FRAME and a frame cache in FRAME_CACHE,
> + return the architecture of the previous frame. */
> + struct gdbarch *(*prev_arch) (frame_info_ptr this_frame,
> + void **frame_cache);

Shouldn't the typedef defined above be used here?

> };

--
Thiago