Re: Suspicious QOM types without instance/class size

[Eduardo Habkost]

On Fri, Aug 21, 2020 at 11:47:32AM +1000, David Gibson 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.
> 
> 
> Comment on the ones within my area:
> > 
> > WARNING: hw/input/adb.c:310:1: class_size should be set to 
> > sizeof(ADBDeviceClass)?
> 
> Yeah, that looks like a bug (though we'll get away with it because
> it's abstract).

Right, luckily we are not touching any ADBDeviceClass field
inside adb_device_class_init().

> 
> > WARNING: hw/ppc/pnv_lpc.c:771:1: instance_size should be set to 
> > sizeof(PnvLpcController)?
> 
> Ditto.

Agreed.

> 
> Should I make fixes for these, or will you?

Please send the fixes, and I will apply them before running the
TypeInfo conversion script.

> 
> > ERROR: hw/ppc/spapr_drc.c:771:1: instance_size should be set to 
> > sizeof(SpaprDrc)?
> 
> I'm confused by this one.  I'm not exactly sure which definition is
> tripping the error, and AFAICT they should all be correctly inheriting
> instance_size from either TYPE_SPAPR_DR_CONNECTOR or
> TYPE_SPAPR_DRC_PHSYICAL.  If anything, it looks like
> TYPE_SPAPR_DRC_PHB could drop it's explicit override of instance_size.

The error is triggered because of this type checking macro at
include/hw/ppc/spapr_drc.h:

#define SPAPR_DRC_PCI(obj) OBJECT_CHECK(SpaprDrc, (obj), \
                                        TYPE_SPAPR_DRC_PCI)

The expectation is that whatever type you use in OBJECT_CHECK
will be the one used for instance_size.  The script also looks at
the parent type, to reduce false positives, but this case was
flagged because SPAPR_DRC_PCI uses SpaprDrc, but the parent type
(SPAPR_DRC_PHYSICAL) uses SpaprDrcPhysical.

Now, I don't understand why we have so many instance checker
macros that use the same typedef (SpaprDrc).  If the code needs a
valid SpaprDrc pointer, it can just use SPAPR_DR_CONNECTOR().

-- 
Eduardo