Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[Stefan Monnier]

> -  FOR_EACH_FRAME (tail, frame)
> -    free_glyphs (XFRAME (frame));
> +  if (!NILP (Vframe_list))
> +    FOR_EACH_FRAME (tail, frame)
> +      free_glyphs (XFRAME (frame));
 
I don't understand:

    #define FOR_EACH_FRAME(list_var, frame_var)     \
      for ((list_var) = (eassume (CONSP (Vframe_list)), Vframe_list); \
           (CONSP (list_var)                        \
            && (frame_var = XCAR (list_var), true)); \
           list_var = XCDR (list_var))

So, if Vframe_list is Qnil, the `CONSP (list_var)` test should make us
exit the loop right from the start.

Is the problem caused by eassume (CONSP (Vframe_list))?
If so, maybe we should simply remove it.


        Stefan

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On 12/15/2017 08:23 AM, Stefan Monnier wrote:
> Is the problem caused by eassume (CONSP (Vframe_list))?
> If so, maybe we should simply remove it.

That's a good idea. However, some calls to FOR_EACH_FRAME do assume that 
frame-list is non-nil, while others don't. So when we remove the eassume 
from FOR_EACH_FRAME, we should add it to callers that have the 
assumption (otherwise --enable-gcc-warnings would sometimes rightly 
complain). I installed the attached.


