On Thu, Jun 4, 2020 at 11:16 AM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote: > > On Mon, May 18, 2020 at 05:16:29PM +0200, Rafael Fonseca wrote: > > On Mon, May 18, 2020 at 3:28 PM Rafael Fonseca <r4f4rfs@xxxxxxxxx> wrote: > > > > > > On Mon, May 18, 2020 at 11:31 AM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote: > > > > > > > > On Sun, May 17, 2020 at 11:03:57PM +0200, Rafael Fonseca wrote: > > > > > On Fri, May 15, 2020 at 5:24 PM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote: > > > > > > Since we're using GitLab now, if you just fork the libvirt repo > > > > > > and push a branch to your gitlab fork, it will run CI which will > > > > > > check for these mistakes. > > > > > > > > > > Good to know. I did that and the CI is only failing now in the > > > > > cross-compilation phase with 32-bit systems: > > > > > > > > > > ../../src/util/virstoragefile.h: In function 'VIR_STORAGE_SOURCE': > > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:2301:6: > > > > > error: cast increases required alignment of target type > > > > > [-Werror=cast-align] > > > > > 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) > > > > > | ^ > > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:484:66: > > > > > note: in expansion of macro '_G_TYPE_CIC' > > > > > 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) > > > > > (_G_TYPE_CIC ((instance), (g_type), c_type)) > > > > > | ^~~~~~~~~~~ > > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:1412:12: > > > > > note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' > > > > > 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, > > > > > module_obj_name##_get_type (), ModuleObjName); } \ > > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > ../../src/util/virstoragefile.h:390:1: note: in expansion of macro > > > > > 'G_DECLARE_FINAL_TYPE' > > > > > 390 | G_DECLARE_FINAL_TYPE(virStorageSource, vir_storage_source, VIR, > > > > > STORAGE_SOURCE, GObject); > > > > > | ^~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > Any ideas on how to solve that? > > > > > > > > This is odd and I can't tell why. Assyuming only virStorageSource shows > > > > the issue though, there must be something in the way the struct is > > > > arranged that causes alignment issues. > > > > > > I disabled the virStorageSource patch and tried again. On armv7l it > > > failed with the same error but in a different part: > > > > > > ../../src/qemu/qemu_conf.h: In function 'VIR_QEMU_DRIVER_CONFIG': > > > > Just to complete the information, I also disabled the QEmuDriverConfig > > patch and the CI finished successfully in armv7l. So it's something in > > virStorageSource and QemuDriverConfig, either the structs themselves > > or something missing from my changes. > > You're not doing anything wrong. The compiler doesn't have enough info to > realize that its warning is wrong. > > We have a "GObject *" which requires 4 byte alignment, and are trying to > cast to "virStorageSource *" or "QemuDriverConfig *" which require 8 byte > alignment. > > The compiler is pessimistic and has to consider that the original memory > for the "GObject *" is only aligned to 4 byte boundary, and thus a bad > cast would result if we cast to "virStorageSource *". > > What the compiler does not know is that any "GObject *" we have will always > have been allocated using g_object_new, which delegates to g_slice_new, > which uses malloc or a slab allocator. Both malloc & the glib slab allocator > guarantee that all memory they allocate is aligned to 8 byte boundaries on > 32-bit. > > So the only way the cast from GObject * -> virStorageSource * could be a > bad cast is if we allocated the GObject on the stack but that'll never > happen. > > So the cast alignment warnings are bogus. > > The difficulty is that I don't see a nice way to eliminate the warnings. > Options I see include: > > 1 Turn off -Wcast-align entirely > 2 Modify GLib to add attribute(align(8)) to GObject > 3 Modify GLib to use Pragma push/pop to selectively disable -Wcast-align > > I think (2) is an ABI change is probably not possible. Possibly we have > todo (1) and then also do (3) so we can revert (1) in future. Thank you for the write up. I don't have much experience with alignment issues, so any help is appreciated. I was doing some experiments and ended up replacing the macro G_DECLARE_FINAL_TYPE by the actual code. The problem doesn't seem to be the cast virStorageSource* -> GObject* but virStorageSource* -> GTypeInstance*. GTypeInstance struct has a pointer to a GTypeClass which has a GType field. GType == gsize. This cast in GLib's code is guarded by the #ifndef G_DISABLE_CAST_CHECKS [https://gitlab.gnome.org/GNOME/glib/-/blob/master/gobject/gtype.h#L2295]. In case that's defined, it skips the g_type_check_instance_cast call and just does (virStorageSource*)obj. So I did that in the code and there were no compiler warnings for virStorageSource. Does this information help in any way? Is "unrolling" the macro by the actual code for these 2 structs (virStorageSource and virQEMUDriverConfig) and replacing the cast checking by just a pure cast an option (4)? Att. -- Rafael Fonseca
