[cairo] [PATCH 2/4] scaled fonts: Use wide enough type for pointer arithmetic

[Uli Schlachter]

Am 10.02.2016 um 21:49 schrieb Simon Richter:
> The "unsigned long" type on Windows is only 32 bit wide, so conversion from
> and to pointers is unsafe.
> 
> Replace with intptr_t, which is guaranteed to be large enough.
> ---
>  src/cairo-cache-private.h       | 2 +-
>  src/cairo-hash.c                | 2 +-
>  src/cairo-scaled-font-subsets.c | 4 ++--
>  src/cairo-scaled-font.c         | 4 ++--
>  src/cairo-types-private.h       | 2 +-
>  5 files changed, 7 insertions(+), 7 deletions(-)
> 

In your "0/4"-mail, you write:

> I'm slightly less sure about the changes to the scaled fonts, but it
> appears that one of the pointers generated from casting an integer back is
> then dereferenced, so it would indeed be good to keep all the bits.
> However, this is where we introduce lots of overhead.

Since this talks about this patch, I'm replying here.

Where is this cast back to a pointer that you talk about? I haven't looked at
the actual code, but from the patch this looks like it just wants a hash and the
code can already handle hash collisions. Plus the high bits are likely the same
for all pointers anyway, so just using the low ones is The Right Thing(tm).

What am I missing?

> diff --git a/src/cairo-cache-private.h b/src/cairo-cache-private.h
> index 24b6d0b..275552c 100644
> --- a/src/cairo-cache-private.h
> +++ b/src/cairo-cache-private.h
> @@ -84,7 +84,7 @@
>   * not be initialized if so desired.
>   **/
>  typedef struct _cairo_cache_entry {
> -    unsigned long hash;
> +    intptr_t hash;
>      unsigned long size;
>  } cairo_cache_entry_t;
>  
> diff --git a/src/cairo-hash.c b/src/cairo-hash.c
> index 928c74b..8709918 100644
> --- a/src/cairo-hash.c
> +++ b/src/cairo-hash.c
> @@ -340,7 +340,7 @@ _cairo_hash_table_lookup (cairo_hash_table_t *hash_table,
>  {
>      cairo_hash_entry_t *entry;
>      unsigned long table_size, i, idx, step;
> -    unsigned long hash = key->hash;
> +    intptr_t hash = key->hash;
>  
>      entry = hash_table->cache[hash & 31];

The high bits look really important here :-)

>      if (entry && entry->hash == hash && hash_table->keys_equal (key, entry))
> diff --git a/src/cairo-scaled-font-subsets.c b/src/cairo-scaled-font-subsets.c
> index 74bfb9e..6080a41 100644
> --- a/src/cairo-scaled-font-subsets.c
> +++ b/src/cairo-scaled-font-subsets.c
> @@ -255,12 +255,12 @@ _cairo_sub_font_init_key (cairo_sub_font_t	*sub_font,
>  {
>      if (sub_font->is_scaled)
>      {
> -        sub_font->base.hash = (unsigned long) scaled_font;
> +        sub_font->base.hash = (intptr_t) scaled_font;
>          sub_font->scaled_font = scaled_font;
>      }
>      else
>      {
> -        sub_font->base.hash = (unsigned long) scaled_font->font_face;
> +        sub_font->base.hash = (intptr_t) scaled_font->font_face;
>          sub_font->scaled_font = scaled_font;
>      }
>  }
> diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
> index ac80c97..1a3483a 100644
> --- a/src/cairo-scaled-font.c
> +++ b/src/cairo-scaled-font.c
> @@ -639,7 +639,7 @@ _cairo_scaled_font_compute_hash (cairo_scaled_font_t *scaled_font)
>      hash = _hash_matrix_fnv (&scaled_font->ctm, hash);
>      hash = _hash_mix_bits (hash);
>  
> -    hash ^= (unsigned long) scaled_font->original_font_face;
> +    hash ^= (intptr_t) scaled_font->original_font_face;
>      hash ^= cairo_font_options_hash (&scaled_font->options);
>  
>      /* final mixing of bits */
> @@ -2852,7 +2852,7 @@ _cairo_scaled_font_allocate_glyph (cairo_scaled_font_t *scaled_font,
>      if (unlikely (page == NULL))
>  	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
>  
> -    page->cache_entry.hash = (unsigned long) scaled_font;
> +    page->cache_entry.hash = (intptr_t) scaled_font;
>      page->cache_entry.size = 1; /* XXX occupancy weighting? */
>      page->num_glyphs = 0;
>  
> diff --git a/src/cairo-types-private.h b/src/cairo-types-private.h
> index 3d15d96..8a540cc 100644
> --- a/src/cairo-types-private.h
> +++ b/src/cairo-types-private.h
> @@ -147,7 +147,7 @@ struct _cairo_observer {
>   * the entry need not be initialized if so desired.
>   **/
>  struct _cairo_hash_entry {
> -    unsigned long hash;
> +    intptr_t hash;
>  };
>  
>  struct _cairo_array {
> 


-- 
99 little bugs in the code
99 little bugs in the code
Take one down, patch it around
117 little bugs in the code
  -- @irqed