[Qemu-devel] [RFC 0/2] Allow QOM struct fields to be marked as private

[Qemu-devel] [RFC 0/2] Allow QOM struct fields to be marked as private

[Peter Maydell]

This patch series provides infrastructure and documentation
for marking QOM struct fields as private to the class implementation.

Patch 1 is the implementation (which is a trivial five lines!) and
documentation of the code pattern that it's intended to be used with.

Patch 2 is an example of its use in the ARM GIC classes. This
patch depends on Andreas' "A9MPCore+A15MPCore QOM'ification" v2
patchset, really just because there aren't any examples in the
tree at the moment of devices which I understand and which use the
"include file with device struct which is included by both
implementation and object users" pattern.

A .h file which uses this pattern ends up with half a dozen
extra lines of boilerplate, which is slightly sad but not too
awful. It would be pretty easy to autogenerate (along with the
type macros themselves) if we decided to do that in future, though.

I'm not hugely attached to either the "__private" or IMPLEMENTING_FOO
naming conventions, so feel free to bikeshed them. (I am aware that
the former is treading on the implementation reserved namespace...)

Incidentally, I think I would be in favour of (as a coding style
issue) having exactly one class per .h file, which the gic doesn't
currently do (it puts both TYPE_ARM_GIC_COMMON and TYPE_ARM_GIC in
one file).


Peter Maydell (2):
  Provide infrastructure for marking private QOM struct fields
  arm_gic: Use new __private macro to mark private fields

 hw/intc/arm_gic.c         |    3 +++
 hw/intc/arm_gic_common.c  |    2 ++
 hw/intc/arm_gic_kvm.c     |    2 ++
 hw/intc/armv7m_nvic.c     |    2 ++
 include/hw/intc/arm_gic.h |   64 ++++++++++++++++++++++++++++-----------------
 include/qemu/compiler.h   |   10 +++++++
 include/qom/object.h      |   47 +++++++++++++++++++++++++++++++++
 7 files changed, 106 insertions(+), 24 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [RFC 1/2] Provide infrastructure for marking private QOM struct fields

[Peter Maydell]

Provide infrastructure for marking private QOM struct fields,
so that a compiler warning is generated when a user of the QOM
object attempts to access them directly.

This is implemented using GCC's 'deprecated' attribute; preprocessor
macros arrange that when compiling the class implementation,
no attribute is applied to the fields; when compiling a user
of the class the fields are marked deprecated.

This allows us to have a single simple C struct defining the
object, and for users of the QOM object to be able to embed
instances of it into other structs, but still to have a guard
against users accidentally touching parts of the structure
they should not be accessing.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qemu/compiler.h |   10 ++++++++++
 include/qom/object.h    |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 155b358..d7cc153 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -52,4 +52,14 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+/* An attribute usable to mark structure fields as private to the
+ * implementation; since this is only a diagnostic to catch programming
+ * errors, it's OK if it expands to nothing on non-gcc compilers.
+ */
+#if defined __GNUC__
+# define QEMU_PRIVATE_ATTR __attribute__((deprecated("this field is private")))
+#else
+# define QEMU_PRIVATE_ATTR
+#endif
+
 #endif /* COMPILER_H */
diff --git a/include/qom/object.h b/include/qom/object.h
index 23fc048..7f02f80 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -284,6 +284,53 @@ typedef struct InterfaceInfo InterfaceInfo;
  *
  * The first example of such a QOM method was #CPUClass.reset,
  * another example is #DeviceClass.realize.
+ *
+ * = Marking fields as private to the class implementation =
+ *
+ * The expected code structure for QOM objects is that they should
+ * have a header file in include/ which defines the class and object
+ * structures and the typecasting macros. This header can then be
+ * included by both the source file which implements the QOM object
+ * and also by other source files which merely wish to use the object.
+ * Users of your object need the class and object structures so that
+ * they can embed instances of the object in their own structures;
+ * however they do not need to be able to access individual fields in
+ * these structures. To enforce this you should use the QEMU_PRIVATE_ATTR
+ * macro in a pattern like this:
+ *
+ * <example>
+ *   <title>Marking fields as private</title>
+ *   <programlisting>
+ * #ifdef IMPLEMENTING_MY_DEVICE
+ * # define __private
+ * #else
+ * # define __private QEMU_PRIVATE_ATTR
+ * #endif
+ *
+ * typedef struct MyDevice
+ * {
+ *     __private DeviceState parent;
+ *
+ *     __private int reg0, reg1, reg2;
+ * } MyDevice;
+ *
+ * typedef struct MyDeviceClass
+ * {
+ *     __private DeviceClass parent;
+ *
+ *     void (*frobnicate) (MyDevice *obj);
+ * } MyDeviceClass;
+ *
+ * #undef __private
+ *   </programlisting>
+ * </example>
+ *
+ * The source files which provide the implementation of your
+ * class (or of subclasses to it) should then have
+ * "#define IMPLEMENTING_MY_DEVICE" before they include any
+ * headers. Since users of the class will not define this
+ * macro, they will get a compilation warning if they access
+ * any of the private fields by mistake.
  */
 
 
-- 
1.7.9.5

[Qemu-devel] [RFC 2/2] arm_gic: Use new __private macro to mark private fields

[Peter Maydell]

Use the new __private macro infrastructure to mark private fields for
the arm_gic classes.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c         |    3 +++
 hw/intc/arm_gic_common.c  |    2 ++
 hw/intc/arm_gic_kvm.c     |    2 ++
 hw/intc/armv7m_nvic.c     |    2 ++
 include/hw/intc/arm_gic.h |   64 ++++++++++++++++++++++++++++-----------------
 5 files changed, 49 insertions(+), 24 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 8e34004..cbe7d27 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -18,6 +18,9 @@
  *  armv7m_nvic device.
  */
 
+#define IMPLEMENTING_ARM_GIC_COMMON
+#define IMPLEMENTING_ARM_GIC
+
 #include "hw/sysbus.h"
 #include "gic_internal.h"
 #include "qom/cpu.h"
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index a89c786..c27c5c1 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -18,6 +18,8 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#define IMPLEMENTING_ARM_GIC_COMMON
+
 #include "gic_internal.h"
 
 static void gic_pre_save(void *opaque)
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index f713975..0acd676 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -18,6 +18,8 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#define IMPLEMENTING_ARM_GIC_COMMON
+
 #include "hw/sysbus.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 178344b..54d62fe 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -10,6 +10,8 @@
  * NVIC.  Much of that is also implemented here.
  */
 
+#define IMPLEMENTING_ARM_GIC_COMMON
+
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "hw/arm/arm.h"
diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
index be945ec..d978d72 100644
--- a/include/hw/intc/arm_gic.h
+++ b/include/hw/intc/arm_gic.h
@@ -40,36 +40,42 @@ typedef struct gic_irq_state {
     bool trigger; /* nonzero = edge triggered.  */
 } gic_irq_state;
 
+#ifdef IMPLEMENTING_ARM_GIC_COMMON
+#define __private
+#else
+#define __private QEMU_PRIVATE_ATTR
+#endif
+
 typedef struct GICState {
     /*< private >*/
-    SysBusDevice busdev;
+    __private SysBusDevice busdev;
     /*< public >*/
 
-    qemu_irq parent_irq[GIC_NCPU];
-    bool enabled;
-    bool cpu_enabled[GIC_NCPU];
+    __private qemu_irq parent_irq[GIC_NCPU];
+    __private bool enabled;
+    __private bool cpu_enabled[GIC_NCPU];
 
-    gic_irq_state irq_state[GIC_MAXIRQ];
-    uint8_t irq_target[GIC_MAXIRQ];
-    uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
-    uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
-    uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
+    __private gic_irq_state irq_state[GIC_MAXIRQ];
+    __private uint8_t irq_target[GIC_MAXIRQ];
+    __private uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
+    __private uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
+    __private uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
 
-    uint16_t priority_mask[GIC_NCPU];
-    uint16_t running_irq[GIC_NCPU];
-    uint16_t running_priority[GIC_NCPU];
-    uint16_t current_pending[GIC_NCPU];
+    __private uint16_t priority_mask[GIC_NCPU];
+    __private uint16_t running_irq[GIC_NCPU];
+    __private uint16_t running_priority[GIC_NCPU];
+    __private uint16_t current_pending[GIC_NCPU];
 
-    uint32_t num_cpu;
+    __private uint32_t num_cpu;
 
-    MemoryRegion iomem; /* Distributor */
+    __private MemoryRegion iomem; /* Distributor */
     /* This is just so we can have an opaque pointer which identifies
      * both this GIC and which CPU interface we should be accessing.
      */
-    struct GICState *backref[GIC_NCPU];
-    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
-    uint32_t num_irq;
-    uint32_t revision;
+    __private struct GICState *backref[GIC_NCPU];
+    __private MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
+    __private uint32_t num_irq;
+    __private uint32_t revision;
 } GICState;
 
 #define TYPE_ARM_GIC_COMMON "arm_gic_common"
@@ -82,13 +88,21 @@ typedef struct GICState {
 
 typedef struct ARMGICCommonClass {
     /*< private >*/
-    SysBusDeviceClass parent_class;
+    __private SysBusDeviceClass parent_class;
     /*< public >*/
 
-    void (*pre_save)(GICState *s);
-    void (*post_load)(GICState *s);
+    __private void (*pre_save)(GICState *s);
+    __private void (*post_load)(GICState *s);
 } ARMGICCommonClass;
 
+#undef __private
+
+#ifdef IMPLEMENTING_ARM_GIC
+#define __private
+#else
+#define __private QEMU_PRIVATE_ATTR
+#endif
+
 #define TYPE_ARM_GIC "arm_gic"
 #define ARM_GIC(obj) \
      OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
@@ -99,10 +113,12 @@ typedef struct ARMGICCommonClass {
 
 typedef struct ARMGICClass {
     /*< private >*/
-    ARMGICCommonClass parent_class;
+    __private ARMGICCommonClass parent_class;
     /*< public >*/
 
-    DeviceRealize parent_realize;
+    __private DeviceRealize parent_realize;
 } ARMGICClass;
 
+#undef __private
+
 #endif
-- 
1.7.9.5

Re: [Qemu-devel] [RFC 2/2] arm_gic: Use new __private macro to mark private fields

[Alexander Graf]

On 25.07.2013, at 18:42, Peter Maydell wrote:

> Use the new __private macro infrastructure to mark private fields for
> the arm_gic classes.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/intc/arm_gic.c         |    3 +++
> hw/intc/arm_gic_common.c  |    2 ++
> hw/intc/arm_gic_kvm.c     |    2 ++
> hw/intc/armv7m_nvic.c     |    2 ++
> include/hw/intc/arm_gic.h |   64 ++++++++++++++++++++++++++++-----------------
> 5 files changed, 49 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 8e34004..cbe7d27 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -18,6 +18,9 @@
>  *  armv7m_nvic device.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +#define IMPLEMENTING_ARM_GIC
> +
> #include "hw/sysbus.h"
> #include "gic_internal.h"
> #include "qom/cpu.h"
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index a89c786..c27c5c1 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -18,6 +18,8 @@
>  * with this program; if not, see <http://www.gnu.org/licenses/>.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +
> #include "gic_internal.h"
> 
> static void gic_pre_save(void *opaque)
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index f713975..0acd676 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -18,6 +18,8 @@
>  * with this program; if not, see <http://www.gnu.org/licenses/>.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +
> #include "hw/sysbus.h"
> #include "sysemu/kvm.h"
> #include "kvm_arm.h"
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 178344b..54d62fe 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -10,6 +10,8 @@
>  * NVIC.  Much of that is also implemented here.
>  */
> 
> +#define IMPLEMENTING_ARM_GIC_COMMON
> +
> #include "hw/sysbus.h"
> #include "qemu/timer.h"
> #include "hw/arm/arm.h"
> diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
> index be945ec..d978d72 100644
> --- a/include/hw/intc/arm_gic.h
> +++ b/include/hw/intc/arm_gic.h
> @@ -40,36 +40,42 @@ typedef struct gic_irq_state {
>     bool trigger; /* nonzero = edge triggered.  */
> } gic_irq_state;
> 
> +#ifdef IMPLEMENTING_ARM_GIC_COMMON
> +#define __private
> +#else
> +#define __private QEMU_PRIVATE_ATTR
> +#endif
> +
> typedef struct GICState {
>     /*< private >*/
> -    SysBusDevice busdev;
> +    __private SysBusDevice busdev;
>     /*< public >*/

I've been wondering about this for a while, but why are private fields in the public section?


Alex

> 
> -    qemu_irq parent_irq[GIC_NCPU];
> -    bool enabled;
> -    bool cpu_enabled[GIC_NCPU];
> +    __private qemu_irq parent_irq[GIC_NCPU];
> +    __private bool enabled;
> +    __private bool cpu_enabled[GIC_NCPU];
> 
> -    gic_irq_state irq_state[GIC_MAXIRQ];
> -    uint8_t irq_target[GIC_MAXIRQ];
> -    uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
> -    uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
> -    uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
> +    __private gic_irq_state irq_state[GIC_MAXIRQ];
> +    __private uint8_t irq_target[GIC_MAXIRQ];
> +    __private uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
> +    __private uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
> +    __private uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
> 
> -    uint16_t priority_mask[GIC_NCPU];
> -    uint16_t running_irq[GIC_NCPU];
> -    uint16_t running_priority[GIC_NCPU];
> -    uint16_t current_pending[GIC_NCPU];
> +    __private uint16_t priority_mask[GIC_NCPU];
> +    __private uint16_t running_irq[GIC_NCPU];
> +    __private uint16_t running_priority[GIC_NCPU];
> +    __private uint16_t current_pending[GIC_NCPU];
> 
> -    uint32_t num_cpu;
> +    __private uint32_t num_cpu;
> 
> -    MemoryRegion iomem; /* Distributor */
> +    __private MemoryRegion iomem; /* Distributor */
>     /* This is just so we can have an opaque pointer which identifies
>      * both this GIC and which CPU interface we should be accessing.
>      */
> -    struct GICState *backref[GIC_NCPU];
> -    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> -    uint32_t num_irq;
> -    uint32_t revision;
> +    __private struct GICState *backref[GIC_NCPU];
> +    __private MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> +    __private uint32_t num_irq;
> +    __private uint32_t revision;
> } GICState;
> 
> #define TYPE_ARM_GIC_COMMON "arm_gic_common"
> @@ -82,13 +88,21 @@ typedef struct GICState {
> 
> typedef struct ARMGICCommonClass {
>     /*< private >*/
> -    SysBusDeviceClass parent_class;
> +    __private SysBusDeviceClass parent_class;
>     /*< public >*/
> 
> -    void (*pre_save)(GICState *s);
> -    void (*post_load)(GICState *s);
> +    __private void (*pre_save)(GICState *s);
> +    __private void (*post_load)(GICState *s);
> } ARMGICCommonClass;
> 
> +#undef __private
> +
> +#ifdef IMPLEMENTING_ARM_GIC
> +#define __private
> +#else
> +#define __private QEMU_PRIVATE_ATTR
> +#endif
> +
> #define TYPE_ARM_GIC "arm_gic"
> #define ARM_GIC(obj) \
>      OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
> @@ -99,10 +113,12 @@ typedef struct ARMGICCommonClass {
> 
> typedef struct ARMGICClass {
>     /*< private >*/
> -    ARMGICCommonClass parent_class;
> +    __private ARMGICCommonClass parent_class;
>     /*< public >*/
> 
> -    DeviceRealize parent_realize;
> +    __private DeviceRealize parent_realize;
> } ARMGICClass;
> 
> +#undef __private
> +
> #endif
> -- 
> 1.7.9.5
> 

Re: [Qemu-devel] [RFC 1/2] Provide infrastructure for marking private QOM struct fields

[Edgar E. Iglesias]

On Thu, Jul 25, 2013 at 05:42:02PM +0100, Peter Maydell wrote:
> Provide infrastructure for marking private QOM struct fields,
> so that a compiler warning is generated when a user of the QOM
> object attempts to access them directly.
> 
> This is implemented using GCC's 'deprecated' attribute; preprocessor
> macros arrange that when compiling the class implementation,
> no attribute is applied to the fields; when compiling a user
> of the class the fields are marked deprecated.
> 
> This allows us to have a single simple C struct defining the
> object, and for users of the QOM object to be able to embed
> instances of it into other structs, but still to have a guard
> against users accidentally touching parts of the structure
> they should not be accessing.

Very cool =)

Maybe there is a way to decrease the plain text impl footprint by
using header files?

#define im_the_implementation
#include xxx
#undef im_the_implementation

or something better...

Cheers,
Edgar

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/qemu/compiler.h |   10 ++++++++++
>  include/qom/object.h    |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 155b358..d7cc153 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -52,4 +52,14 @@
>  #define GCC_FMT_ATTR(n, m)
>  #endif
>  
> +/* An attribute usable to mark structure fields as private to the
> + * implementation; since this is only a diagnostic to catch programming
> + * errors, it's OK if it expands to nothing on non-gcc compilers.
> + */
> +#if defined __GNUC__
> +# define QEMU_PRIVATE_ATTR __attribute__((deprecated("this field is private")))
> +#else
> +# define QEMU_PRIVATE_ATTR
> +#endif
> +
>  #endif /* COMPILER_H */
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 23fc048..7f02f80 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -284,6 +284,53 @@ typedef struct InterfaceInfo InterfaceInfo;
>   *
>   * The first example of such a QOM method was #CPUClass.reset,
>   * another example is #DeviceClass.realize.
> + *
> + * = Marking fields as private to the class implementation =
> + *
> + * The expected code structure for QOM objects is that they should
> + * have a header file in include/ which defines the class and object
> + * structures and the typecasting macros. This header can then be
> + * included by both the source file which implements the QOM object
> + * and also by other source files which merely wish to use the object.
> + * Users of your object need the class and object structures so that
> + * they can embed instances of the object in their own structures;
> + * however they do not need to be able to access individual fields in
> + * these structures. To enforce this you should use the QEMU_PRIVATE_ATTR
> + * macro in a pattern like this:
> + *
> + * <example>
> + *   <title>Marking fields as private</title>
> + *   <programlisting>
> + * #ifdef IMPLEMENTING_MY_DEVICE
> + * # define __private
> + * #else
> + * # define __private QEMU_PRIVATE_ATTR
> + * #endif
> + *
> + * typedef struct MyDevice
> + * {
> + *     __private DeviceState parent;
> + *
> + *     __private int reg0, reg1, reg2;
> + * } MyDevice;
> + *
> + * typedef struct MyDeviceClass
> + * {
> + *     __private DeviceClass parent;
> + *
> + *     void (*frobnicate) (MyDevice *obj);
> + * } MyDeviceClass;
> + *
> + * #undef __private
> + *   </programlisting>
> + * </example>
> + *
> + * The source files which provide the implementation of your
> + * class (or of subclasses to it) should then have
> + * "#define IMPLEMENTING_MY_DEVICE" before they include any
> + * headers. Since users of the class will not define this
> + * macro, they will get a compilation warning if they access
> + * any of the private fields by mistake.
>   */
>  
>  
> -- 
> 1.7.9.5
> 
>