Re: Suspicious QOM types without instance/class size

[Eduardo Habkost]

On Fri, Aug 21, 2020 at 01:53:52PM +0300, Roman Bolshakov wrote:
> On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote:
> > While trying to convert TypeInfo declarations to the new
> > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases
> > where instance_size or class_size is not set, despite having type
> > checker macros that use a specific type.
> > 
> > The ones with "WARNING" are abstract types (maybe not serious if
> > subclasses set the appropriate sizes).  The ones with "ERROR"
> > don't seem to be abstract types.
> > 
> 
> > ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to 
> > sizeof(HVFState)?
> 
> Hi Eduardo,
> 
> How do you get the error?

My script looks for corresponding type checking macros, and check
if instance_size is set to sizeof(T) with the right type from the
type checking macro.

The code is here:
https://github.com/ehabkost/qemu-hacks/blob/920b2c521ad2a29fa663256854e24ed2059ba9cd/scripts/codeconverter/codeconverter/qom_type_info.py#L136


> 
> Given your changes, instance size should really be sizeof(HVFState).
> 

The changes I've made shouldn't make any difference (if there's
an issue, it is there before or after my series).

> BTW, the object definition for hvf seems different from KVM (and perhaps
> wrong?), e.g. HVFState is allocated within init_machine handler and then
> assigned to a global variable:

Interesting.  It looks like hvf_state is _not_ the actual QOM
object instance.  The actual TYPE_HVF_ACCEL instance is created
by do_configure_accelerator().  That would explain why the lack
of instance_init never caused any problems.

Luckily, no code ever used the HVF_STATE macro.  If
HVF_STATE(hvf_state) got called, it would crash because of
uninitialized object instance data.  If HVF_STATE(machine->accel)
got called, it would return an invalid HVFState pointer (not
hvf_state).

I believe the simplest short term solution here is to just delete
the HVF_STATE macro and HVFState::parent field.  We can worry
about actually moving hvf_state to the machine->accel QOM object
later.

> 
> static int hvf_accel_init(MachineState *ms)
> {
>     int x;
>     hv_return_t ret;
>     HVFState *s;
> 
>     ret = hv_vm_create(HV_VM_DEFAULT);
>     assert_hvf_ok(ret);
> 
>     s = g_new0(HVFState, 1);
>  
>     s->num_slots = 32;
>     for (x = 0; x < s->num_slots; ++x) {
>         s->slots[x].size = 0;
>         s->slots[x].slot_id = x;
>     }
>   
>     hvf_state = s;
>     cpu_interrupt_handler = hvf_handle_interrupt;
>     memory_listener_register(&hvf_memory_listener, &address_space_memory);
>     return 0;
> }
> 
> static void hvf_accel_class_init(ObjectClass *oc, void *data)
> {
>     AccelClass *ac = ACCEL_CLASS(oc);
>     ac->name = "HVF";
>     ac->init_machine = hvf_accel_init;
>     ac->allowed = &hvf_allowed;
> }
> 
> static const TypeInfo hvf_accel_type = {
>     .name = TYPE_HVF_ACCEL,
>     .parent = TYPE_ACCEL,
>     .class_init = hvf_accel_class_init,
> };
> 
> Thanks,
> Roman
> 

-- 
Eduardo