Re: fix alignment issue on SPARC processors

Hello Uli,

thank you for the feedback.

>> There is an alignment issue on SPARC processors which causes a SIGBUS when
>> trying to write data to edges in the polygon scan converter.
> 
> Could you provide some more information about this? I guess you are on 32bit
> SPARC since otherwise all members of struct _pool_chunk already be 64bit in size.

Right, this is 32bit SPARC (Solaris 10).

> So size_t is a 32 bit type for you and the prev_chunk member thus starts aligned
> to a 64 bit boundary for you. Since it is a 32 bit pointer, the end of the
> struct is no longer 32bit-aligned. You fix this by adding a 16-bit member in
> front of prev_chunk so that (plus the 16-bit hole that this forces) the end of
> struct becomes 64-bit aligned.
> 
> Did I understand this correctly so far?

Right, see below.

> Also, where exactly does the SIGBUS happen? I guess that you are refering to the
> int64_t dy member of struct edge which e.g. polygon_add_edge() allocates from
> some pool via pool_alloc(). So if the struct itself isn't aligned to a 64 bit
> boundary, the dy member isn't either.


The SIGBUS happens when writing to edge->dxdy.rem, which is a long long.

Here is a reproducer:

int main(void) {
  struct polygon * polygon;
  struct edge * e;

  polygon = (struct polygon *) malloc(sizeof (struct polygon));
  polygon_init (polygon, NULL);

  e = pool_alloc (polygon->edge_pool.base, sizeof (struct edge));

  e->dxdy.quo = 42;   /* this works */
  e->dxdy.rem = 42;   /* SIGBUS here */

  /* sanity check */
  printf("%d\n", e->dxdy.quo);
  printf("%d\n", e->dxdy.rem);
  return 0;
} 

There are two cases that I could find where the pointer arithmetic
is in disagreement with the alignment rule.

In pool_alloc, we have this:

    struct _pool_chunk *chunk = pool->current;

    if (size <= chunk->capacity - chunk->size) {
        void *obj = ((unsigned char*)chunk + sizeof(*chunk) + chunk->size);
        chunk->size += size;
        return obj;

and we have the equivalent in _pool_alloc_from_new_chunk:

    obj = ((unsigned char*)chunk + sizeof(*chunk) + chunk->size);

We have sizeof(*chunk) = 12 on both cases, so obj->dxdy.rem is
not aligned after this addition.

Bumping sizeof(*chunk) to a multiple of 8 circumvents the problem in 
both cases.

> It would be nice if you submit your patch in the format produced by git
> format-patch. That includes a commit message and tracks you as the author of
> this, instead of someone else claiming the fame for this. All of the above
> reasoning and thoughts about this change could go into the commit message.

Attached. I am also happy with someone else suggesting a better patch and 
claiming the fame for this.

> The big downside of this: On 64bit platforms this adds an unused 64 bit hole to
> the struct.

Right, although my understanding of this part is that these are ephemeral, and
we want to privilege speed over memory consumption.

> I would instead suggest to add a new member "int64_t data" to the struct with a
> suitable comment explaining why it is of type int64_t. Instead of using
> ((unsigned char *)chunk + sizeof(*chunk)) for calculating the beginning of the
> data area of the chunk, the code could then use (unsigned char*)&chunk->data
> which IMO is more readable anyway.

I didn't understand this suggestion.

> However, I guess that such a change is more complicated since it requires
> reviewing the code more carefully since now sizeof(*chunk) is no longer the size
> of the "chunk header"...

Right. Perhaps we could also pre-allocate the array (embedded) as part of the pool
rather than a separate variable after the end of the pool, and keep an index on the
first free element of the array.

Nicolas

-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo

From edd1914967c57ad08c1bf14b1537d05a5803f289 Mon Sep 17 00:00:00 2001
From: Nicolas Setton <[email protected]>
Date: Thu, 4 Dec 2014 13:09:00 -0500
Subject: [PATCH] Pad against alignment issue

Add padding to _pool_chunk to make the size of the struct
a multiple of 8 bytes, to prevent a sigbus on 32-bit SPARC
processors where the alignment for long long is strictly 64
bits.
---
 src/cairo-tor-scan-converter.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/cairo-tor-scan-converter.c b/src/cairo-tor-scan-converter.c
index 14922d0..d811825 100644
--- a/src/cairo-tor-scan-converter.c
+++ b/src/cairo-tor-scan-converter.c
@@ -273,6 +273,11 @@ struct _pool_chunk {
     /* # bytes total in this chunk */
     size_t capacity;
 
+    /* Pad to fix alignment issue on sparc: the long long integers
+     * are aligned on 64-bit boundaries. Make sure that this is the
+     * case in the memory pool by bumping the size of this struct. */
+    short padding;
+
     /* Pointer to the previous chunk or %NULL if this is the sentinel
      * chunk in the pool header. */
     struct _pool_chunk *prev_chunk;
-- 
1.9.3 (Apple Git-50)