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.
> * 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.
> * 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 :-/
> * 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...)
> * src/runtime/gencgc.c
> (hunk at @@ -3611,6 +3665,158 @@) at "lr_code_offset = ..." there's
> some whitespace offset damage.
> (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.
> * 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.
> * src/runtime/interrupt.c
> (hunk at @@ -305,7 +306,14) whitespace
> (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
> 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".
> 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...