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

[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. 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 :)


> obj serves for possible optimization, as demonstrated by an earlier
> patch by Peter that avoids the hashtable lookup by iterating over obj's
> parent classes. If we pass obj down through our macros then such changes
> can be done in one central place rather than reviewing and changing the
> whole code base.

>> 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.


-- 
Alexey