GNU bug report logs - #72279
[PATCH] Non-local exits from outside the lexical scope are caught by cl-block

Previous Next

To reply to this bug, email your comments to 72279 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Non-local exits from outside the lexical scope are caught
 by cl-block
Date: Wed, 24 Jul 2024 19:36:13 +0200

[Message part 1 (text/plain, inline)]

Since the thrown and caught tags in `cl-block' and `cl-return-from' are
interned and deterministic, a `cl-block' catches `cl-return-from's with
the corresponding names even from outside the current lexical scope.

The only mechanism in place to stop this is the compiler macro around
cl--block-catch, which removes the block if no approriate returns are
found, however not only is this bound to break if the compiler macro
fails to expand, a valid exit is all that is needed to work around this.

  (defun foo () (cl-return "ruh roh"))
  (cl-block nil (foo) (cl-return t)) ; => "ruh ruh"

The first patch attached attempts to solve this by moving the
functionality of the wrapper compiler macros to the macros themselves
and by using uninterned symbols for the thrown and caught tags,
communicated by the block to the corresponding returns.  All the
existing tests seemed to run just fine but I did not do any
comprehensive testing (and there doesn't appear to be any relevant
suites either).

I do take minor issue with `macroexpand-all'ing all things inside a
block, making debugging via macrostep really annoying, but I don't know
of a better solution, outside of communicating the tag during
evaluation, which would look something like the second patch.

PS. I would also like to have a discussion about a problem that I have
noticed when trying to build with the second patch, maybe here maybe in
another bug: Because struct slots are defined using `cl-defsubst', the
whole body is wrapped in a `cl-block'.  The only reason `setf' works
with such slots is because `cl-block' expands into the body itself when
there are no `cl-return's.  If it were to instead expand into a `catch'
- whether because there is a `cl-return' or because `cl-block' is
modified to always expandi into a `catch' as it is in my second patch -
the setf will expand into (setf catch) which is not defined.  I see two
possible solutions, either define a (setf catch) or switch to defsubst
instead of cl-defsubst.

[0001-Use-uninterned-tags-in-cl-block-remove-block-wrapper.patch (text/x-patch, attachment)]

[0002-Avoid-macroexpanding-in-cl-block.patch (text/x-patch, attachment)]

Message #8 received at 72279 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <acorallo <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 72279 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#72279: [PATCH] Non-local exits from outside the lexical
 scope are caught by cl-block
Date: Thu, 25 Jul 2024 11:15:12 -0400

Thuna <thuna.cing <at> gmail.com> writes:

> Since the thrown and caught tags in `cl-block' and `cl-return-from' are
> interned and deterministic, a `cl-block' catches `cl-return-from's with
> the corresponding names even from outside the current lexical scope.
>
> The only mechanism in place to stop this is the compiler macro around
> cl--block-catch, which removes the block if no approriate returns are
> found, however not only is this bound to break if the compiler macro
> fails to expand, a valid exit is all that is needed to work around this.
>
>   (defun foo () (cl-return "ruh roh"))
>   (cl-block nil (foo) (cl-return t)) ; => "ruh ruh"

Hi Thuna,

yes I agree this is not optimal.  We should not even get to evaluating
the second form as we should error on the first (I think your patch
does that).

> The first patch attached attempts to solve this by moving the
> functionality of the wrapper compiler macros to the macros themselves
> and by using uninterned symbols for the thrown and caught tags,
> communicated by the block to the corresponding returns.  All the
> existing tests seemed to run just fine but I did not do any
> comprehensive testing (and there doesn't appear to be any relevant
> suites either).

Have you tried some general use with Emacs compiled with this patch?

> I do take minor issue with `macroexpand-all'ing all things inside a
> block, making debugging via macrostep really annoying, but I don't know
> of a better solution, outside of communicating the tag during
> evaluation, which would look something like the second patch.


IIUC installing first.patch we keep the same capability of cleaning-up
all un-necessary catches if no approriate throws are found correct?

> PS. I would also like to have a discussion about a problem that I have
> noticed when trying to build with the second patch, maybe here maybe in
> another bug: Because struct slots are defined using `cl-defsubst', the
> whole body is wrapped in a `cl-block'.  The only reason `setf' works
> with such slots is because `cl-block' expands into the body itself when
> there are no `cl-return's.  If it were to instead expand into a `catch'
> - whether because there is a `cl-return' or because `cl-block' is
> modified to always expandi into a `catch' as it is in my second patch -
> the setf will expand into (setf catch) which is not defined.  I see two
> possible solutions, either define a (setf catch) or switch to defsubst
> instead of cl-defsubst.
>
>>From 4027c50645260a202e45a2a074dfeb48468394c1 Mon Sep 17 00:00:00 2001
> From: Thuna <thuna.cing <at> gmail.com>
> Date: Wed, 24 Jul 2024 18:41:25 +0200
> Subject: [PATCH 1/2] Use uninterned tags in cl-block, remove block wrappers
>

[...]

> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> index 2e501005bf7..31b88aec889 100644
> --- a/lisp/emacs-lisp/cl-macs.el
> +++ b/lisp/emacs-lisp/cl-macs.el
> @@ -889,6 +889,8 @@ cl-etypecase
>  
>  ;;; Blocks and exits.
>  
> +(defvar cl--active-block-names nil)

I think would be nice to document this variable with how its content is
supposed to look like.

I'm Ccing Stefan maybe he has opinions on these patches.

Also I don't see you in the copyright file.  Would you like to do the
FSF paperwork in order to be able to contribute non trivial patches to
Emacs?

Thanks!

  Andrea

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#72279: [PATCH] Non-local exits from outside the lexical scope
 are caught by cl-block
Date: Thu, 25 Jul 2024 19:00:23 +0200

> >   (defun foo () (cl-return "ruh roh"))
> >   (cl-block nil (foo) (cl-return t)) ; => "ruh ruh"
> 
> yes I agree this is not optimal.  We should not even get to evaluating
> the second form as we should error on the first (I think your patch
> does that).

Yes, with my first patch applied, the call to `foo' signals a `no-catch'
in (cl-return "ruh roh").  It also emits a warning while macroexpanding.

> > The first patch attached attempts to solve this by moving the
> > functionality of the wrapper compiler macros to the macros themselves
> > and by using uninterned symbols for the thrown and caught tags,
> > communicated by the block to the corresponding returns.  All the
> > existing tests seemed to run just fine but I did not do any
> > comprehensive testing (and there doesn't appear to be any relevant
> > suites either).
> 
> Have you tried some general use with Emacs compiled with this patch?

No, sorry, I didn't.  I am currently working on a different thing which
changes the definitions for these macros yet again so I proposed this
more as an interliminary change so that my later changes don't end up
moving where cl-block is calculated AND modify what it does all in one
step.

> > I do take minor issue with `macroexpand-all'ing all things inside a
> > block, making debugging via macrostep really annoying, but I don't know
> > of a better solution, outside of communicating the tag during
> > evaluation, which would look something like the second patch.
>  
> IIUC installing first.patch we keep the same capability of cleaning-up
> all un-necessary catches if no approriate throws are found correct?

Correct, I have not changed that behavior; if no appropriate
`cl-return-from's are found within the body of the `cl-block', then the
`cl-block' simply expands into the (macroexpanded, though that can be
fixed) body.

> > PS. I would also like to have a discussion about a problem that I have
> > noticed when trying to build with the second patch, maybe here maybe in
> > another bug: Because struct slots are defined using `cl-defsubst', the
> > whole body is wrapped in a `cl-block'.  The only reason `setf' works
> > with such slots is because `cl-block' expands into the body itself when
> > there are no `cl-return's.  If it were to instead expand into a `catch'
> > - whether because there is a `cl-return' or because `cl-block' is
> > modified to always expandi into a `catch' as it is in my second patch -
> > the setf will expand into (setf catch) which is not defined.  I see two
> > possible solutions, either define a (setf catch) or switch to defsubst
> > instead of cl-defsubst.
> >
> >>From 4027c50645260a202e45a2a074dfeb48468394c1 Mon Sep 17 00:00:00 2001
> > From: Thuna <thuna.cing <at> gmail.com>
> > Date: Wed, 24 Jul 2024 18:41:25 +0200
> > Subject: [PATCH 1/2] Use uninterned tags in cl-block, remove block wrappers
> >
> 
> [...]
> 
> > diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> > index 2e501005bf7..31b88aec889 100644
> > --- a/lisp/emacs-lisp/cl-macs.el
> > +++ b/lisp/emacs-lisp/cl-macs.el
> > @@ -889,6 +889,8 @@ cl-etypecase
> >  
> >  ;;; Blocks and exits.
> >  
> > +(defvar cl--active-block-names nil)
> 
> I think would be nice to document this variable with how its content is
> supposed to look like.

This was a variable which already existed; I simply moved it here,
though I wouldn't mind documenting it.  A decent place to start from is:
"A list of the currently active `cl-block' forms.

Each element is of the form (NAME VAR FOUNDP) where NAME is the name of
the block as it is written in `cl-block' and `cl-return-from', VAR is
the tag as it is caught and thrown by `catch' and `throw', and FOUNDP is
non-nil if a `cl-return' or `cl-return-from' with the same name was
found.

This variable should be set such that the modifications can only be seen
within the same lexical scope."

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Craig Topham via RT <copyright-clerk <at> fsf.org>
Subject: Re: bug#72279: FSF copyright assignment
Date: Thu, 25 Jul 2024 19:22:43 +0200

> Also I don't see you in the copyright file.  Would you like to do the
> FSF paperwork in order to be able to contribute non trivial patches to
> Emacs?

I have applied for an assignment a bit over two years ago now.
Unfortunately, the problem which stopped it from going through still
persists to date, namely that the university in which I am a student
(which is based in EU if that helps) refuses to sign the copyright
disclaimer - or more specifically that I cannot get a response from the
legal department to begin with.  Craig has previously reached out on my
behalf as well, but as far as I am aware they have similarly not had
much success.

The only way forward that I can see is a. do not consider them a
possibly claimant to my work an forgo the copyright disclaimer or
b. wait a year after which I will (hopefully) graduate and hope that the
university I go to for my post-grad is more open to communication.  I do
not believe that b. is bound to see much success, but I do also do not
know if a. is a viable option for the FSF.

Message #17 received at 72279 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 72279 <at> debbugs.gnu.org
Subject: Re: bug#72279: [PATCH] Non-local exits from outside the lexical
 scope are caught by cl-block
Date: Thu, 25 Jul 2024 19:24:52 -0400

>   (defun foo () (cl-return "ruh roh"))
>   (cl-block nil (foo) (cl-return t)) ; => "ruh ruh"
>
> The first patch attached attempts to solve this by moving the
> functionality of the wrapper compiler macros to the macros themselves
> and by using uninterned symbols for the thrown and caught tags,
> communicated by the block to the corresponding returns.  All the
> existing tests seemed to run just fine but I did not do any
> comprehensive testing (and there doesn't appear to be any relevant
> suites either).

BTW, for the second patch a simpler solution is to expand

    (cl-block A FOO)
