On Thu, Jun 4, 2020 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> 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@gmail.com> wrote:
On Mon, May 18, 2020 at 11:31 AM Daniel P. Berrangé <berrange@redhat.com> 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@redhat.com> 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