Re: [PATCH v2 04/11] gdbserver: convert init_register_cache and new_register_cache into constructors

Simon Marchi
2025-01-07T04:51:53Z

> @@ -112,55 +112,38 @@ regcache_invalidate (void)
>
> #endif
>
> -struct regcache *
> -init_register_cache (struct regcache *regcache,
> - const struct target_desc *tdesc,
> - unsigned char *regbuf)
> +regcache::regcache (const target_desc *tdesc,
> + unsigned char *regbuf)
> {
> - if (regbuf == NULL)
> - {
> -#ifndef IN_PROCESS_AGENT
> - /* Make sure to zero-initialize the register cache when it is
> - created, in case there are registers the target never
> - fetches. This way they'll read as zero instead of
> - garbage. */
> - regcache->tdesc = tdesc;
> - regcache->registers
> - = (unsigned char *) xcalloc (1, tdesc->registers_size);
> - regcache->registers_owned = true;
> - regcache->register_status
> - = (unsigned char *) xmalloc (tdesc->reg_defs.size ());
> - memset ((void *) regcache->register_status, REG_UNAVAILABLE,
> - tdesc->reg_defs.size ());
> -#else
> - gdb_assert_not_reached ("can't allocate memory from the heap");
> -#endif
> - }
> - else
> - {
> - regcache->tdesc = tdesc;
> - regcache->registers = regbuf;
> - regcache->registers_owned = false;
> + gdb_assert (regbuf != nullptr);
> +
> + this->tdesc = tdesc;
> + this->registers = regbuf;
> + this->registers_owned = false;
> #ifndef IN_PROCESS_AGENT
> - regcache->register_status = NULL;
> + this->register_status = nullptr;
> #endif

These could be initialized in an initialization list.

> - }
> -
> - regcache->registers_fetched = false;
> -
> - return regcache;
> + this->registers_fetched = false;
> }
>
> #ifndef IN_PROCESS_AGENT
>
> -struct regcache *
> -new_register_cache (const struct target_desc *tdesc)
> +regcache::regcache (const target_desc *tdesc)
> {
> - struct regcache *regcache = new struct regcache;
> -
> gdb_assert (tdesc->registers_size != 0);
>
> - return init_register_cache (regcache, tdesc, NULL);
> + /* Make sure to zero-initialize the register cache when it is
> + created, in case there are registers the target never
> + fetches. This way they'll read as zero instead of
> + garbage. */
> + this->tdesc = tdesc;
> + this->registers
> + = (unsigned char *) xcalloc (1, tdesc->registers_size);
> + this->registers_owned = true;
> + this->register_status
> + = (unsigned char *) xmalloc (tdesc->reg_defs.size ());

Same here.

> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index 12345a3439cd7dedc70cd06f13450dde58214124..ea6c26a37a07bf6d336d6bdf327d543fae0b6370 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -45,7 +45,13 @@ struct regcache : public reg_buffer_common
> #ifndef IN_PROCESS_AGENT
> /* One of REG_UNAVAILABLE or REG_VALID. */
> unsigned char *register_status = nullptr;
> +
> + /* Constructors. */
> + regcache (const target_desc *tdesc);
> #endif
> + regcache (const target_desc *tdesc, unsigned char *regbuf);

I don't think the "Constructors" is that helpful. On the other hand, I
would appreciate a comment saying what the constructor with `regbuf`
does differently from the other one (I guess use that buffer to store
register data instead of dynamically allocating?).

> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
> index 1b6e9d05273ea7a709f1880473c070c59169ddd5..9a00d1f272f905cadbc87265ed0be3d87f7b54df 100644
> --- a/gdbserver/tracepoint.cc
> +++ b/gdbserver/tracepoint.cc
> @@ -1288,7 +1288,7 @@ struct tracepoint_hit_ctx
> struct fast_tracepoint_ctx : public tracepoint_hit_ctx
> {
> fast_tracepoint_ctx (unsigned char *_regs)
> - : regcache_initted (0), regs (_regs)
> + : regcache (), regs (_regs)

`regcache ()` becomes unnecessary.

Simon