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

Submitted by Adrian Johnson on Feb. 20, 2016, 10:49 p.m.

Details

Message ID 56C8ED67.7060702@redneon.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Adrian Johnson Feb. 20, 2016, 10:49 p.m.
On 12/02/16 10:19, Simon Richter wrote:
> Hi,
> 
> On 11.02.2016 21:00, Uli Schlachter wrote:
> 
>> Where is this cast back to a pointer that you talk about?
> 
> There are two instances in cairo-scaled-font.c, in lines 480 and 2827.
> Both of them read
> 
>     scaled_font = (cairo_scaled_font_t *) page->cache_entry.hash;
> 
> and the pointer is dereferenced afterwards.
> 
> I agree with your assessment that this should really be a hash value
> only, and using the lower bits might work for that (although the
> bottommost bits are probably all zero due to alignment, so it is not a
> very good hash), but fixing that requires more changes.
> 
>    Simon

Attached is a patch to avoid storing the pointer in the hash value.

Patch hide | download patch | download mbox

From 8e520e9b7bf1197a932bb3d063c0a7a7115a7698 Mon Sep 17 00:00:00 2001
From: Adrian Johnson <ajohnson@redneon.com>
Date: Sun, 21 Feb 2016 09:07:40 +1030
Subject: [PATCH] scaled-font: don't store pointer in hash value

---
 src/cairo-scaled-font.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index ac80c97..b2557d4 100644
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -85,7 +85,7 @@  static cairo_cache_t cairo_scaled_glyph_page_cache;
 #define CAIRO_SCALED_GLYPH_PAGE_SIZE 32
 struct _cairo_scaled_glyph_page {
     cairo_cache_entry_t cache_entry;
-
+    cairo_scaled_font_t *scaled_font;
     cairo_list_t link;
 
     unsigned int num_glyphs;
@@ -477,7 +477,7 @@  _cairo_scaled_glyph_page_pluck (void *closure)
 
     assert (! cairo_list_is_empty (&page->link));
 
-    scaled_font = (cairo_scaled_font_t *) page->cache_entry.hash;
+    scaled_font = page->scaled_font;
 
     CAIRO_MUTEX_LOCK (scaled_font->mutex);
     _cairo_scaled_glyph_page_destroy (scaled_font, page);
@@ -2824,7 +2824,7 @@  _cairo_scaled_glyph_page_can_remove (const void *closure)
     const cairo_scaled_glyph_page_t *page = closure;
     const cairo_scaled_font_t *scaled_font;
 
-    scaled_font = (cairo_scaled_font_t *) page->cache_entry.hash;
+    scaled_font = page->scaled_font;
     return scaled_font->cache_frozen == 0;
 }
 
@@ -2853,6 +2853,7 @@  _cairo_scaled_font_allocate_glyph (cairo_scaled_font_t *scaled_font,
 	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
 
     page->cache_entry.hash = (unsigned long) scaled_font;
+    page->scaled_font = scaled_font;
     page->cache_entry.size = 1; /* XXX occupancy weighting? */
     page->num_glyphs = 0;
 
-- 
2.1.4


Comments

Uli Schlachter Feb. 21, 2016, 3:05 p.m.
Am 20.02.2016 um 23:49 schrieb Adrian Johnson:
> On 12/02/16 10:19, Simon Richter wrote:
>> Hi,
>>
>> On 11.02.2016 21:00, Uli Schlachter wrote:
>>
>>> Where is this cast back to a pointer that you talk about?
>>
>> There are two instances in cairo-scaled-font.c, in lines 480 and 2827.
>> Both of them read
>>
>>     scaled_font = (cairo_scaled_font_t *) page->cache_entry.hash;
>>
>> and the pointer is dereferenced afterwards.
>>
>> I agree with your assessment that this should really be a hash value
>> only, and using the lower bits might work for that (although the
>> bottommost bits are probably all zero due to alignment, so it is not a
>> very good hash), but fixing that requires more changes.
>>
>>    Simon
> 
> Attached is a patch to avoid storing the pointer in the hash value.

Thanks! This looks fine to me (count this as a R-b if one is needed and no one
else dares).

And just for reference, apparently this was introduced in the following commit
(before it there was a cairo_scaled_glyph_page_key_t containing a
cairo_cache_entry_t and a cairo_scaled_font_t*):

  commit 9c80392ac415e7f07c71261d280ac4376d3c8471
  Author: Chris Wilson <chris@chris-wilson.co.uk>
  Date:   Mon Mar 16 19:31:38 2009 +0000

      [scaled-font] Lean and mean global glyph cache.

Cheers,
Uli