Re: [PATCH v2 2/4] arc: Recognize registers available on Linux targets

Simon Marchi
2020-05-14T15:01:46Z

Just some nits - I don't really know about the specific details of the ARC architecture.

On 2020-04-28 12:04 p.m., Shahab Vahedi via Gdb-patches wrote:
> @@ -1950,13 +1984,17 @@ arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> if (arc_debug)
> debug_printf ("arc: Architecture initialization.\n");
>
> - if (!arc_tdesc_init (info, &tdesc, &tdesc_data))
> - return NULL;
> -
> /* Allocate the ARC-private target-dependent information structure, and the
> GDB target-independent information structure. */
> struct gdbarch_tdep *tdep = XCNEW (struct gdbarch_tdep);
> tdep->jb_pc = -1; /* No longjmp support by default. */
> +
> + if (!arc_tdesc_init (info, &tdesc, &tdesc_data, tdep))
> + {
> + xfree (tdep);
> + return nullptr;
> + }
> +

I would suggest to do it in a bit more C++-y way, to even avoid the xfree:

gdb::unique_xmalloc_ptr<gdbarch_tdep> tdep (XCNEW (struct gdbarch_tdep));
tdep->jb_pc = -1; /* No longjmp support by default. */

if (!arc_tdesc_init (info, &tdesc, &tdesc_data, tdep.get ()))
return nullptr;

struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep.release ());

This way it shows clearly that this function is giving ownership of the tdep
to gdbarch_alloc.

> struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep);
>
> /* Data types. */
> @@ -1987,6 +2025,13 @@ arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> set_gdbarch_ps_regnum (gdbarch, ARC_STATUS32_REGNUM);
> set_gdbarch_fp0_regnum (gdbarch, -1); /* No FPU registers. */
>
> + /* Confirm that register name lists have proper length. */
> + gdb_static_assert (ARC_LAST_REGNUM + 1
> + == (ARRAY_SIZE (core_v2_register_names)
> + + ARRAY_SIZE (aux_minimal_register_names)));
> + gdb_static_assert (ARRAY_SIZE (core_v2_register_names)
> + == ARRAY_SIZE (core_arcompact_register_names));

Does this need to be here? I would expect them to be near (just after) the definitions
of said arrays.

Simon