Re: fix alignment issue on SPARC processors

On Fri, Dec 05, 2014 at 02:59:06PM +0100, Uli Schlachter wrote:
> Hi Nicolas,
> 
> Am 04.12.2014 um 20:44 schrieb Nicolas Setton:
> [...]
> >> 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.
> 
> Could you test the attached patch? Also, this is pretty much the idea that I had
> in mind. Compared to your patch, it does not add a hole to struct _pool_chunk on
> 64bit platforms (in fact, nothing should change at all on 64bit).
> 
> I basically did no testing at all on this. The test suite run on the image
> backend does not crash. So it would also be nice if others could take a look at
> this and spot my mistakes. Pointer arithmetic is scary.
> 
> Cheers,
> Uli
> -- 
> "In the beginning the Universe was created. This has made a lot of
>  people very angry and has been widely regarded as a bad move."

Inline patches -gt attachments.  ;-)

> >From 9f318fc51963c221faf26b2877804d106a31b3e9 Mon Sep 17 00:00:00 2001
> From: Uli Schlachter <[email protected]>
> Date: Fri, 5 Dec 2014 14:43:26 +0100
> Subject: [PATCH] tor-scan-converter: Correctly align 64bit types
> 
> On 32bit SPARC the scan converter was causing a SIGBUS due to an unaligned
> memory access while accessing an int64_t. This memory accessing was to struct
> quorem's rem member.
> 
> This crash occurred because the rot-scan-converter contains its own

s/rot-/tor-/

