From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37980) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6xEz-0003gl-NH for qemu-devel@nongnu.org; Wed, 07 Aug 2013 02:29:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6xEu-0008CA-LA for qemu-devel@nongnu.org; Wed, 07 Aug 2013 02:29:01 -0400 Received: from cantor2.suse.de ([195.135.220.15]:42256 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6xEu-0008Bz-80 for qemu-devel@nongnu.org; Wed, 07 Aug 2013 02:28:56 -0400 Message-ID: <5201E922.6030807@suse.de> Date: Wed, 07 Aug 2013 08:28:50 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1375323463-32747-1-git-send-email-afaerber@suse.de> <1375323463-32747-2-git-send-email-afaerber@suse.de> <52008FAE.8060709@ozlabs.ru> <5200B4C1.9010008@suse.de> <5201C0CA.2070808@ozlabs.ru> <5201DD73.1080001@ozlabs.ru> <5201DEDE.1020901@suse.de> <5201E0D8.3040604@ozlabs.ru> In-Reply-To: <5201E0D8.3040604@ozlabs.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-next v2 01/22] object: Add OBJECT_GET_PARENT_CLASS() macro List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Peter Crosthwaite , Peter Chubb , qemu-devel@nongnu.org, Anthony Liguori Am 07.08.2013 07:53, schrieb Alexey Kardashevskiy: > On 08/07/2013 03:45 PM, Andreas F=C3=A4rber 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 = wrote: >>>>> On 08/06/2013 06:33 PM, Andreas F=C3=A4rber wrote: >>>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>>>>> On 08/01/2013 12:17 PM, Andreas F=C3=A4rber wrote: >>>>>>>> The object argument is currently unused and may be used to optim= ize the >>>>>>>> class lookup when needed. >>>>>>>> >>>>>>>> Inspired-by: Peter Crosthwaite >>>>>>>> Signed-off-by: Andreas F=C3=A4rber >>>>>>>> --- >>>>>>>> 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)), na= me) >>>>>>>> >>>>>>>> /** >>>>>>>> + * 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 cl= ass. >>>>>>>> + */ >>>>>>>> +#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 waitin= g for >>>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I p= ut 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), (n= ame))) >>>>> >>>> >>>> You have changed the semantic from what Andreas has implemented. Thi= s >>>> will always return the parent of the concrete class, whereas to solv= e >>>> the save-override-call problem you need to get the parent of abstrac= t >>>> 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 cl= ass >>>>> 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 CPU= s >>>> however have a whole bunch of concrete classes inheriting from >>>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will on= ly >>>> be able to get a handle to the parent of the concrete class (i.e. >>>> TYPE_ARM_CPU)=20 >>> >>> 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. >=20 > 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 th= is > example)? >=20 > 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_CP= U) >>> 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 c= opied >>> into TYPE_ARM_CPU's type info struct. class_init() callbacks are call= ed for >>> every class in the chain, so if some class_init() won't write to real= ize() >>> 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, n= ot >> some random realize determined by what type dev happens to have. >=20 > 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 fro= m 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg