On Tue, Sep 15, 2020 at 12:09:59PM -0700, Richard Henderson wrote: > On 9/15/20 11:07 AM, Eduardo Habkost wrote: > > On Tue, Sep 15, 2020 at 10:46:31AM -0700, Richard Henderson wrote: > >> It turns out that some hosts have a default malloc alignment less > >> than that required for vectors. > >> > >> We assume that, with compiler annotation on CPUArchState, that we > >> can properly align the vector portion of the guest state. Fix the > >> alignment of the allocation by using qemu_memalloc when required. > >> > >> Signed-off-by: Richard Henderson <richard.hender...@linaro.org> > >> --- > >> Cc: Paolo Bonzini <pbonz...@redhat.com> > >> Cc: "Daniel P. Berrangé" <berra...@redhat.com> > >> Cc: Eduardo Habkost <ehabk...@redhat.com> > >> --- > >> include/qom/object.h | 4 ++++ > >> qom/object.c | 16 +++++++++++++--- > >> 2 files changed, 17 insertions(+), 3 deletions(-) > >> > >> diff --git a/include/qom/object.h b/include/qom/object.h > >> index 056f67ab3b..d52d0781a3 100644 > >> --- a/include/qom/object.h > >> +++ b/include/qom/object.h > >> @@ -770,6 +770,9 @@ struct Object > >> * @instance_size: The size of the object (derivative of #Object). If > >> * @instance_size is 0, then the size of the object will be the size of > >> the > >> * parent object. > >> + * @instance_align: The required alignment of the object. If > >> @instance_align > >> + * is 0, then normal malloc alignment is sufficient; if non-zero, then > >> we > >> + * must use qemu_memalign for allocation. > >> * @instance_init: This function is called to initialize an object. The > >> parent > >> * class will have already been initialized so the type is only > >> responsible > >> * for initializing its own members. > >> @@ -807,6 +810,7 @@ struct TypeInfo > >> const char *parent; > >> > >> size_t instance_size; > >> + size_t instance_align; > >> void (*instance_init)(Object *obj); > >> void (*instance_post_init)(Object *obj); > >> void (*instance_finalize)(Object *obj); > >> diff --git a/qom/object.c b/qom/object.c > >> index 387efb25eb..2e53cb44a6 100644 > >> --- a/qom/object.c > >> +++ b/qom/object.c > >> @@ -50,6 +50,7 @@ struct TypeImpl > >> size_t class_size; > >> > >> size_t instance_size; > >> + size_t instance_align; > >> > >> void (*class_init)(ObjectClass *klass, void *data); > >> void (*class_base_init)(ObjectClass *klass, void *data); > >> @@ -114,6 +115,7 @@ static TypeImpl *type_new(const TypeInfo *info) > >> > >> ti->class_size = info->class_size; > >> ti->instance_size = info->instance_size; > >> + ti->instance_align = info->instance_align; > >> > >> ti->class_init = info->class_init; > >> ti->class_base_init = info->class_base_init; > >> @@ -691,13 +693,21 @@ static void object_finalize(void *data) > >> static Object *object_new_with_type(Type type) > >> { > >> Object *obj; > >> + size_t size, align; > >> > >> g_assert(type != NULL); > >> type_initialize(type); > >> > >> - obj = g_malloc(type->instance_size); > >> - object_initialize_with_type(obj, type->instance_size, type); > >> - obj->free = g_free; > >> + size = type->instance_size; > >> + align = type->instance_align; > >> + if (align) { > > > > If we check for (align > G_MEM_ALIGN) instead, we will be able to > > set instance_align automatically at OBJECT_DEFINE_TYPE*. > > I agree a value check would be good here, as well as setting this by default. > > As for the value check itself... > > I see that G_MEM_ALIGN isn't actually defined in an interesting or even > correct > way. E.g. it doesn't take the long double type into account. > > The usual mechanism is > > struct s { > char pad; > union { > long l; > void *p; > double d; > long double ld; > } u; > }; > > offsetof(s, u) > > since all of these types are required to be taken into account by the system > malloc.
Due to the existence of G_MEM_ALIGN, I thought GLib alignment guarantees could be weaker than malloc(). I've just learned that GLib uses system malloc() since 2.46. > > E.g it doesn't take other host guarantees into account, e.g. i386-linux > guarantees 16-byte alignment. This possibly dubious ABI change was made 20+ > years ago with the introduction of SSE and is now set in stone. > > Glibc has a "malloc-alignment.h" internal header that defaults to > > MIN(2 * sizeof(size_t), __alignof__(long double)) > > and is overridden for i386. Sadly, it doesn't export MALLOC_ALIGNMENT. > > Musl has two different malloc implementations. One has UNIT = 16; the other > has SIZE_ALIGN = 4*sizeof(size_t). Both have a minimum value of 16, and this > is not target-specific. > > Any further comments on the subject, or should I put together something that > computes the MAX of the above? Once we move to C11, we can just use max_align_t. While we don't move to C11, why not just use __alignof__(union { long l; void *p; double d; long double ld;}) ? -- Eduardo