> implementation of a memory pool. This implementation only guarantees an
> alignment of sizeof(void *), which is less than what a 64 bit type requires on
> 32bit platforms. This 4 byte alignment is guaranteed, because struct _pool_chunk
> (which is the struct that is used for managing free space) contains elements of
> that size and so the size of that struct is a multiple of this size as well.
> 
> This problem was introduced with commit 03c3d4b7c15.
> 
> To fix this problem, this commit introduces a int64_t member to struct
> _pool_chunk that marks the beginning of the free data space. Thanks to this, the
> compiler ensures proper alignment and sizeof(struct _pool_chunk) becomes a
> multiple of 8.
> 
> However, previously the end of the struct marked the beginning of the data and
> sizeof() was used for correctly calculating offsets to the data section. So,
> just adding such a member would work, but would also waste some memory. To avoid
> this, this commit also changes the rest of the pool implementation to
> accommodate.
> 
> Reported-by: Nicolas Setton <[email protected]>
> Signed-off-by: Uli Schlachter <[email protected]>
> ---
>  src/cairo-tor-scan-converter.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/src/cairo-tor-scan-converter.c b/src/cairo-tor-scan-converter.c
> index 14922d0..7adb5ab 100644
> --- a/src/cairo-tor-scan-converter.c
> +++ b/src/cairo-tor-scan-converter.c
> @@ -277,7 +277,8 @@ struct _pool_chunk {
>       * chunk in the pool header. */
>      struct _pool_chunk *prev_chunk;
>  
> -    /* Actual data starts here.	 Well aligned for pointers. */
> +    /* Actual data starts here. Well aligned even for 64 bit types. */
> +    int64_t data;
>  };
>  
>  /* A memory pool.  This is supposed to be embedded on the stack or
> @@ -299,8 +300,11 @@ struct pool {
>  
>      /* Header for the sentinel chunk.  Directly following the pool
>       * struct should be some space for embedded elements from which
> -     * the sentinel chunk allocates from. */
> -    struct _pool_chunk sentinel[1];
> +     * the sentinel chunk allocates from. This is expressed as a char
> +     * array so that the 'int64_t data' member of _pool_chunk isn't
> +     * included. This way embedding struct pool in other structs works
> +     * without wasting space. */
> +    char sentinel[sizeof(struct _pool_chunk) - sizeof(int64_t)];
>  };
>  
>  /* A polygon edge. */
> @@ -474,8 +478,9 @@ static struct _pool_chunk *
>  _pool_chunk_create(struct pool *pool, size_t size)
>  {
>      struct _pool_chunk *p;
> +    size_t sizeof_chunk = sizeof(struct _pool_chunk) - sizeof(int64_t);
>  
> -    p = malloc(size + sizeof(struct _pool_chunk));
> +    p = malloc(sizeof_chunk + size);

Using sizeof_chunk is a nice cleanup, and I'd think the compiler would
optimize it away, although since this same phrase is used in the
sentinel declaration, maybe it's worth making it a preprocessor
definition so it can be used in both places?

>      if (unlikely (NULL == p))
>  	longjmp (*pool->jmp, _cairo_error (CAIRO_STATUS_NO_MEMORY));
>  
> @@ -489,10 +494,10 @@ pool_init(struct pool *pool,
>  	  size_t embedded_capacity)
>  {
>      pool->jmp = jmp;
> -    pool->current = pool->sentinel;
> +    pool->current = (void*) &pool->sentinel[0];
>      pool->first_free = NULL;
>      pool->default_capacity = default_capacity;
> -    _pool_chunk_init(pool->sentinel, NULL, embedded_capacity);
> +    _pool_chunk_init(pool->current, NULL, embedded_capacity);
>  }
>  
>  static void
> @@ -502,7 +507,7 @@ pool_fini(struct pool *pool)
>      do {
>  	while (NULL != p) {
>  	    struct _pool_chunk *prev = p->prev_chunk;
> -	    if (p != pool->sentinel)
> +	    if (p != (void *) &pool->sentinel[0])

Is the deref/ref of sentinel just for clarity or is this actually needed
for the fix.  (Fine either way, I'm just curious.)

>  		free(p);
>  	    p = prev;
>  	}
> @@ -542,14 +547,14 @@ _pool_alloc_from_new_chunk(
>  	chunk = _pool_chunk_create (pool, capacity);
>      pool->current = chunk;
>  
> -    obj = ((unsigned char*)chunk + sizeof(*chunk) + chunk->size);
> +    obj = ((unsigned char*)&chunk->data + chunk->size);

Nice, this makes it easier to grok.

>      chunk->size += size;
>      return obj;
>  }
>  
>  /* Allocate size bytes from the pool.  The first allocated address
> - * returned from a pool is aligned to sizeof(void*).  Subsequent
> - * addresses will maintain alignment as long as multiples of void* are
> + * returned from a pool is aligned to 8 bytes.  Subsequent
> + * addresses will maintain alignment as long as multiples of 8 are
>   * allocated.  Returns the address of a new memory area or %NULL on
>   * allocation failures.	 The pool retains ownership of the returned
>   * memory. */
> @@ -559,7 +564,7 @@ pool_alloc (struct pool *pool, size_t size)
>      struct _pool_chunk *chunk = pool->current;
>  
>      if (size <= chunk->capacity - chunk->size) {
> -	void *obj = ((unsigned char*)chunk + sizeof(*chunk) + chunk->size);
> +	void *obj = ((unsigned char*)&chunk->data + chunk->size);
>  	chunk->size += size;
>  	return obj;
>      } else {
> @@ -573,16 +578,16 @@ pool_reset (struct pool *pool)
>  {
>      /* Transfer all used chunks to the chunk free list. */
>      struct _pool_chunk *chunk = pool->current;
> -    if (chunk != pool->sentinel) {
> -	while (chunk->prev_chunk != pool->sentinel) {
> +    if (chunk != (void *) &pool->sentinel[0]) {
> +	while (chunk->prev_chunk != (void *) &pool->sentinel[0]) {
>  	    chunk = chunk->prev_chunk;
>  	}
>  	chunk->prev_chunk = pool->first_free;
>  	pool->first_free = pool->current;
>      }
>      /* Reset the sentinel as the current chunk. */
> -    pool->current = pool->sentinel;
> -    pool->sentinel->size = 0;
> +    pool->current = (void *) &pool->sentinel[0];
> +    pool->current->size = 0;

This is intentional to set the current chunk's size to 0?

Assuming it is, 

Reviewed-by: Bryce Harrington <[email protected]>

>  }
>  
>  /* Rewinds the cell list's cursor to the beginning.  After rewinding
> -- 
> 2.1.3
> 


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