[-- Attachment #2: 0001-FOR_EACH_FRAME-no-longer-assumes-frame-list.patch --]
[-- Type: text/x-patch, Size: 4037 bytes --]

From 3716d4c5f02abbcc5bad5f0c884302687bcbbb7e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 15 Dec 2017 09:07:52 -0800
Subject: [PATCH] FOR_EACH_FRAME no longer assumes frame-list

This cleans up a recent fix related to Bug#29661.
Suggested by Stefan Monnier in:
https://lists.gnu.org/r/emacs-devel/2017-12/msg00544.html
* src/frame.c (next_frame, prev_frame, delete_frame):
Restore debugging checks that Vframe_list is non-nil,
as FOR_EACH_FRAME no longer has these checks.
(delete_frame): Remove no-longer-needed checks that Vframe_list is
non-nil, as FOR_EACH_FRAME no longer assumes that.
* src/frame.h (FOR_EACH_FRAME): Do not assume Vframe_list is non-nil.
---
 src/frame.c | 13 ++++++++-----
 src/frame.h |  5 ++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/frame.c b/src/frame.c
index 66d1b5c759..63fa8abb7d 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -1607,6 +1607,8 @@ next_frame (Lisp_Object frame, Lisp_Object minibuf)
   Lisp_Object f, tail;
   int passed = 0;
 
+  eassume (CONSP (Vframe_list));
+
   while (passed < 2)
     FOR_EACH_FRAME (tail, f)
       {
@@ -1629,6 +1631,8 @@ prev_frame (Lisp_Object frame, Lisp_Object minibuf)
 {
   Lisp_Object f, tail, prev = Qnil;
 
+  eassume (CONSP (Vframe_list));
+
   FOR_EACH_FRAME (tail, f)
     {
       if (EQ (frame, f) && !NILP (prev))
@@ -1914,6 +1918,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
   if (f == sf)
     {
       Lisp_Object tail;
+      eassume (CONSP (Vframe_list));
 
       /* Look for another visible frame on the same terminal.
 	 Do not call next_frame here because it may loop forever.
@@ -2058,7 +2063,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
 
   /* If we've deleted the last_nonminibuf_frame, then try to find
      another one.  */
-  if (f == last_nonminibuf_frame && !NILP (Vframe_list))
+  if (f == last_nonminibuf_frame)
     {
       last_nonminibuf_frame = 0;
 
@@ -2076,7 +2081,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
 
   /* If there's no other frame on the same kboard, get out of
      single-kboard state if we're in it for this kboard.  */
-  if (kb != NULL && !NILP (Vframe_list))
+  if (kb != NULL)
     {
       /* Some frame we found on the same kboard, or nil if there are none.  */
       Lisp_Object frame_on_same_kboard = Qnil;
@@ -2093,9 +2098,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force)
   /* If we've deleted this keyboard's default_minibuffer_frame, try to
      find another one.  Prefer minibuffer-only frames, but also notice
      frames with other windows.  */
-  if (kb != NULL
-      && EQ (frame, KVAR (kb, Vdefault_minibuffer_frame))
-      && !NILP (Vframe_list))
+  if (kb != NULL && EQ (frame, KVAR (kb, Vdefault_minibuffer_frame)))
     {
       /* The last frame we saw with a minibuffer, minibuffer-only or not.  */
       Lisp_Object frame_with_minibuf = Qnil;
diff --git a/src/frame.h b/src/frame.h
index a3b7763643..a5d4e4fc88 100644
--- a/src/frame.h
+++ b/src/frame.h
@@ -1149,8 +1149,7 @@ default_pixels_per_inch_y (void)
 /* FOR_EACH_FRAME (LIST_VAR, FRAME_VAR) followed by a statement is a
    `for' loop which iterates over the elements of Vframe_list.  The
    loop will set FRAME_VAR, a Lisp_Object, to each frame in
-   Vframe_list in succession and execute the statement.  Vframe_list
-   should be nonempty, so the body is executed at least once.  LIST_VAR
+   Vframe_list in succession and execute the statement.  LIST_VAR
    should be a Lisp_Object too; it is used to iterate through the
    Vframe_list.  Note that this macro walks over child frames and
    the tooltip frame as well.
@@ -1160,7 +1159,7 @@ default_pixels_per_inch_y (void)
    something which executes the statement once.  */
 
 #define FOR_EACH_FRAME(list_var, frame_var)	\
-  for ((list_var) = (eassume (CONSP (Vframe_list)), Vframe_list); \
+  for ((list_var) = Vframe_list;		\
        (CONSP (list_var)			\
 	&& (frame_var = XCAR (list_var), true)); \
        list_var = XCDR (list_var))
-- 
2.14.3

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[martin rudalics]

 > That's a good idea. However, some calls to FOR_EACH_FRAME do assume
 >  that frame-list is non-nil, while others don't. So when we remove the
 >  eassume from FOR_EACH_FRAME, we should add it to callers that have
 >  the assumption (otherwise --enable-gcc-warnings would sometimes
 >  rightly complain). I installed the attached.

I'm confused.  How did you recognize the "callers that have the
assumption"?  What about the call in check_glyph_memory?

martin

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[Paul Eggert]

On 12/15/2017 10:17 AM, martin rudalics wrote:
> How did you recognize the "callers that have the
> assumption"?  What about the call in check_glyph_memory?

I recognized callers that either --enable-gcc-warnings complained about 
(because code after the FOR_EACH_FRAME loop clearly had undefined 
behavior unless the loop iterated at least once), and callers where 
historically there was an eassert that checked the assumption. 
check_glyph_memory doesn't fall into either category; and now that I 
look at its implementation it's clear that check_glyph_memory is not 
making the assumption.

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[martin rudalics]

 > I recognized callers that either --enable-gcc-warnings complained
 > about (because code after the FOR_EACH_FRAME loop clearly had
 > undefined behavior unless the loop iterated at least once),

Which ones were these?  Neither next_frame nor prev_frame exhibit such
behavior IMO.

 > and
 > callers where historically there was an eassert that checked the
 > assumption.

How far did you go back in history?

 > check_glyph_memory doesn't fall into either category; and
 > now that I look at its implementation it's clear that
 > check_glyph_memory is not making the assumption.

Then you should have reverted that change too.  I've done that now.

Thanks, martin

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[Paul Eggert]

martin rudalics wrote:
>  > I recognized callers that either --enable-gcc-warnings complained
>  > about (because code after the FOR_EACH_FRAME loop clearly had
>  > undefined behavior unless the loop iterated at least once),
> 
> Which ones were these?

The code with undefined behavior is in delete_frame, after its 3rd use of 
FOR_EACH_FRAME. This loop head is of the form 'FOR_EACH_FRAME (tail, frame1) 
...' and the code after the loop assumes that frame1 is initialized, an 
assumption that is false if Vframe_list is nil.

> Neither next_frame nor prev_frame exhibit such behavior IMO.

That's right. However, commit 8720f601e715e5f1d41f7cf863a525a1cc1bc12c removed 
these functions' assertions that frame-list is non-nil, so I thought it wise to 
resurrect them.

>  > and
>  > callers where historically there was an eassert that checked the
>  > assumption.
> 
> How far did you go back in history?

To commit 8720f601e715e5f1d41f7cf863a525a1cc1bc12c, which is the commit that 
inserted the (now-removed) assumption that frame-list is non-nil when 
FOR_EACH_FRAME is first executed.

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[martin rudalics]

 > The code with undefined behavior is in delete_frame, after its 3rd use
 > of FOR_EACH_FRAME. This loop head is of the form 'FOR_EACH_FRAME
 > (tail, frame1) ...' and the code after the loop assumes that frame1 is
 > initialized, an assumption that is false if Vframe_list is nil.

Sorry for being completely dense but

       eassume (CONSP (Vframe_list));

now means that we can assume that `frame-list' is non-empty.  Isn't that
assumption incorrect in the scenario of the bug we're discussing here?

 >> Neither next_frame nor prev_frame exhibit such behavior IMO.
 >
 > That's right. However, commit 8720f601e715e5f1d41f7cf863a525a1cc1bc12c
 > removed these functions' assertions that frame-list is non-nil, so I
 > thought it wise to resurrect them.

I don't think that these assertions did any good and would remove them
at least on master.

martin

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[Paul Eggert]

martin rudalics wrote:

>        eassume (CONSP (Vframe_list));
> 
> now means that we can assume that `frame-list' is non-empty.  Isn't that
> assumption incorrect in the scenario of the bug we're discussing here?

Yes and no. The assumption is incorrect for this particular bug. But it is 
correct for the two instances of eassume (CONSP (Vframe_list)) that I resurrected.

>  > commit 8720f601e715e5f1d41f7cf863a525a1cc1bc12c
>  > removed these functions' assertions that frame-list is non-nil, so I
>  > thought it wise to resurrect them.
> 
> I don't think that these assertions did any good and would remove them
> at least on master.

Sounds good to me. Assertions that aren't useful for finding bugs are more 
trouble than they're worth. I kept them only because I didn't know the context well.

Re: [Emacs-diffs] emacs-26 9bf66c6: Don't run FOR_EACH_FRAME when there's no frame left (Bug#29961)

[martin rudalics]

 >>        eassume (CONSP (Vframe_list));
 >>
 >> now means that we can assume that `frame-list' is non-empty.  Isn't that
 >> assumption incorrect in the scenario of the bug we're discussing here?
 >

 > Yes and no. The assumption is incorrect for this particular bug. But
 > it is correct for the two instances of eassume (CONSP (Vframe_list))
 > that I resurrected.

Those two instances have no connection with the bug.  It's the one in
delete_frame that caused the segfault.

martin