gobject declare macros cause alignment warnings on armhf/armhf/mipsel

Philip Withnall @pwithnall said:

(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.

Philip Withnall @pwithnall closed a related bug:

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

Christian Hergert @chergert said:

If it's not 2sizeof(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).

Philip Withnall @pwithnall said:

(In reply to Christian Hergert from comment 3)

If it's not 2sizeof(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.

Christian Hergert @chergert said:

(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).

Philip Withnall @pwithnall said:

(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?

Christian Hergert @chergert said:

(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.

Christian Hergert @chergert said:

(In reply to Christian Hergert from comment 7)

Largest basic type probably.

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

Philip Withnall @pwithnall submitted a patch:

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

Patch 365250, "gobject: Assert that GObjects are at least as aligned as basic types":
gobject-Assert-that-GObjects-are-at-least-as-align.patch

Philip Withnall @pwithnall submitted a patch:

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

Patch 365251, "gtype: Eliminate -Wcast-align warnings with G_TYPE_CHECK_INSTANCE_CAST":
gtype-Eliminate--Wcast-align-warnings-with-GTYPECH.patch

Philip Withnall @pwithnall submitted a patch:

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

Patch 365252, "gobject: Add advice on larger alignment requirements for GObject members":
gobject-Add-advice-on-larger-alignment-requirement.patch

Philip Withnall @pwithnall said:

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?

Christian Hergert @chergert reviewed patch 365250:

::: 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

Christian Hergert @chergert reviewed patch 365251:

Cool

Christian Hergert @chergert reviewed patch 365252:

Cool

Philip Withnall @pwithnall said:

(In reply to Christian Hergert from comment 13)

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.

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?

Christian Hergert @chergert said:

(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

Philip Withnall @pwithnall said:

(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.

Philip Withnall @pwithnall said:

Colin? Matthias?

added ARM label

mentioned in merge request !92 (merged)

mentioned in issue #1581 (closed)

changed milestone to %2.62.0

assigned to @pwithnall

changed milestone to %2.63.0

changed milestone to %2.63.1

changed milestone to %2.63.2

mentioned in issue #1855

changed milestone to %2.63.4

changed milestone to %2.63.5

changed milestone to %2.65.0

Due to lack of person–hours, this missed the feature/API freeze for 2.64, so it’s been punted to 2.65.

changed milestone to %2.65.3

changed milestone to %2.67.0

changed milestone to %2.67.1

We also see issues with this on macOS w/ Clang on x86_64.

It would be good if someone with access to an ARM/macOS machine could take this on, as I can’t reproduce it and have so far been unable to find time to take it on. Unassigning myself.

unassigned @pwithnall

mentioned in commit 336c34db

mentioned in merge request !1741 (closed)

mentioned in commit cfed0f6d

mentioned in commit 9260f6a8

changed milestone to %2.67.2

changed milestone to %2.67.3

changed milestone to %2.67.4

changed milestone to %2.67.5

changed milestone to %2.69.0

changed milestone to %2.69.1

changed milestone to %2.69.2

gobject declare macros cause alignment warnings on armhf/armhf/mipsel

Submitted by Christian Hergert `@chergert`

Description

Blocking

Designs

Child items ...

Activity

gobject declare macros cause alignment warnings on armhf/armhf/mipsel

Submitted by Christian Hergert @chergert

Description

Blocking

Activity

Submitted by Christian Hergert `@chergert`