On Tue, 5 Mar 2024 08:27:52 +0100 Thomas Huth <th...@redhat.com> wrote:
> On 04/03/2024 16.10, Jonathan Cameron wrote: > > On Mon, 4 Mar 2024 11:44:06 +0100 > > Thomas Huth <th...@redhat.com> wrote: > > > >> When setting GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_58 or higher, > >> glib adds type safety checks to the g_steal_pointer() macro. This > >> triggers errors in the ct3_build_cdat_entries_for_mr() function which > >> uses the g_steal_pointer() for type-casting from one pointer type to > >> the other (which also looks quite weird since the local pointers have > >> all been declared with g_autofree though they are never freed here). > >> Fix it by using a proper typecast instead. For making this possible, we > >> have to remove the QEMU_PACKED attribute from some structs since GCC > >> otherwise complains that the source and destination pointer might > >> have different alignment restrictions. Removing the QEMU_PACKED should > >> be fine here since the structs are already naturally aligned. Anyway, > >> add some QEMU_BUILD_BUG_ON() statements to make sure that we've got > >> the right sizes (without padding in the structs). > > > > I missed these as well when getting rid of the false handling > > of failure of g_new0 calls. > > > > Another alternative would be to point to the head structures rather > > than the containing structure - would avoid need to cast. > > That might be neater? Should I think also remove the alignment > > question? > > I gave it a try, but it does not help against the alignment issue, I still > get: > > ../../devel/qemu/hw/mem/cxl_type3.c: In function > ‘ct3_build_cdat_entries_for_mr’: > ../../devel/qemu/hw/mem/cxl_type3.c:138:34: error: taking address of packed > member of ‘struct CDATDsmas’ may result in an unaligned pointer value > [-Werror=address-of-packed-member] > 138 | cdat_table[CT3_CDAT_DSMAS] = &dsmas->header; > | ^~~~~~~~~~~~~~ > > From my experience, it's better anyway to avoid __attribute__((packed)) on > structures unless it is really really required. At least we should avoid it > as good as possible as long as we still support running QEMU on Sparc hosts > (that don't support misaligned memory accesses), since otherwise you can end > up with non-working code there, see e.g.: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg439899.html > > or: > > https://gitlab.com/qemu-project/qemu/-/commit/cb89b349074310ff9eb7ebe18a > > Thus I'd rather prefer to keep this patch as it is right now. > > Thomas Fair enough. Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com> >
