(In reply to Christian Hergert from comment #0)
> (since we are guaranteed to get a 2-pointer aligned allocation from GSlice
> anyway).

Really? The documentation for g_slice_alloc() says that the returned address:
> can be expected to be aligned to at least 1 * sizeof (void*), though in general slices are 2 * sizeof (void*) bytes aligned, if a malloc() fallback implementation is used instead, the alignment may be reduced in a libc dependent fashion.

I haven’t checked the GSlice code, but if that’s true then our alignment might potentially not be correct for objects like this.

*** Bug 712370 has been marked as a duplicate of this bug. ***

If it's not 2*sizeof(void*), it should at least be >=sizeof(gdouble) since malloc is guaranteed to return a pointer that can store the basic types.

So we should at least mark the alignment as 8 instead of 4. This will fix situations like:

/* in header */
G_DECLARE_FINAL_TYPE (FooBar, fooBar, FOO, BAR, GObject)

/* in source */
struct _FooBar {
  GObject parent;
  gdouble bar;
};

Currently, on 32-bit there is a strong chance you'll get a warning for this, because the gdouble increases the alignment requirement to 8, but the ref_count field from GObject is only 4 (and the header forward-declaration of the type will not know about the double).

(In reply to Christian Hergert from comment #3)
> If it's not 2*sizeof(void*), it should at least be >=sizeof(gdouble) since
> malloc is guaranteed to return a pointer that can store the basic types.

Right, but sizeof(double) is undefined in the C standard, iirc. It’s almost always 8 bytes (guaranteed to be so on x86/x86_64 platforms, since they’re guaranteed to use IEEE 754 floating point; but might differ on other platforms).

> So we should at least mark the alignment as 8 instead of 4. This will fix
> situations like:
> 
> /* in header */
> G_DECLARE_FINAL_TYPE (FooBar, fooBar, FOO, BAR, GObject)
> 
> /* in source */
> struct _FooBar {
>   GObject parent;
>   gdouble bar;
> };
> 
> Currently, on 32-bit there is a strong chance you'll get a warning for this,
> because the gdouble increases the alignment requirement to 8, but the
> ref_count field from GObject is only 4 (and the header forward-declaration
> of the type will not know about the double).

Yeah. If we marked the alignment of GObject as 8, that wouldn’t be a significant performance hit anywhere, would it? It might mean an extra 4 bytes of allocation padding on some platforms (but would mean no difference on most platforms). It would easily solve all the problems like this. I’d be in favour of that if there really are no downsides.

(In reply to Philip Withnall from comment #4)
> Yeah. If we marked the alignment of GObject as 8, that wouldn’t be a
> significant performance hit anywhere, would it? It might mean an extra 4
> bytes of allocation padding on some platforms (but would mean no difference
> on most platforms). It would easily solve all the problems like this. I’d be
> in favour of that if there really are no downsides.

Performance-wise, I'd expect it to only ever be better since you reduce your chances of reading data across alignment boundaries (which could be a SIGBUS on some architectures like SPARC anyway).

GSlice first categorizes the allocation into a few categories:

 1 - allocate from magazine
 2 - allocate from slab
 3 - fallback to malloc

For 3, we don't need to do anything, since we're guaranteed to get something back that can store a double.

For 1 and 2, they use the chunk_size, which is the nearest ^2 from the requested allocation size (gslice.c:1005). Any power of two that is >=8 will have a natural alignment that is safe for double (and thereby, anything containing a double would be safe using natural packing alignments).

Since the fallback for gslice (G_SLICE=always-malloc) will always be aligned for double storage, I believe this is safe to just add an alignment == sizeof(double) (and that might require a configure time check instead of 8, but I doubt it anywhere we run today).

(In reply to Christian Hergert from comment #5)
> GSlice first categorizes the allocation into a few categories:
> 
>  1 - allocate from magazine
>  2 - allocate from slab
>  3 - fallback to malloc
> 
> For 3, we don't need to do anything, since we're guaranteed to get something
> back that can store a double.

That would give sufficient alignment to fix the original bug here, but it doesn’t strictly translate into a guarantee that everything is at least 8-byte aligned.

Do we want to give a guarantee that everything is 8-byte aligned, or a guarantee that everything is aligned to the largest basic type?

(In reply to Philip Withnall from comment #6)
> That would give sufficient alignment to fix the original bug here, but it
> doesn’t strictly translate into a guarantee that everything is at least
> 8-byte aligned.

But it does, because if we look at the GObject struct:

  struct  _GObject
  {
    GTypeInstance  g_type_instance;

    /*< private >*/
    volatile guint ref_count;
    GData         *qdata;
  };

On 32-bit systems, that will be 12-bytes, meaning we use the 16-byte slab/magazine in GSlice. So it will always have a natural alignment of at least a 16-byte boundary.

For malloc based allocations, we are guaranteed to have something that can store a long long/uint64_t/etc. So we maintain at least an 8-byte alignment.

> Do we want to give a guarantee that everything is 8-byte aligned, or a
> guarantee that everything is aligned to the largest basic type?

Largest basic type probably.

(In reply to Christian Hergert from comment #7)
> Largest basic type probably.

As long as that basic type is <= 16 bytes.

Created attachment 365250 [details] [review]
gobject: Assert that GObjects are at least as aligned as basic types

See the reasoning in the patch for why we believe GObjects *are*
(already) as aligned as the basic types.

We want to make this guarantee so that it’s guaranteed to be safe for
people to ignore -Wcast-align warnings for GObjects which contain basic
types. This typically happens with gdouble on 32-bit ARM platforms.

The checks are slightly complicated by the need to support GObjects with
custom constructors. We should expect that a custom construction
function will chain up to g_object_constructor (which calls
g_type_create_instance() as normal), but it’s possible that someone has
done something crazy and uses a custom allocator which doesn’t return
with the same alignment as GSlice. Hand them a warning in that case. If
that is true, the code which uses their custom-constructed GObject can
presumably already deal with the alignment it gets given.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 365251 [details] [review]
gtype: Eliminate -Wcast-align warnings with G_TYPE_CHECK_INSTANCE_CAST

Regardless of the actual alignment of the GTypeInstance in question,
these do a runtime check on the type, so if the type was originally
aligned correctly when allocated, it should be aligned correctly if the
type check succeeds. -Wcast-align is meant to warn about casts between
types, which this isn’t (if the check succeeds).

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 365252 [details] [review]
gobject: Add advice on larger alignment requirements for GObject members

We now guarantee that GObjects will always be allocated at least as
aligned as the basic types. If you want to put an element in your
GObject which has higher alignment requirements, we can’t guarantee it
will be aligned*. If you need it to be aligned, you’ll need to put it on
the heap (aligned appropriately), or add appropriate padding in your
GObject struct.

*Actually, GSlice will guarantee that the whole GObject is aligned to at
least the power of 2 greater than or equal to the size of the GObject,
which means any element in the GObject struct should always be
appropriate aligned if the compiler pads it appropriately. If malloc()
is used, however, it doesn’t make that guarantee, so we can’t make that
guarantee overall.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

OK, here’s my take on it, in three parts:
 • Add a runtime assert that GObject is at least as aligned as gdouble/guint64.

 • Cast through (void*) in G_TYPE_CHECK_INSTANCE_CAST. Since we’re doing a runtime type check, it doesn’t matter what the alignment actually is: if the object’s been aligned correctly on allocation, any successful cast to it is guaranteed to be aligned. I haven’t made the corresponding change on the optimised version of G_TYPE_CHECK_INSTANCE_CAST which doesn’t do the runtime check, since it doesn’t cast through GTypeInstance*. Maybe I should, for consistency with the first version? I could see there being warnings emitted when building against a non-debug GLib which aren’t emitted with debug GLib then.

 • Document what people are supposed to do if they want a more-aligned member in a GObject struct: allocate the member on the heap, or ensure their struct is appropriately padded.

Writing the last patch made me wonder whether perhaps we should instead be making stronger guarantees about the alignments of GObjects: for example, the GSlice guarantee that they’re aligned to the power of 2 greater than or equal to the struct size; or a guarantee that they’re always as aligned as their member with the largest alignment.

I was also wondering about how to expose this runtime alignment information to the compiler (to tell it that GObject*s are guaranteed to be aligned to at least the alignment of the largest basic type). Would adding an architecture-dependent __attribute__((aligned(x))) to struct GObject do that (where x is calculated at configure time as the maximum alignof() all the basic types)? I can’t test it (no suitable ARM box at the moment), so I haven’t made the change. (On that note, I haven’t tested any of these patches on ARM.)

Christian, what do you think?

Review of attachment 365250 [details] [review]:

::: gobject/gobject.c
@@ +1664,3 @@
+{
+  return ((((guintptr) (void *) object) %
+             MAX (_g_alignof (gdouble),

I wonder if it makes sense to have this in glib-config.h? I assume this is still completely resolvable at compile time at least.

Also, have you checked to see if the compiler is smart enough to convert this into:

  (objectptr & (maxval-1)) == 0

Review of attachment 365251 [details] [review]:

Cool

Review of attachment 365252 [details] [review]:

Cool

(In reply to Christian Hergert from comment #13)
> Review of attachment 365250 [details] [review] [review]:
> 
> ::: gobject/gobject.c
> @@ +1664,3 @@
> +{
> +  return ((((guintptr) (void *) object) %
> +             MAX (_g_alignof (gdouble),
> 
> I wonder if it makes sense to have this in glib-config.h? I assume this is
> still completely resolvable at compile time at least.

It probably would, though we could do that as a separate follow-up.

> Also, have you checked to see if the compiler is smart enough to convert
> this into:
> 
>   (objectptr & (maxval-1)) == 0

No, but I should do.

Have you thought about the questions in my last two paragraphs in comment #12?

(In reply to Philip Withnall from comment #12)
> Writing the last patch made me wonder whether perhaps we should instead be
> making stronger guarantees about the alignments of GObjects: for example,
> the GSlice guarantee that they’re aligned to the power of 2 greater than or
> equal to the struct size; or a guarantee that they’re always as aligned as
> their member with the largest alignment.
> 
> I was also wondering about how to expose this runtime alignment information
> to the compiler (to tell it that GObject*s are guaranteed to be aligned to
> at least the alignment of the largest basic type). Would adding an
> architecture-dependent __attribute__((aligned(x))) to struct GObject do that
> (where x is calculated at configure time as the maximum alignof() all the
> basic types)? I can’t test it (no suitable ARM box at the moment), so I
> haven’t made the change. (On that note, I haven’t tested any of these
> patches on ARM.)

Sorry, forgot to respond here.

For attribute alignment, __attribute__((aligned(8)) (or whatever our max aligned type) isn't enough, because that will only work for GCC/Clang/Mingw. We need both an alignment prefix and suffix so we can support MSVC.

G_ALIGNED_BEGIN(8)
typedef struct {
  ...
} GObject
G_ALIGNED_END(8);

MSVC uses a prefix like: __declspec(align(8))

Anyway, here is what we use in libdazzle:

#if defined(_MSC_VER)
# define DZL_ALIGNED_BEGIN(_N) __declspec(align(_N))
# define DZL_ALIGNED_END(_N)
#else
# define DZL_ALIGNED_BEGIN(_N)
# define DZL_ALIGNED_END(_N) __attribute__((aligned(_N)))
#endif

(In reply to Philip Withnall from comment #12)
> Writing the last patch made me wonder whether perhaps we should instead be
> making stronger guarantees about the alignments of GObjects: for example,
> the GSlice guarantee that they’re aligned to the power of 2 greater than or
> equal to the struct size; or a guarantee that they’re always as aligned as
> their member with the largest alignment.

I’d also like Colin’s and Matthias’ opinions on this.

Colin? Matthias?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1231.