Re: [PATCH v5 04/18] gdbstub: DebugClass added to system mode.

["Alex Bennée" <alex.bennee@linaro.org>]

Nicolas Eder <nicolas.eder@lauterbach.com> writes:

> This class is used to abstract debug features between different debuggers
> ---
>  debug/common/debug.c     | 33 +++++++++++++++++++++++++++++++++
>  debug/common/meson.build |  1 +
>  debug/gdbstub/system.c   | 18 ++++++++++++++++++
>  debug/meson.build        |  1 +
>  include/hw/boards.h      |  1 +
>  include/qemu/debug.h     | 20 ++++++++++++++++++++
>  include/qemu/typedefs.h  |  2 ++
>  system/cpus.c            |  9 ++++++++-
>  8 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/debug/common/debug.c b/debug/common/debug.c
> index c24aaf1202..476c969c98 100644
> --- a/debug/common/debug.c
> +++ b/debug/common/debug.c
> @@ -16,3 +16,36 @@
>   *
>   * SPDX-License-Identifier: LGPL-2.0+
>   */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/debug.h"
> +#include "qom/object_interfaces.h"
> +
> +static void debug_instance_init(Object *obj)
> +{
> +}
> +
> +static void debug_finalize(Object *obj)
> +{
> +}
> +
> +static void debug_class_init(ObjectClass *oc, void *data)
> +{
> +}
> +
> +static const TypeInfo debug_info = {
> +    .name = TYPE_DEBUG,
> +    .parent = TYPE_OBJECT,
> +    .instance_size = sizeof(DebugState),
> +    .instance_init = debug_instance_init,
> +    .instance_finalize = debug_finalize,
> +    .class_size = sizeof(DebugClass),
> +    .class_init = debug_class_init
> +};
> +
> +static void debug_register_types(void)
> +{
> +    type_register_static(&debug_info);
> +}

You shouldn't need empty functions if you are not using them. Moreover
you should use the inheritance feature and have something like:

static void gdb_debug_class_init(ObjectClass *klass, void *data)
{
    DebugClass *dc = DEBUG_CLASS(klass);
    dc->set_stop_cpu = gdb_set_stop_cpu;
};

static const TypeInfo debug_info[] = {
    {
        .name = TYPE_DEBUG,
        .parent = TYPE_OBJECT,
        .instance_size = sizeof(DebugState),
        .class_size = sizeof(DebugClass),
        .abstract = true,
    },
    {
        .name = TYPE_GDB_DEBUG,
        .parent = TYPE_DEBUG,
        .class_init = gdb_debug_class_init,
    },
};

DEFINE_TYPES(debug_info)


> +
<snip>
>  
> +/**
> + * gdb_init_debug_class() - initialize gdb-specific DebugClass
> + */
> +static void gdb_init_debug_class(void)
> +{
> +    Object *obj;
> +    obj = object_new(TYPE_DEBUG);
> +    DebugState *ds = DEBUG(obj);
> +    DebugClass *dc = DEBUG_GET_CLASS(ds);
> +    dc->set_stop_cpu = gdb_set_stop_cpu;

This should be part of the class init above

> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    ms->debug_state = ds;
> +}
>          }
>      } else {
> -        gdb_set_stop_cpu(cpu);
> +        MachineState *ms = MACHINE(qdev_get_machine());
> +        DebugState *ds = ms->debug_state;
> +        DebugClass *dc = DEBUG_GET_CLASS(ds);
> +
> +        if (dc->set_stop_cpu) {
> +            dc->set_stop_cpu(cpu);
> +        }

If there is no explicit state perhaps we should just save the class
rather than the instance.

>          qemu_system_debug_request();
>          cpu->stopped = true;
>      }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro