Re: [PATCH] object: require maximal alignment in base class

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 12, 2013 at 04:07:26PM -0700, Eric Blake wrote:
> Recent changes to events (commit 8a29ffcf) resulted in new compile
> failures on some targets (such as ARM OMAP5):
> conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc':
> conf/domain_event.c:1198:30: error: cast increases required alignment of
> target type [-Werror=cast-align]
> conf/domain_event.c:1314:34: error: cast increases required alignment of
> target type [-Werror=cast-align]
> cc1: all warnings being treated as errors
> 
> The error is due to alignment; the base class is merely aligned
> to the worst of 'int' and 'void*', while the child class must
> be aligned to a 'long long'.  The solution is to include a
> 'long long' (and for good measure, a function pointer) in the
> base class to ensure correct alignment regardless of what a
> child class may add, but to wrap the inclusion in a union so
> as to not incur any wasted space.  On a typical x86_64 platform,
> the base class remains 16 bytes; on i686, the base class remains
> 12 bytes; and on the impacted ARM platform, the base class grows
> from 12 bytes to 16 bytes due to the increase of alignment from
> 4 to 8 bytes.
> 
> Reported by Michele Paolino and others.
> 
> * src/util/virobject.h (_virObject): Use a union to ensure that
> subclasses never have stricter alignment than the parent.
> * src/util/virobject.c (virObjectNew, virObjectUnref)
> (virObjectRef): Adjust clients.
> * src/libvirt.c (virConnectRef, virDomainRef, virNetworkRef)
> (virInterfaceRef, virStoragePoolRef, virStorageVolRef)
> (virNodeDeviceRef, virSecretRef, virStreamRef, virNWFilterRef)
> (virDomainSnapshotRef): Likewise.
> * src/qemu/qemu_monitor.c (qemuMonitorOpenInternal)
> (qemuMonitorClose): Likewise.
> 
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> 
> Even though this fixes a build-breaker, I'd rather that someone
> actually experiencing the build failure test that this fixes
> the problem for them before I push (I don't have easy access to
> hardware exhibiting the problem).
> 
>  src/libvirt.c           | 22 +++++++++++-----------
>  src/qemu/qemu_monitor.c |  4 ++--
>  src/util/virobject.c    | 10 +++++-----
>  src/util/virobject.h    | 16 ++++++++++++++--
>  4 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index f01de83..d15d617 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -1563,7 +1563,7 @@ virConnectRef(virConnectPtr conn)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("conn=%p refs=%d", conn, conn->object.refs);
> +    VIR_DEBUG("conn=%p refs=%d", conn, conn->object.u.s.refs);
>      virObjectRef(conn);
>      return 0;
>  }
> @@ -2469,7 +2469,7 @@ virDomainRef(virDomainPtr domain)
>          return -1;
>      }
> 
> -    VIR_DOMAIN_DEBUG(domain, "refs=%d", domain->object.refs);
> +    VIR_DOMAIN_DEBUG(domain, "refs=%d", domain->object.u.s.refs);
>      virObjectRef(domain);
>      return 0;
>  }
> @@ -11977,7 +11977,7 @@ virNetworkRef(virNetworkPtr network)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("network=%p refs=%d", network, network->object.refs);
> +    VIR_DEBUG("network=%p refs=%d", network, network->object.u.s.refs);
>      virObjectRef(network);
>      return 0;
>  }
> @@ -12921,7 +12921,7 @@ virInterfaceRef(virInterfacePtr iface)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("iface=%p refs=%d", iface, iface->object.refs);
> +    VIR_DEBUG("iface=%p refs=%d", iface, iface->object.u.s.refs);
>      virObjectRef(iface);
>      return 0;
>  }
> @@ -13980,7 +13980,7 @@ virStoragePoolRef(virStoragePoolPtr pool)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("pool=%p refs=%d", pool, pool->object.refs);
> +    VIR_DEBUG("pool=%p refs=%d", pool, pool->object.u.s.refs);
>      virObjectRef(pool);
>      return 0;
>  }
> @@ -15101,7 +15101,7 @@ virStorageVolRef(virStorageVolPtr vol)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("vol=%p refs=%d", vol, vol->object.refs);
> +    VIR_DEBUG("vol=%p refs=%d", vol, vol->object.u.s.refs);
>      virObjectRef(vol);
>      return 0;
>  }
> @@ -15792,7 +15792,7 @@ virNodeDeviceRef(virNodeDevicePtr dev)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("dev=%p refs=%d", dev, dev->object.refs);
> +    VIR_DEBUG("dev=%p refs=%d", dev, dev->object.u.s.refs);
>      virObjectRef(dev);
>      return 0;
>  }
> @@ -16900,7 +16900,7 @@ virSecretRef(virSecretPtr secret)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("secret=%p refs=%d", secret, secret->object.refs);
> +    VIR_DEBUG("secret=%p refs=%d", secret, secret->object.u.s.refs);
>      virObjectRef(secret);
>      return 0;
>  }
> @@ -16994,7 +16994,7 @@ virStreamRef(virStreamPtr stream)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("stream=%p refs=%d", stream, stream->object.refs);
> +    VIR_DEBUG("stream=%p refs=%d", stream, stream->object.u.s.refs);
>      virObjectRef(stream);
>      return 0;
>  }
> @@ -18400,7 +18400,7 @@ virNWFilterRef(virNWFilterPtr nwfilter)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("nwfilter=%p refs=%d", nwfilter, nwfilter->object.refs);
> +    VIR_DEBUG("nwfilter=%p refs=%d", nwfilter, nwfilter->object.u.s.refs);
>      virObjectRef(nwfilter);
>      return 0;
>  }
> @@ -20699,7 +20699,7 @@ virDomainSnapshotRef(virDomainSnapshotPtr snapshot)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("snapshot=%p, refs=%d", snapshot, snapshot->object.refs);
> +    VIR_DEBUG("snapshot=%p, refs=%d", snapshot, snapshot->object.u.s.refs);
>      virObjectRef(snapshot);
>      return 0;
>  }
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 1514715..1fa1492 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -821,7 +821,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
> 
>      PROBE(QEMU_MONITOR_NEW,
>            "mon=%p refs=%d fd=%d",
> -          mon, mon->parent.parent.refs, mon->fd);
> +          mon, mon->parent.parent.u.s.refs, mon->fd);
>      virObjectUnlock(mon);
> 
>      return mon;
> @@ -893,7 +893,7 @@ void qemuMonitorClose(qemuMonitorPtr mon)
> 
>      virObjectLock(mon);
>      PROBE(QEMU_MONITOR_CLOSE,
> -          "mon=%p refs=%d", mon, mon->parent.parent.refs);
> +          "mon=%p refs=%d", mon, mon->parent.parent.u.s.refs);
> 
>      if (mon->fd >= 0) {
>          if (mon->watch) {
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 61b5413..4f83bc1 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -192,9 +192,9 @@ void *virObjectNew(virClassPtr klass)
>                        klass->objectSize - sizeof(virObject)) < 0)
>          return NULL;
> 
> -    obj->magic = klass->magic;
> +    obj->u.s.magic = klass->magic;
>      obj->klass = klass;
> -    virAtomicIntSet(&obj->refs, 1);
> +    virAtomicIntSet(&obj->u.s.refs, 1);
> 
>      PROBE(OBJECT_NEW, "obj=%p classname=%s", obj, obj->klass->name);
> 
> @@ -252,7 +252,7 @@ bool virObjectUnref(void *anyobj)
>      if (!obj)
>          return false;
> 
> -    bool lastRef = virAtomicIntDecAndTest(&obj->refs);
> +    bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
>      PROBE(OBJECT_UNREF, "obj=%p", obj);
>      if (lastRef) {
>          PROBE(OBJECT_DISPOSE, "obj=%p", obj);
> @@ -265,7 +265,7 @@ bool virObjectUnref(void *anyobj)
> 
>          /* Clear & poison object */
>          memset(obj, 0, obj->klass->objectSize);
> -        obj->magic = 0xDEADBEEF;
> +        obj->u.s.magic = 0xDEADBEEF;
>          obj->klass = (void*)0xDEADBEEF;
>          VIR_FREE(obj);
>      }
> @@ -289,7 +289,7 @@ void *virObjectRef(void *anyobj)
> 
>      if (!obj)
>          return NULL;
> -    virAtomicIntInc(&obj->refs);
> +    virAtomicIntInc(&obj->u.s.refs);
>      PROBE(OBJECT_REF, "obj=%p", obj);
>      return anyobj;
>  }
> diff --git a/src/util/virobject.h b/src/util/virobject.h
> index 3a08f10..d571b5c 100644
> --- a/src/util/virobject.h
> +++ b/src/util/virobject.h
> @@ -36,9 +36,21 @@ typedef virObjectLockable *virObjectLockablePtr;
> 
>  typedef void (*virObjectDisposeCallback)(void *obj);
> 
> +/* Most code should not play with the contents of this struct; however,
> + * the struct itself is public so that it can be embedded as the first
> + * field of a subclassed object.  */
>  struct _virObject {
> -    unsigned int magic;
> -    int refs;
> +    /* Ensure correct alignment of this and all subclasses, even on
> +     * platforms where 'long long' or function pointers have stricter
> +     * requirements than 'void *'.  */
> +    union {
> +        long long dummy_align1;
> +        void (*dummy_align2) (void);
> +        struct {
> +            unsigned int magic;
> +            int refs;
> +        } s;
> +    } u;
>      virClassPtr klass;
>  };

ACK

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]