[cairo] fix alignment issue on SPARC processors

[Nicolas Setton]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Pad-against-alignment-issue.patch
Type: application/octet-stream
Size: 1146 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20141204/0e1150ae/attachment-0001.obj>