[libvirt] [PATCH] object: require maximal alignment in base class
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@redhat.com> --- 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; }; -- 1.8.4.2
On 13.12.2013 00:07, 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@redhat.com> ---
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/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; };
Beside the comment above union I got the same diff. I did compile check on my armv6l box and it passed. ACK Michal
On 12/13/2013 03:22 AM, Michal Privoznik wrote:
On 13.12.2013 00:07, 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]
* 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.
Beside the comment above union I got the same diff. I did compile check on my armv6l box and it passed.
ACK
Thanks for the testing; patch now pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> ---
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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik