Re: [Qemu-devel] [PATCH for-next v2 01/22] object: Add OBJECT_GET_PARENT_CLASS() macro

[Andreas Färber]

Am 07.08.2013 07:53, schrieb Alexey Kardashevskiy:
> On 08/07/2013 03:45 PM, Andreas Färber wrote:
>> Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy:
>>> On 08/07/2013 02:20 PM, Peter Crosthwaite wrote:
>>>> Hi,
>>>>
>>>> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <address@hidden> 
>>>> wrote:
>>>>> On 08/06/2013 06:33 PM, Andreas Färber wrote:
>>>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy:
>>>>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote:
>>>>>>>> The object argument is currently unused and may be used to optimize the
>>>>>>>> class lookup when needed.
>>>>>>>>
>>>>>>>> Inspired-by: Peter Crosthwaite <address@hidden>
>>>>>>>> Signed-off-by: Andreas Färber <address@hidden>
>>>>>>>> ---
>>>>>>>>  include/qom/object.h | 10 ++++++++++
>>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>>>>>>> index 23fc048..a8e71dc 100644
>>>>>>>> --- a/include/qom/object.h
>>>>>>>> +++ b/include/qom/object.h
>>>>>>>> @@ -511,6 +511,16 @@ struct TypeInfo
>>>>>>>>      OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
>>>>>>>>
>>>>>>>>  /**
>>>>>>>> + * OBJECT_GET_PARENT_CLASS:
>>>>>>>> + * @obj: The object to obtain the parent class for.
>>>>>>>> + * @name: The QOM typename of @obj.
>>>>>>>> + *
>>>>>>>> + * Returns the parent class for a given object of a specific class.
>>>>>>>> + */
>>>>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \
>>>>>>>> +    object_class_get_parent(object_class_by_name(name))
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>>   * InterfaceInfo:
>>>>>>>>   * @type: The name of the interface.
>>>>>>>>   *
>>>>>>>>
>>>>>>>
>>>>>>> Has anyone ever tried to use this macro?
>>>>>>
>>>>>> Since you're asking me, obviously later in this virtio series it's used
>>>>>> and in the IndustryPack series as well.
>>>>>>
>>>>>> I'm not aware of anyone else having used it yet - I'm still waiting for
>>>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it
>>>>>> on qom-next.
>>>>>
>>>>>
>>>>> Still do not understand what "obj" is doing here.
>>>>
>>>> The object is currently where cast cache optimizations are
>>>> implemented. So having a handle to it is useful if ever these
>>>> get-parent operations end up in fast paths and we need to do one of
>>>> Anthony's caching tricks.
>>>>
>>>>> This what I would suggest:
>>>>>
>>>>> #define OBJECT_GET_PARENT_CLASS(obj, name) \
>>>>>     object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name)))
>>>>>
>>>>
>>>> You have changed the semantic from what Andreas has implemented. This
>>>> will always return the parent of the concrete class, whereas to solve
>>>> the save-override-call problem you need to get the parent of abstract
>>>> level that is overriding the function (not always the concrete class).
>>>>
>>>>> @name here is just to make sure we are at the right level of the class
>>>>> hierarchy.
>>>>>
>>>>
>>>> Its @name that is actually important. Its more than just assertive,
>>>> variation of @name for the same object will and should return
>>>> different results (aside from just checking). The demonstrative case
>>>> for this requirement is TYPE_ARM_CPU, which has a realize fn that
>>>> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs
>>>> however have a whole bunch of concrete classes inheriting from
>>>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only
>>>> be able to get a handle to the parent of the concrete class (i.e.
>>>> TYPE_ARM_CPU) 
>>>
>>> This is what I would really (really) expect in this situation.
>>>
>>>> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as
>>>> intended.
>>>
>>> Oh. Then it is not get_parent() at all, this is get_grand_parent.
>>
>> No. It is parent of TYPE_ARM_CPU, as specified by the name argument.
> 
> Here I lost you. You really want finalize() of TYPE_CPU, no matter how many
> classes are between TYPE_CPU and your final ARM CPU class.

Correct.

> So call it
> directly, why do you need this workaround with get_parent if it is not
> really a parent of the current obj and you do not trust to what you get in
> finalize callback field of all intermediate classes (TYPE_CPU_ARM in this
> example)?
> 
> Again, I am not arguing, I really that dump and do not understand :)

This macro represents Java's "super" or C#'s "base" (which refer to the
method's parent type, not the object pointer's) rather than C++'s
SomeType:: (which refers to some concrete type down the
multi-inheritence chain and is compile-checked in C++ but is independent
of whether it's an immediate or indirect parent type). I.e.,

class ArmCpu {
   protected override void realize(DeviceState dev, ref Error err) {
       base.realize(dev, err);
       ...
   }
}

rather than

void ARMCPU::realize(DeviceState *dev, Error &err)
{
    CPU::realize(dev, err); // what if we get a 32bitCPU parent?
    ...
}

>>> realize() of TYPE_ARM_CPU_CORTEXTA9 (example) calls its parent
>>> (TYPE_ARM_CPU) realize() and that realize() calls its parent (TYPE_CPU)
>>> realize.
>>>
>>> It some level (for example, TYPE_ARM_CPU) does not want to implement
>>> realize(), then pointer to realize() from the upper class should be copied
>>> into TYPE_ARM_CPU's type info struct. class_init() callbacks are called for
>>> every class in the chain, so if some class_init() won't write to realize()
>>> pointer, it should be what the parent initialized it to, no?
>>>
>>> What do I miss here?
>>
>> You are missing that we do implement realize for TYPE_ARM_CPU, not for
>> its derived types. So we want to call TYPE_CPU's realize from there, not
>> some random realize determined by what type dev happens to have.
> 
> How is it random? Inheritance is strict. It is CPU -> ARM_CPU -> A9_ARM_CPU
> so their finalize() should point to something reasonable or inherit from a
> parent.

Inheritence can change by modifying .parent in TypeInfo. I have done so
several times for 1.6 to allow proper FOO() casts for types sharing a
state struct and you have done it for common XICS, too, so it is not
just theory. If that happens, not relying on the parent type's constant
across code is beneficial; as long as the type a function is for does
not change (i.e., not xics_realize -> common_xics_realize) then the
macro can be reused unchanged. If you hardcode TYPE_* inline, then
someone needs to go through and check all that code, making review and
changes harder.

In case you were not aware, read the documentation Peter Ch. just fixed
on overriding QOM methods in include/qom/object.h to see how Anthony
originally asked us to do things. This macro (and its predecessors) try
to simplify that process by avoiding adding *Class classes per type
GObject-style, which does not scale well for leaf types (such as
cortex-a9-arm-cpu).
If Anthony does not object to this approach, then we should update the
documentation and I should pick up and queue Peter Cr.'s ISA and ARM
patches.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg