Re: [PATCH v2 03/11] gdbserver: use inheritance more to define tracepoint contexts

Simon Marchi
2025-01-07T04:41:32Z

On 2024-12-30 05:49, Tankut Baris Aktemur wrote:
> This is a continuation of the previous refactoring to use inheritance
> in the definition of tracepoints contexts. Again, no behavioral change
> is intended.
>
> Different tracepoint contexts are identified by the `type` field. The
> field is used only in `get_context_regcache`, where we essentially
> have 3 cases, each corresponding to a tracepoint context type. Remove
> the `type` field and split the `get_context_regcache` function into 3
> virtual method implementations.
>
> Tested by rebuilding gdbserver with '--enable-inprocess-agent=no' and
> '--enable-inprocess-agent=yes'.
> ---
> gdbserver/tracepoint.cc | 95 +++++++++++++++++++++++++------------------------
> 1 file changed, 48 insertions(+), 47 deletions(-)
>
> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
> index fc0586a5c5ace6edc1fc72d24511c6f36e97f759..1b6e9d05273ea7a709f1880473c070c59169ddd5 100644
> --- a/gdbserver/tracepoint.cc
> +++ b/gdbserver/tracepoint.cc
> @@ -1278,7 +1278,7 @@ static char *tracing_stop_note;
> collect_data_at_tracepoint. */
> struct tracepoint_hit_ctx
> {
> - enum tracepoint_type type;
> + virtual struct regcache *get_regcache () = 0;

I would name this method just `regcache ()`, I try to push not to have
the "get_" prefix for getters. Methods without a verb are implicitly
getters.

> };
>
> #ifdef IN_PROCESS_AGENT
> @@ -1290,9 +1290,10 @@ struct fast_tracepoint_ctx : public tracepoint_hit_ctx
> fast_tracepoint_ctx (unsigned char *_regs)
> : regcache_initted (0), regs (_regs)
> {
> - type = fast_tracepoint;
> }
>
> + virtual struct regcache *get_regcache () override;

I think you could move the implementation of those methods here, it's
all in the same .cc file anyway. And it would help with readability,
given the mess of ifdefs below.

> +struct regcache *
> +static_tracepoint_ctx::get_regcache ()
> +{
> #ifdef HAVE_UST
> - if (ctx->type == static_tracepoint)
> - {
> - auto sctx = static_cast <static_tracepoint_ctx *> (ctx);
> + const struct target_desc *ipa_tdesc = get_ipa_tdesc (ipa_tdesc_idx);
>
> - if (!sctx->regcache_initted)
> - {
> - sctx->regcache_initted = 1;
> - init_register_cache (&sctx->regcache, ipa_tdesc, sctx->regspace);
> - supply_regblock (&sctx->regcache, NULL);
> - /* Pass down the tracepoint address, because REGS doesn't
> - include the PC, but we know what it must have been. */
> - supply_static_tracepoint_registers (&sctx->regcache,
> - (const unsigned char *)
> - sctx->regs,
> - sctx->tpoint->address);
> - }
> - regcache = &sctx->regcache;
> - }
> -#endif
> -#else
> - if (ctx->type == trap_tracepoint)
> + if (!this->regcache_initted)
> {
> - auto tctx = static_cast<trap_tracepoint_ctx *> (ctx);
> - regcache = tctx->regcache;
> + this->regcache_initted = 1;
> + init_register_cache (&this->regcache, ipa_tdesc, this->regspace);
> + supply_regblock (&this->regcache, nullptr);
> + /* Pass down the tracepoint address, because REGS doesn't
> + include the PC, but we know what it must have been. */
> + supply_static_tracepoint_registers (&this->regcache,
> + (const unsigned char *)
> + this->regs,
> + this->tpoint->address);
> }
> +
> + return &this->regcache;
> +#else
> + gdb_assert_not_reached ("static tracepoint feature requires UST");
> #endif

I think that static_tracepoint_ctx (and therefore this method) could
simply not exist at all if !HAVE_UST? Although if you remove UST
support, the question won't matter anymore.

Simon