Re: [Sbcl-devel] gencgc/ppc candidate 4 patch available

[Cyrus H. <ch...@bo...>]

On Feb 21, 2006, at 8:12 AM, Christophe Rhodes wrote:

> In patch order:
>
> * src/assembly/ppc/array.lisp
>
>   Turning off the storage to 0 at the end of the vector is fine; from
>   a brief read of the assembly, x86 and x86-64 don't store a 0 there
>   either; it was probably some workaround for a long-dead version of
>   SunOS.  Have you tried with it on, though?  If not, then saying that
>   you haven't would be nice.

I tried it in various versions along the way. Didn't try at the end.  
Commented to this effect.

> * src/code/ppc-vm.lisp
>
>   *SCAVENGE-READ-ONLY-SPACE* controls code that is commented out in
>   SBCL; furthermore, since it's not propagated by genesis (since it's
>   unexported), it's not going to do anything any time soon in SBCL.
>   That hunk can be removed.

Done.

> * src/compiler/generic/genesis.lisp
>
>   A comment here for the unwary warning them that the offset fixup
>   stuff is only half-finished would be good.

Commented.

> * src/compiler/ppc/c-call.lisp
>
>   Under what circumstances can NULL-TN not have NIL in it when calling
>   a callback?

Coming in from C code, I don't think it may be set up properly yet.  
The x86-64 version uses nil-value and null-tn seemed to be failing  
for me, so I switched to nil-value.

> * src/compiler/ppc/macros.lisp
>
>   Allocation macro comment: doesn't really follow the guidelines in
>   STYLE.  The ";;; Allocation macro" header can just go, and then the
>   next sentence can probably be reworked."

Done. And added FIXME and comment about stack-p being ignored.

> * src/runtime/alloc.c
>
>   See other mail :-/

Uggh...

> * src/runtime/gc.h
>
>   There's some whitespace lossage here; basically the C runtime files
>   should be indented with c-basic-offset 4 and no tabs.  Additionally,
>   it might be worth surrounding all of those #defines with #ifdef
>   LISP_FEATURE_GENCGC, to make the point that they're not needed for
>   CheneyGC.  (Or are they?  If they are, then this patch won't build
>   on non-x86 non-ppc platforms, but see below...)

ifdef'ing LISP_FEATURE_GENCGC.

> * src/runtime/gencgc.c
>
>   (hunk at @@ -3611,6 +3665,158 @@) at "lr_code_offset = ..." there's
>   some whitespace offset damage.

fixed

>   (hunk at @@ -4266,16 +4485,26 @@) I didn't even begin to understand
>   the added comment here, I'm afraid.  Is it explaining why the check
>   is off?

Comment and ifdef removed.

>   (hunk at @@ -4310,7 +4539,7 @@) This is the other half of the
>   problem noted in my other mail, but in addition there's some
>   whitespace damage here.

more ugh.

> * src/runtime/globals.h
>
>   If I understand correctly, gencgc no longer has a non-global
>   dynamic_space_free_pointer, so the warning comment can go away.

Reworked comment.


> * src/runtime/interrupt.c
>
>   (hunk at @@ -305,7 +306,14) whitespace
fixed.

>   (hunk at @@ -360,11 +368,13) While calling
>   arch_clear_pseudo_atomic_interrupted() unconditionally in
>   interrupt_handle_pending is probably a good idea, it will probably
>   cause the system to fail to build on non-x86 non-ppc platforms,
>   because those architectures aren't carefully factored into
>   implementing that call as a function, but instead do things like
>             *os_context_register_addr(context, reg_ALLOC) &= ~7;
>   inline.  (lovely, yes).  I think it probably isn't too hard to
>   factor the other platforms, but it's something else that needs to be
>   done.  (I can test on sparc; in fact I'm building one that I expect
>   to fail as I write)

deferring pending your tests.

> * src/runtime/ppc-arch.c
>
>   We can delete the pc stuff with more violence.  It's utterly
>   unnecessary.
done

>   The comment above the definition of enable_some_signals() is at
>   least misleading; I would expect SIGILL not to be used on ppc.
>   Similarly the comment above the call to enable_some_signals().  What
>   are we enabling these signals for?  To allow a possible call to
>   e.g. SUB-GC or a lisp-side signal handler to cons?  Why was this not
>   an issue before?

I'm not exactly sure. I cribbed this from rtoy. Oh, I see, in CMUCL  
they handle a pending interrupt and call MAYBE_GC directly from  
within alloc. We, OTOH, set GC_PENDING, set p_a_i and let the  
interrupt stuff handle it. This can probably go away. I'll investigate.

>   You misspelt "its" as "it's".  HTH, HAND, etc :-)

again, I blame rtoy. :-)

> * src/runtime/ppc-arch.h
>
>   Added comment but no corresponding code..

yeah, that stuff moved. deleted.

> * src/runtime/ppc-darwin-spacelist.h
>
>   The first hunk has an extra space before "#define
>   N_SEGMENTS_TO_PRODUCE 5".
>
Fixed.

> And that's it!  Thanks for reading!

Thanks for the comments.

> Though this seems like a lot, if it hadn't been for the one-and-a-half
> large issues, regarding allocation and
> arch_clear_pseudo_atomic_interrupted(), and the fact that my computer
> was broken, they would have been fixed up silently as part of my
> review process; it's only because sadly there are still these niggly
> showstoppers that I am delegating these mostly trivial fixups.  This
> list should absolutely not detract from Cyrus' achievement of
> understanding the system well enough to get the thing running; we're
> now talking about a thin layer of extra icing on a very rich cake.

Well, now on to the tricky bits...

Cyrus