[PATCH] conf: force 8 byte alignment for virObjectEvent
We need to be able to cast from virObjectEventPtr to one of its many subclasses. Some of these subclasses have 8 byte alignment on 32-bit platforms, but virObjectEventPtr only has 4 byte alignment. Previously the virObject base class had 8 byte alignment but this dropped to 4 byte when converted to inherit from GObject. This introduces cast alignment warnings on 32-bit: ../../src/conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc': ../../src/conf/domain_event.c:1656:30: error: cast increases required alignment of target type [-Werror=cast-align] 1656 | rtcChangeEvent = (virDomainEventRTCChangePtr)event; | ^ ../../src/conf/domain_event.c:1785:34: error: cast increases required alignment of target type [-Werror=cast-align] 1785 | balloonChangeEvent = (virDomainEventBalloonChangePtr)event; | ^ ../../src/conf/domain_event.c:1896:35: error: cast increases required alignment of target type [-Werror=cast-align] 1896 | blockThresholdEvent = (virDomainEventBlockThresholdPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventDispatchFunc': ../../src/conf/domain_event.c:1974:24: error: cast increases required alignment of target type [-Werror=cast-align] 1974 | qemuMonitorEvent = (virDomainQemuMonitorEventPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventFilter': ../../src/conf/domain_event.c:2179:20: error: cast increases required alignment of target type [-Werror=cast-align] 2179 | monitorEvent = (virDomainQemuMonitorEventPtr) event; | ^ Forcing 8-byte alignment on virObjectEventPtr removes the alignment increase during casts to subclasses. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- Technically a build-breaker, but since we don't have any existing usage of __attribute__((aligned)), I wanted to get a second opinion on this approach. One alternative approach would be to switch one of the current "int" fields in virObjectEvent to "long long". src/conf/object_event_private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index b31441c53a..126464a9a5 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -42,7 +42,7 @@ typedef void virConnectObjectEventGenericCallback cb, void *cbopaque); -struct _virObjectEvent { +struct __attribute__((aligned(4))) _virObjectEvent { virObject parent; int eventID; virObjectMeta meta; -- 2.24.1
On Wed, Jun 03, 2020 at 11:22:47AM +0100, Daniel P. Berrangé wrote:
We need to be able to cast from virObjectEventPtr to one of its many subclasses. Some of these subclasses have 8 byte alignment on 32-bit platforms, but virObjectEventPtr only has 4 byte alignment.
Previously the virObject base class had 8 byte alignment but this dropped to 4 byte when converted to inherit from GObject. This introduces cast alignment warnings on 32-bit:
../../src/conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc': ../../src/conf/domain_event.c:1656:30: error: cast increases required alignment of target type [-Werror=cast-align] 1656 | rtcChangeEvent = (virDomainEventRTCChangePtr)event; | ^ ../../src/conf/domain_event.c:1785:34: error: cast increases required alignment of target type [-Werror=cast-align] 1785 | balloonChangeEvent = (virDomainEventBalloonChangePtr)event; | ^ ../../src/conf/domain_event.c:1896:35: error: cast increases required alignment of target type [-Werror=cast-align] 1896 | blockThresholdEvent = (virDomainEventBlockThresholdPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventDispatchFunc': ../../src/conf/domain_event.c:1974:24: error: cast increases required alignment of target type [-Werror=cast-align] 1974 | qemuMonitorEvent = (virDomainQemuMonitorEventPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventFilter': ../../src/conf/domain_event.c:2179:20: error: cast increases required alignment of target type [-Werror=cast-align] 2179 | monitorEvent = (virDomainQemuMonitorEventPtr) event; | ^
Forcing 8-byte alignment on virObjectEventPtr removes the alignment increase during casts to subclasses.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> ---
Technically a build-breaker, but since we don't have any existing usage of __attribute__((aligned)), I wanted to get a second opinion on this approach.
One alternative approach would be to switch one of the current "int" fields in virObjectEvent to "long long".
I personally prefer using __attribute__. We already use that GCC extension so I don't see any reason not using another part of that extension.
src/conf/object_event_private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index b31441c53a..126464a9a5 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -42,7 +42,7 @@ typedef void virConnectObjectEventGenericCallback cb, void *cbopaque);
-struct _virObjectEvent { +struct __attribute__((aligned(4))) _virObjectEvent { virObject parent; int eventID; virObjectMeta meta;
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On 6/3/20 5:22 AM, Daniel P. Berrangé wrote:
Forcing 8-byte alignment on virObjectEventPtr removes the alignment increase during casts to subclasses.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> ---
Technically a build-breaker, but since we don't have any existing usage of __attribute__((aligned)), I wanted to get a second opinion on this approach.
One alternative approach would be to switch one of the current "int" fields in virObjectEvent to "long long".
-struct _virObjectEvent { +struct __attribute__((aligned(4))) _virObjectEvent { virObject parent; int eventID; virObjectMeta meta;
As in this, although it makes the struct larger on 32-bit platforms (which may in turn affect cache usage): struct _virObjectEvent { virObject parent; long long eventID; ... Another possibility: use the extension of unnamed unions (but if we're going to rely on gcc extensions, __attribute__ is nicer than unnamed unions): struct _virObjectEvent { union { virObject parent; long long alignment_; }; int eventID; ... We already limit ourselves to gcc and clang because of __attribute__((cleanup)), so I don't see any problem with your approach. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Jun 3, 2020 at 12:29 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
We need to be able to cast from virObjectEventPtr to one of its many subclasses. Some of these subclasses have 8 byte alignment on 32-bit platforms, but virObjectEventPtr only has 4 byte alignment.
Previously the virObject base class had 8 byte alignment but this dropped to 4 byte when converted to inherit from GObject. This introduces cast alignment warnings on 32-bit:
../../src/conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc': ../../src/conf/domain_event.c:1656:30: error: cast increases required alignment of target type [-Werror=cast-align] 1656 | rtcChangeEvent = (virDomainEventRTCChangePtr)event; | ^ ../../src/conf/domain_event.c:1785:34: error: cast increases required alignment of target type [-Werror=cast-align] 1785 | balloonChangeEvent = (virDomainEventBalloonChangePtr)event; | ^ ../../src/conf/domain_event.c:1896:35: error: cast increases required alignment of target type [-Werror=cast-align] 1896 | blockThresholdEvent = (virDomainEventBlockThresholdPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventDispatchFunc': ../../src/conf/domain_event.c:1974:24: error: cast increases required alignment of target type [-Werror=cast-align] 1974 | qemuMonitorEvent = (virDomainQemuMonitorEventPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventFilter': ../../src/conf/domain_event.c:2179:20: error: cast increases required alignment of target type [-Werror=cast-align] 2179 | monitorEvent = (virDomainQemuMonitorEventPtr) event; | ^
Forcing 8-byte alignment on virObjectEventPtr removes the alignment increase during casts to subclasses.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> ---
Technically a build-breaker, but since we don't have any existing usage of __attribute__((aligned)), I wanted to get a second opinion on this approach.
One alternative approach would be to switch one of the current "int" fields in virObjectEvent to "long long".
src/conf/object_event_private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index b31441c53a..126464a9a5 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -42,7 +42,7 @@ typedef void virConnectObjectEventGenericCallback cb, void *cbopaque);
-struct _virObjectEvent { +struct __attribute__((aligned(4))) _virObjectEvent { virObject parent; int eventID; virObjectMeta meta; -- 2.24.1
This is the same error I'm seeing with my patches in CI when converting virStorageSource to GObject except no alignment value seems to fix it. -- Rafael Fonseca
On 6/3/20 12:22 PM, Daniel P. Berrangé wrote:
We need to be able to cast from virObjectEventPtr to one of its many subclasses. Some of these subclasses have 8 byte alignment on 32-bit platforms, but virObjectEventPtr only has 4 byte alignment.
Previously the virObject base class had 8 byte alignment but this dropped to 4 byte when converted to inherit from GObject. This introduces cast alignment warnings on 32-bit:
../../src/conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc': ../../src/conf/domain_event.c:1656:30: error: cast increases required alignment of target type [-Werror=cast-align] 1656 | rtcChangeEvent = (virDomainEventRTCChangePtr)event; | ^ ../../src/conf/domain_event.c:1785:34: error: cast increases required alignment of target type [-Werror=cast-align] 1785 | balloonChangeEvent = (virDomainEventBalloonChangePtr)event; | ^ ../../src/conf/domain_event.c:1896:35: error: cast increases required alignment of target type [-Werror=cast-align] 1896 | blockThresholdEvent = (virDomainEventBlockThresholdPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventDispatchFunc': ../../src/conf/domain_event.c:1974:24: error: cast increases required alignment of target type [-Werror=cast-align] 1974 | qemuMonitorEvent = (virDomainQemuMonitorEventPtr)event; | ^ ../../src/conf/domain_event.c: In function 'virDomainQemuMonitorEventFilter': ../../src/conf/domain_event.c:2179:20: error: cast increases required alignment of target type [-Werror=cast-align] 2179 | monitorEvent = (virDomainQemuMonitorEventPtr) event; | ^
Forcing 8-byte alignment on virObjectEventPtr removes the alignment increase during casts to subclasses.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> ---
Technically a build-breaker, but since we don't have any existing usage of __attribute__((aligned)), I wanted to get a second opinion on this approach.
One alternative approach would be to switch one of the current "int" fields in virObjectEvent to "long long".
src/conf/object_event_private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index b31441c53a..126464a9a5 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -42,7 +42,7 @@ typedef void virConnectObjectEventGenericCallback cb, void *cbopaque);
-struct _virObjectEvent { +struct __attribute__((aligned(4))) _virObjectEvent { virObject parent; int eventID; virObjectMeta meta;
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (5)
-
Daniel P. Berrangé -
Eric Blake -
Michal Privoznik -
Pavel Hrdina -
Rafael Fonseca