On Fri, Jul 18, 2008 at 11:08 PM, Nikodemus Siivola
<demoss@...> wrote:
> 1.0.18.22: DX arguments in non-let-converted local calls
>
> * When a non-let function has dynamic extent arguments, the
> combination must end its block, or stack analysis will miss the
> cleanup, and stack will be popped too soon.
>
> Index: locall.lisp
> ===================================================================
> RCS file: /cvsroot/sbcl/sbcl/src/compiler/locall.lisp,v
> retrieving revision 1.85
> retrieving revision 1.86
> diff -u -d -r1.85 -r1.86
> --- locall.lisp 8 May 2008 11:52:05 -0000 1.85
> +++ locall.lisp 18 Jul 2008 20:07:59 -0000 1.86
> @@ -76,6 +76,11 @@
> (not (lvar-dynamic-extent arg)))
> append (handle-nested-dynamic-extent-lvars arg) into dx-lvars
> finally (when dx-lvars
> + ;; A call to non-LET with DX args must end its block,
> + ;; otherwise stack analysis will not see the combination and
> + ;; the associated cleanup/entry.
> + (unless (eq :let (functional-kind fun))
> + (node-ends-block call))
> (binding* ((before-ctran (node-prev call))
> (nil (ensure-block-start before-ctran))
> (block (ctran-block before-ctran))
I realized two things 15 minutes after committing this:
* We never see a :LET here, and even if some refactoring did make :LETs
appear here, they would still have to end their blocks -- so the
conditional is pointless. I'm committing a patch that removes it shortly.
* In HANDLE-NESTED-DYNAMIC-EXTENT-LVARS we have this
;; Stack analysis wants DX value generators to end their
;; blocks. Uses of mupltiple used LVARs already end their blocks,
;; so we just need to process used-once LVARs.
(when (node-p uses)
(node-ends-block uses))
which I *THINK* is a not right -- not in the sense of being harmful,
but in the sense that as far as I can tell, it is not the generators
that need to end their blocks -- just the calls, so that MAP-BLOCK-NLXES
sees the cleanups we insert.
At least I cannot trigger any bad behaviour if I delete this one when
the new NODE-ENDS-BLOCK is in place... but I'm not 100% sure I'm not
missing something.
I'm not touching this bit now, especially this late in the release
cycle, but comments would be most welcome.
Cheers,
-- Nikodemus

Thread view

On Fri, Jul 18, 2008 at 11:08 PM, Nikodemus Siivola
<demoss@...> wrote:
> 1.0.18.22: DX arguments in non-let-converted local calls
>
> * When a non-let function has dynamic extent arguments, the
> combination must end its block, or stack analysis will miss the
> cleanup, and stack will be popped too soon.
>
> Index: locall.lisp
> ===================================================================
> RCS file: /cvsroot/sbcl/sbcl/src/compiler/locall.lisp,v
> retrieving revision 1.85
> retrieving revision 1.86
> diff -u -d -r1.85 -r1.86
> --- locall.lisp 8 May 2008 11:52:05 -0000 1.85
> +++ locall.lisp 18 Jul 2008 20:07:59 -0000 1.86
> @@ -76,6 +76,11 @@
> (not (lvar-dynamic-extent arg)))
> append (handle-nested-dynamic-extent-lvars arg) into dx-lvars
> finally (when dx-lvars
> + ;; A call to non-LET with DX args must end its block,
> + ;; otherwise stack analysis will not see the combination and
> + ;; the associated cleanup/entry.
> + (unless (eq :let (functional-kind fun))
> + (node-ends-block call))
> (binding* ((before-ctran (node-prev call))
> (nil (ensure-block-start before-ctran))
> (block (ctran-block before-ctran))
I realized two things 15 minutes after committing this:
* We never see a :LET here, and even if some refactoring did make :LETs
appear here, they would still have to end their blocks -- so the
conditional is pointless. I'm committing a patch that removes it shortly.
* In HANDLE-NESTED-DYNAMIC-EXTENT-LVARS we have this
;; Stack analysis wants DX value generators to end their
;; blocks. Uses of mupltiple used LVARs already end their blocks,
;; so we just need to process used-once LVARs.
(when (node-p uses)
(node-ends-block uses))
which I *THINK* is a not right -- not in the sense of being harmful,
but in the sense that as far as I can tell, it is not the generators
that need to end their blocks -- just the calls, so that MAP-BLOCK-NLXES
sees the cleanups we insert.
At least I cannot trigger any bad behaviour if I delete this one when
the new NODE-ENDS-BLOCK is in place... but I'm not 100% sure I'm not
missing something.
I'm not touching this bit now, especially this late in the release
cycle, but comments would be most welcome.
Cheers,
-- Nikodemus