OpenBSD 4.9/i386 segfaults

[Andy Dougherty]

On Mon, 18 Jul 2011, Christoph Otto wrote:

> On Mon, 18 Jul 2011 14:06 -0400, "Andy Dougherty"
> <doughera at lafayette.edu> wrote:
> > OpenBSD 4.9/i386 failed on parrot master as follows:
> >
> > <snip>
> >
> > git bisect blames
> >
> > 1d270a0331242a9e50170697e0b5d7d4d65096f9 is the first bad commit
> > commit 1d270a0331242a9e50170697e0b5d7d4d65096f9 Author: chromatic
> > <chromatic at wgz.org> Date:   Tue Jul 5 17:03:38 2011 -0700
> >
> >     [GC] Fixed Win32-killing type errors in d789b5.
> >
> >
> > --
> >     Andy Dougherty          doughera at lafayette.edu
> 
> What happens when you revert that commit?  If that lets you get the
> build working again, let us know and we can revert it, pull it into its
> own branch and investigate after the release.
> Thanks for reporting this.

Yes, reverting that commit "fixes" it, but based on the commit message, I 
imagine reverting it would break Win32, so that's not a good option 
either.

Staring at that patch for a while, I think I found the problem -- when 
arena or ptr have their high bit set, conversion to a signed type (namely 
ptrdiff_t) results in negative numbers, which causes the <= test to be 
backwards.  The suggested patch below avoids the sign extension errors by 
only using ptrdiff_t to hold differences of pointers.  As a side effect, 
the conditional can be simplified.

I have only tested this on 32-bit OpenBSD with gcc.

diff --git a/src/gc/fixed_allocator.c b/src/gc/fixed_allocator.c
index 7e4883d..425c472 100644
--- a/src/gc/fixed_allocator.c
+++ b/src/gc/fixed_allocator.c
@@ -510,19 +510,15 @@ pool_is_owned(ARGMOD(Pool_Allocator *pool), ARGIN(void *ptr))
 
         /* We can cache these values. All arenas are same size */
         const ptrdiff_t             a_size  = arena_size(pool);
-        const ptrdiff_t             ptritem = (ptrdiff_t)ptr;
         const ptrdiff_t             objsize = pool->object_size;
 
         while (arena) {
-            const ptrdiff_t arena_item = (ptrdiff_t)(arena + 1);
+            const Pool_Allocator_Arena *arena_item   = arena + 1;
+            const ptrdiff_t ptr_diff = (char *) ptr - (const char *) arena_item;
 
-            if (arena_item <= ptritem) {
-                const ptrdiff_t ptr_diff = ptritem - arena_item;
-
-                if (ptr_diff < a_size
+            if (ptr_diff >= 0 && ptr_diff < a_size
                 &&  ptr_diff % pool->object_size == 0)
                     return 1;
-            }
 
             arena = arena->next;
         }


-- 
    Andy Dougherty		doughera at lafayette.edu