to
    (let ((cl--block--A ',(make-symbol "A")))
      (catch cl--block--A FOO))

and

    (cl-return B FOO)
to
    (throw cl--block--B FOO)

which will signal an "unknown variable cl--block--B" if a `cl-return` is
used outside of its block.

But the optimization of `cl-block` when the tag is not used is
important, so I don't think it's a good approach.

> I do take minor issue with `macroexpand-all'ing all things inside a
> block, making debugging via macrostep really annoying, but I don't know
> of a better solution, outside of communicating the tag during
> evaluation, which would look something like the second patch.

I think in theory it's possible to avoid the `macroexpand-all`, but it
requires a compiler-macro which (assuming we generate code like
I outlined above) will have to recognize those `(catch <VAR> ...)` where
the tag is not used anywhere else.

This is because compiler macros are executed by `macroexpand-all` both
before and after performing the normal macro expansions, so they do get
to see the code after `macroexpand-all`.  But it can be slightly tricky
to get it right and reliable (because it has to be careful to not mess
up the code if it hasn't been macroexpanded yet), and it has to walk the
whole code, which means that it needs to copy a fair bit of the code of
`macroexpand-all`.

> If it were to instead expand into a `catch' - whether because there is
> a `cl-return' or because `cl-block' is modified to always expandi into
> a `catch' as it is in my second patch - the setf will expand into
> (setf catch) which is not defined.

Yup, as I said, the "optimization" is important (most of the
`cl-block`s are implicit and unused, IME).

> I see two possible solutions, either define a (setf catch) or switch
> to defsubst instead of cl-defsubst.

I don't think we can implement a `setf catch` (tho we could probably
come up with a hack which works in practice for those
cl-defstruct slots).  🙁

IIRC for the slot accessors `defsubst` would lead to worse code than
what we currently get with `cl-defsubst` so it wouldn't be as good, but
I believe we could use `define-inline` instead.

AFAIC, your first patch looks good to me.


        Stefan

Message #20 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#72279: [PATCH] Non-local exits from outside the lexical scope
 are caught by cl-block
Date: Fri, 26 Jul 2024 02:41:59 +0200

> BTW, for the second patch a simpler solution is to expand
>
>     (cl-block A FOO)
> to
>     (let ((cl--block--A ',(make-symbol "A")))
>       (catch cl--block--A FOO))
>
> and
>
>     (cl-return B FOO)
> to
>     (throw cl--block--B FOO)
>
> which will signal an "unknown variable cl--block--B" if a `cl-return` is
> used outside of its block.

Aha, yes, that would indeed be simpler.

> But the optimization of `cl-block` when the tag is not used is
> important, so I don't think it's a good approach.

I don't have any particular complaints against keeping this
optimization, but for the record would you be able to expand on what
exactly the problem is?  It doesn't seem obvious to me that unused
`catch'es would make such a meaningful difference.

> Yup, as I said, the "optimization" is important (most of the
> `cl-block`s are implicit and unused, IME).

It would be nice if this would be documented at least in a comment above
`cl-block', even though it's unlikely that it will see too many changes.

>> I see two possible solutions, either define a (setf catch) or switch
>> to defsubst instead of cl-defsubst.
>
> I don't think we can implement a `setf catch` (tho we could probably
> come up with a hack which works in practice for those
> cl-defstruct slots).  🙁
>
> IIRC for the slot accessors `defsubst` would lead to worse code than
> what we currently get with `cl-defsubst` so it wouldn't be as good, but
> I believe we could use `define-inline` instead.

That sounds like a solution.  Defining a hack for `setf catch' seems
like it would lead to a lot of problems down the line, so it would be
better to put it off until someone comes up with a good way to do it
(though I doubt it is possible given dynamic exits).

Message #23 received at 72279 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <acorallo <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 72279 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#72279: [PATCH] Non-local exits from outside the lexical
 scope are caught by cl-block
Date: Fri, 26 Jul 2024 03:39:46 -0400

Thuna <thuna.cing <at> gmail.com> writes:

>> BTW, for the second patch a simpler solution is to expand
>>
>>     (cl-block A FOO)
>> to
>>     (let ((cl--block--A ',(make-symbol "A")))
>>       (catch cl--block--A FOO))
>>
>> and
>>
>>     (cl-return B FOO)
>> to
>>     (throw cl--block--B FOO)
>>
>> which will signal an "unknown variable cl--block--B" if a `cl-return` is
>> used outside of its block.
>
> Aha, yes, that would indeed be simpler.
>
>> But the optimization of `cl-block` when the tag is not used is
>> important, so I don't think it's a good approach.
>
> I don't have any particular complaints against keeping this
> optimization, but for the record would you be able to expand on what
> exactly the problem is?  It doesn't seem obvious to me that unused
> `catch'es would make such a meaningful difference.

Unused catches are a problem because they bloat bytecode unnecessary,
also they prevent a number of optimizations in the native compiler as
well.  Essentially every time you have a landing pad like a catch you
cannot make assumptions on where you are landing from and what's the
state of your machine.

It's very important we keep on removing unnecessary catches, that's why
I was asking in my previous mail.


  Andrea

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.