bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Juanma Barranquero]

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

This is with an empty init.el, and the following early-init.el:

D:\...\.emacs.d> type early-init.el
(setq default-frame-alist '((left . (+ 0))))

D:\...\.emacs.d> emacs.exe

lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)

Setting the same value on init.el works.



In GNU Emacs 27.0.50 (build 1, x86_64-w64-mingw32)
 of 2019-09-16 built on ODIEFAST
Repository revision: b3e4b50578778e03327b049f7a595981bfbf3713
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 10.0.18362
System Description: Microsoft Windows 10 Home (v10.0.1903.18362.356)

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --prefix=/d/Devel/emacs/repo/trunk --with-modules
 --enable-checking=yes
 --enable-locallisppath=%emacs_dir%/../site-lisp:%emacs_dir%/share/emacs/@VER@
/site-lisp:%emacs_dir%/share/emacs/site-lisp
 'CFLAGS=-Og -ggdb3'
 PKG_CONFIG_PATH=/mingw64/lib/pkgconfig:/mingw64/share/pkgconfig'

[-- Attachment #2: Type: text/html, Size: 1248 bytes --]

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 16 Sep 2019 00:34:01 +0200
> 
> This is with an empty init.el, and the following early-init.el:
> 
> D:\...\.emacs.d> type early-init.el
> (setq default-frame-alist '((left . (+ 0))))
> 
> D:\...\.emacs.d> emacs.exe
> 
> lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)

Is this use case important enough to support?  IOW, would just
signaling an error do the job here?  If not, please explain why did
you need to set default-frame-alist in the early-init file.

Thanks.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Juanma Barranquero]

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

On Tue, Sep 17, 2019 at 6:01 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Is this use case important enough to support?

Not at this moment, no.

> IOW, would just signaling an error do the job here?

Yes, likely.

> If not, please explain why did you need to set default-frame-alist in the
early-init file.

I was just trying stuff and stumbled upon the problem.

Thanks,

   Juanma

[-- Attachment #2: Type: text/html, Size: 643 bytes --]

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Paul Eggert]

FWIW I can't reproduce the bug on Fedora 30 x86-64. Perhaps it's some 
MS-Windows thing. I can't see in the portable code how a non-fixnum 
could leak into the code that uses XFIXNUM in this area.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >> This is with an empty init.el, and the following early-init.el:
 >>
 >> D:\...\.emacs.d> type early-init.el
 >> (setq default-frame-alist '((left . (+ 0))))
 >>
 >> D:\...\.emacs.d> emacs.exe
 >>
 >> lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)
 >
 > Is this use case important enough to support?  IOW, would just
 > signaling an error do the job here?  If not, please explain why did
 > you need to set default-frame-alist in the early-init file.

Wasn't setting 'default-frame-alist' (or 'initial-frame-alist') one of
the major motivations for adding the early-init file feature?  IIRC
it's used to suppress temporarily showing the initial frame at an
unwanted position or with unwanted size.  So IMHO this should never
fail.

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Wed, 18 Sep 2019 09:45:59 +0200
> 
>  >> lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)
>  >
>  > Is this use case important enough to support?  IOW, would just
>  > signaling an error do the job here?  If not, please explain why did
>  > you need to set default-frame-alist in the early-init file.
> 
> Wasn't setting 'default-frame-alist' (or 'initial-frame-alist') one of
> the major motivations for adding the early-init file feature?

No.  the main motivation was to be able to set package.el related
variables.

> IIRC it's used to suppress temporarily showing the initial frame at
> an unwanted position or with unwanted size.  So IMHO this should
> never fail.

Feel free to fix it, if it's practical.  I chimed in after hearing
nothing from you for a few days ;-)

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >> Wasn't setting 'default-frame-alist' (or 'initial-frame-alist') one of
 >> the major motivations for adding the early-init file feature?
 >
 > No.  the main motivation was to be able to set package.el related
 > variables.

In section 49.4.6 of the Emacs manual frames "loaded before the
package system and GUI is initialized, so in it you can customize
variables that affect frame appearance as well as the package
initialization process" come first ;-)

 >> IIRC it's used to suppress temporarily showing the initial frame at
 >> an unwanted position or with unwanted size.  So IMHO this should
 >> never fail.
 >
 > Feel free to fix it, if it's practical.  I chimed in after hearing
 > nothing from you for a few days ;-)

Initially, I couldn't reproduce it (and still can't on Debian although
setting it doesn't seem to have any effect there).  But even if we
cannot fix the behavior we have to avoid the crash.  After all, this
is the first release processing an early init file.

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Thu, 19 Sep 2019 10:17:42 +0200
> 
>  > Feel free to fix it, if it's practical.  I chimed in after hearing
>  > nothing from you for a few days ;-)
> 
> Initially, I couldn't reproduce it (and still can't on Debian although
> setting it doesn't seem to have any effect there).

Try on MS-Windows, it's trivial there.

> But even if we cannot fix the behavior we have to avoid the crash.

That was my purpose when I proposed to signal an error in this case.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >> But even if we cannot fix the behavior we have to avoid the crash.
 >
 > That was my purpose when I proposed to signal an error in this case.

Should we even signal an error then?  The parameter would be applied
later anyway as soon as we can parse the display geometry.  At least
here the frame appears as expected if my early-init.el just contains

(setq default-frame-alist '((left . (- 100))))

and the patch below is used.

martin

diff --git a/src/w32fns.c b/src/w32fns.c
index 34abd026f9..3771d9d5f9 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -5427,14 +5427,16 @@ my_create_window (struct frame * f)
                                RES_TYPE_NUMBER);
    top = gui_display_get_arg (dpyinfo, Qnil, Qtop, "top", "Top",
                               RES_TYPE_NUMBER);
-  if (EQ (left, Qunbound))
-    coords[0] = CW_USEDEFAULT;
-  else
+
+  if (FIXNUMP (left))
      coords[0] = XFIXNUM (left);
-  if (EQ (top, Qunbound))
-    coords[1] = CW_USEDEFAULT;
    else
+    coords[0] = CW_USEDEFAULT;
+
+  if (FIXNUMP (top))
      coords[1] = XFIXNUM (top);
+  else
+    coords[1] = CW_USEDEFAULT;

    if (!PostThreadMessage (dwWindowsThreadId, WM_EMACS_CREATEWINDOW,
  			  (WPARAM)f, (LPARAM)coords))

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 20 Sep 2019 10:13:03 +0200
> 
>  >> But even if we cannot fix the behavior we have to avoid the crash.
>  >
>  > That was my purpose when I proposed to signal an error in this case.
> 
> Should we even signal an error then?

If we cannot do better, then yes.

> The parameter would be applied
> later anyway as soon as we can parse the display geometry.  At least
> here the frame appears as expected if my early-init.el just contains
> 
> (setq default-frame-alist '((left . (- 100))))
> 
> and the patch below is used.

If the patch below solves the original problem, please install it.  If
it doesn't, then please help me understand how is it related to the
issue at hand, because I don't think I understand.

Thanks.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Juanma Barranquero]

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

On Fri, Sep 20, 2019 at 10:13 AM martin rudalics <rudalics@gmx.at> wrote:

> Should we even signal an error then?  The parameter would be applied
> later anyway as soon as we can parse the display geometry.  At least
> here the frame appears as expected if my early-init.el just contains
>
> (setq default-frame-alist '((left . (- 100))))
>
> and the patch below is used.

It works here too. Please install it.

Thanks,

   Juanma

[-- Attachment #2: Type: text/html, Size: 616 bytes --]

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

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

 > If the patch below solves the original problem, please install it.  If
 > it doesn't, then please help me understand how is it related to the
 > issue at hand, because I don't think I understand.

It hardly makes sense to install a patch you don't understand.  So
let's get back to the original problem.  Juanma said that

 > This is with an empty init.el, and the following early-init.el:
 >
 > D:\...\.emacs.d> type early-init.el
 > (setq default-frame-alist '((left . (+ 0))))
 >
 > D:\...\.emacs.d> emacs.exe
 >
 > lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)

and when I try that on Windows 10 with a 64-bit build of master I get
the following backtrace

(gdb) bt
#0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at ../../src/emacs.c:375
#1  0x00000004002bb1d2 in die (msg=0x4009fdf3e <DEFAULT_REHASH_SIZE+5690> "FIXNUMP (a)", file=0x4009fdee0 <DEFAULT_REHASH_SIZE+5596> "../../src/lisp.h", line=1231) at ../../src/alloc.c:7245
#2  0x0000000400447414 in XFIXNUM (a=XIL(0x69699b3)) at ../../src/lisp.h:1231
#3  0x0000000400458b94 in my_create_window (f=0x76193d0) at ../../src/w32fns.c:5433
#4  0x0000000400458f5e in w32_window (f=0x76193d0, window_prompting=4, minibuffer_only=false) at ../../src/w32fns.c:5505
#5  0x000000040045af89 in Fx_create_frame (parameters=XIL(0x69835e3)) at ../../src/w32fns.c:6010
#6  0x0000000400319b96 in funcall_subr (subr=0x400957e00 <Sx_create_frame>, numargs=1, args=0xbfc658) at ../../src/eval.c:2867
#7  0x000000040031966f in Ffuncall (nargs=2, args=0xbfc650) at ../../src/eval.c:2794
#8  0x0000000400396404 in exec_byte_code (bytestr=XIL(0x61d586c), vector=XIL(0x61d40c5), maxdepth=make_fixnum(13), args_template=make_fixnum(256), nargs=1, args=0xbfcb90) at ../../src/bytecode.c:633
#9  0x000000040031a2c7 in funcall_lambda (fun=XIL(0x61d4095), nargs=1, arg_vector=0xbfcb88) at ../../src/eval.c:2989
#10 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfcb80) at ../../src/eval.c:2796
#11 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x61d58ac), vector=XIL(0x61d4055), maxdepth=make_fixnum(3), args_template=make_fixnum(257), nargs=1, args=0xbfd200) at ../../src/bytecode.c:633
#12 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x61d4005), nargs=1, arg_vector=0xbfd1f8) at ../../src/eval.c:2989
#13 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfd1f0) at ../../src/eval.c:2796
#14 0x00000004003184c5 in Fapply (nargs=2, args=0xbfd1f0) at ../../src/eval.c:2381
#15 0x0000000400319a8c in funcall_subr (subr=0x400952540 <Sapply>, numargs=2, args=0xbfd1f0) at ../../src/eval.c:2847
#16 0x000000040031966f in Ffuncall (nargs=3, args=0xbfd1e8) at ../../src/eval.c:2794
#17 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x6073844), vector=XIL(0x61d1325), maxdepth=make_fixnum(15), args_template=make_fixnum(128), nargs=1, args=0xbfd738) at ../../src/bytecode.c:633
#18 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x61d12f5), nargs=1, arg_vector=0xbfd738) at ../../src/eval.c:2989
#19 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfd730) at ../../src/eval.c:2796
#20 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x61e239c), vector=XIL(0x5fb9a0d), maxdepth=make_fixnum(14), args_template=make_fixnum(256), nargs=1, args=0xbfdd28) at ../../src/bytecode.c:633
#21 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x5fb99d5), nargs=1, arg_vector=0xbfdd20) at ../../src/eval.c:2989
#22 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfdd18) at ../../src/eval.c:2796
#23 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x638d3d4), vector=XIL(0x638d32d), maxdepth=make_fixnum(6), args_template=make_fixnum(0), nargs=0, args=0xbfe220) at ../../src/bytecode.c:633
#24 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x638d2fd), nargs=0, arg_vector=0xbfe220) at ../../src/eval.c:2989
#25 0x00000004003196b3 in Ffuncall (nargs=1, args=0xbfe218) at ../../src/eval.c:2796
#26 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x6393434), vector=XIL(0x638f585), maxdepth=make_fixnum(14), args_template=make_fixnum(0), nargs=0, args=0xbfed88) at ../../src/bytecode.c:633
#27 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x638f555), nargs=0, arg_vector=0xbfed88) at ../../src/eval.c:2989
#28 0x00000004003196b3 in Ffuncall (nargs=1, args=0xbfed80) at ../../src/eval.c:2796
#29 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x639401c), vector=XIL(0x6393605), maxdepth=make_fixnum(12), args_template=make_fixnum(0), nargs=0, args=0xbff3d0) at ../../src/bytecode.c:633
#30 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x63935d5), nargs=0, arg_vector=0xbff3d0) at ../../src/eval.c:2989
#31 0x0000000400319f4d in apply_lambda (fun=XIL(0x63935d5), args=XIL(0), count=4) at ../../src/eval.c:2926
#32 0x0000000400318106 in eval_sub (form=XIL(0x64e9d2b)) at ../../src/eval.c:2318
#33 0x00000004003173b0 in Feval (form=XIL(0x64e9d2b), lexical=XIL(0)) at ../../src/eval.c:2102
#34 0x00000004001aa0fb in top_level_2 () at ../../src/keyboard.c:1100
#35 0x00000004003154e2 in internal_condition_case (bfun=0x4001aa0d0 <top_level_2>, handlers=XIL(0x90), hfun=0x4001a9948 <cmd_error>) at ../../src/eval.c:1355
#36 0x00000004001aa153 in top_level_1 (ignore=XIL(0)) at ../../src/keyboard.c:1108
#37 0x000000040031493b in internal_catch (tag=XIL(0xdc80), func=0x4001aa101 <top_level_1>, arg=XIL(0)) at ../../src/eval.c:1116
#38 0x00000004001a9fcf in command_loop () at ../../src/keyboard.c:1069
#39 0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Lisp Backtrace:
"x-create-frame" (0xbfc658)
"x-create-frame-with-faces" (0xbfcb88)
0x61d4000 PVEC_COMPILED
"apply" (0xbfd1f0)
"frame-creation-function" (0xbfd738)
"make-frame" (0xbfdd20)
"frame-initialize" (0xbfe220)
"command-line" (0xbfed88)
"normal-top-level" (0xbff3d0)

I get a similar backtrace when I try the more reasonable

(setq default-frame-alist '((left . (- 100))))

in my early-init.el (more reasonable because, after all,

(setq default-frame-alist '((left . (+ 0))))

is equivalent to

(setq default-frame-alist '((left . 0)))

which _can_ be handled from within early-init.el).  Do we agree so
far?  If so, then obviously

   if (EQ (left, Qunbound))
     coords[0] = CW_USEDEFAULT;
   else
     coords[0] = XFIXNUM (left);

will choke when 'left' is something like '(+ 0)' or '(- 100)' since
neither of these pass the

eassert (FIXNUMP (a))

check we have in XFIXNUM.  Still agreed?  Then doing something like

   if (FIXNUMP (left))
     coords[0] = XFIXNUM (left);
   else
     coords[0] = CW_USEDEFAULT;

should fix the assertion failure if my poor understanding of C doesn't
let me down completely (the 'top' parameter needing a similar fix).
Does that reasoning still make sense to you?

And finally, with the sole evidence of my poor eyesight,

(setq default-frame-alist '((left . (- 100))))

seems to work here too, despite of the fact that for the first frame
the defaults are used.  Maybe you can try to verify (I attach the
patch for easier use this time).

So do you still think that we should signal an error?

Thanks, martin

[-- Attachment #2: left+0.diff --]
[-- Type: text/plain, Size: 764 bytes --]

diff --git a/src/w32fns.c b/src/w32fns.c
index 34abd026f9..3771d9d5f9 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -5427,14 +5427,16 @@ my_create_window (struct frame * f)
                               RES_TYPE_NUMBER);
   top = gui_display_get_arg (dpyinfo, Qnil, Qtop, "top", "Top",
                              RES_TYPE_NUMBER);
-  if (EQ (left, Qunbound))
-    coords[0] = CW_USEDEFAULT;
-  else
+
+  if (FIXNUMP (left))
     coords[0] = XFIXNUM (left);
-  if (EQ (top, Qunbound))
-    coords[1] = CW_USEDEFAULT;
   else
+    coords[0] = CW_USEDEFAULT;
+
+  if (FIXNUMP (top))
     coords[1] = XFIXNUM (top);
+  else
+    coords[1] = CW_USEDEFAULT;
 
   if (!PostThreadMessage (dwWindowsThreadId, WM_EMACS_CREATEWINDOW,
 			  (WPARAM)f, (LPARAM)coords))

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sat, 21 Sep 2019 10:51:49 +0200
> 
> I get a similar backtrace when I try the more reasonable
> 
> (setq default-frame-alist '((left . (- 100))))
> 
> in my early-init.el (more reasonable because, after all,
> 
> (setq default-frame-alist '((left . (+ 0))))
> 
> is equivalent to
> 
> (setq default-frame-alist '((left . 0)))
> 
> which _can_ be handled from within early-init.el).  Do we agree so
> far?  If so, then obviously
> 
>    if (EQ (left, Qunbound))
>      coords[0] = CW_USEDEFAULT;
>    else
>      coords[0] = XFIXNUM (left);
> 
> will choke when 'left' is something like '(+ 0)' or '(- 100)' since
> neither of these pass the
> 
> eassert (FIXNUMP (a))
> 
> check we have in XFIXNUM.  Still agreed?  Then doing something like
> 
>    if (FIXNUMP (left))
>      coords[0] = XFIXNUM (left);
>    else
>      coords[0] = CW_USEDEFAULT;
> 
> should fix the assertion failure if my poor understanding of C doesn't
> let me down completely (the 'top' parameter needing a similar fix).
> Does that reasoning still make sense to you?
> 
> And finally, with the sole evidence of my poor eyesight,
> 
> (setq default-frame-alist '((left . (- 100))))
> 
> seems to work here too, despite of the fact that for the first frame
> the defaults are used.  Maybe you can try to verify (I attach the
> patch for easier use this time).

Thanks, but I probably should have explain the nature of my confusion
better (and would have done that, should I know you will act upon it
so seriously).  Sorry about that.

Here's what confused me in this problem:

  . the FIXNUMP assertion is probably there for a reason; what is that
    reason?
  . how come we don't hit this assertion when the same expression is
    in the init file, only in the early-init file?
  . why doesn't the X build hit the same assertion in the same
    scenario?

I'm probably missing something because I don't find answers to these
questions anywhere in what you wrote.  I do understand the "mechanics"
of the patch, I just cannot convince myself it's the right fix,
without being able to answer these questions, and then assess the fix
with that knowledge in hand.  Which shouldn't prevent you from
installing it, of course: I don't have to understand everything in
what's going on in Emacs development.

> So do you still think that we should signal an error?

A red herring: I only proposed to signal an error if we cannot find a
better solution for this use case.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Juanma Barranquero]

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

On Sat, Sep 21, 2019 at 11:14 AM Eli Zaretskii <eliz@gnu.org> wrote:

>   . the FIXNUMP assertion is probably there for a reason; what is that
>     reason?

Before 572fe798cd0a00ad4a9050a7962cf8e8fbcc209b (from 2014-09-30), the
computation of left and top was done in w32_createwindow:

      /* When called with RES_TYPE_NUMBER, w32_get_arg will return zero
        for anything that is not a number and is not Qunbound.  */
      left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
RES_TYPE_NUMBER);
      top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top", RES_TYPE_NUMBER);

and anything not a number was turned to 0.

In that commit you moved the code to my_create_window and used XINT:

 +  /* When called with RES_TYPE_NUMBER, x_get_arg will return zero for
 +     anything that is not a number and is not Qunbound.  */
 +  left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
RES_TYPE_NUMBER);
 +  top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top", RES_TYPE_NUMBER);
 +  if (EQ (left, Qunbound))
 +    coords[0] = CW_USEDEFAULT;
 +  else
 +    coords[0] = XINT (left);
 +  if (EQ (top, Qunbound))
 +    coords[1] = CW_USEDEFAULT;
 +  else
 +    coords[1] = XINT (top);
 +

and these XINTs were transformed into XFIXNUM (which easserts) as part of a
big XFIXNUM/XFIXNAT change  (by Tom Tromey, in
commit d1ec3a0a8e4d7d56ebc1e4fa743130b9974ac6a8 from 2018-08-07).

So, from my ignorance, it seems like the idea was always to set non-nums to
zero, and the assertion came by accident.

    Juanma

[-- Attachment #2: Type: text/html, Size: 2435 bytes --]

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sat, 21 Sep 2019 12:02:51 +0200
> Cc: martin rudalics <rudalics@gmx.at>, 37415@debbugs.gnu.org
> 
> Before 572fe798cd0a00ad4a9050a7962cf8e8fbcc209b (from 2014-09-30), the computation of left and top
> was done in w32_createwindow:
> 
>       /* When called with RES_TYPE_NUMBER, w32_get_arg will return zero
>         for anything that is not a number and is not Qunbound.  */
>       left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left", RES_TYPE_NUMBER);
>       top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top", RES_TYPE_NUMBER);
> 
> and anything not a number was turned to 0.
> 
> In that commit you moved the code to my_create_window and used XINT:
> 
>  +  /* When called with RES_TYPE_NUMBER, x_get_arg will return zero for
>  +     anything that is not a number and is not Qunbound.  */
>  +  left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left", RES_TYPE_NUMBER);
>  +  top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top", RES_TYPE_NUMBER);
>  +  if (EQ (left, Qunbound))
>  +    coords[0] = CW_USEDEFAULT;
>  +  else
>  +    coords[0] = XINT (left);
>  +  if (EQ (top, Qunbound))
>  +    coords[1] = CW_USEDEFAULT;
>  +  else
>  +    coords[1] = XINT (top);
>  +

Yes, but contrary to the comment, in the case in point we don't get
zero from x_get_arg.  Which is why I asked all those questions.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Juanma Barranquero]

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

Curiouser and curiouser.

With no init.el or early-init.el:

   emacs --eval "(let ((default-frame-alist '((left . 1000))))
(make-frame-command))"

works (it creates an additional frame, displaced to the right).

   emacs --eval "(let ((default-frame-alist '((left . (- 0)))))
(make-frame-command))"

lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)

Or, equivalently,

  emacs -Q --no-site-file

  M-: (let ((default-frame-alist '((left . (- 0))))) (make-frame-command))
<RET>

so the bit about early-init.el seems like a red herring.



On Sat, Sep 21, 2019 at 2:27 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Juanma Barranquero <lekktu@gmail.com>
> > Date: Sat, 21 Sep 2019 12:02:51 +0200
> > Cc: martin rudalics <rudalics@gmx.at>, 37415@debbugs.gnu.org
> >
> > Before 572fe798cd0a00ad4a9050a7962cf8e8fbcc209b (from 2014-09-30), the
> computation of left and top
> > was done in w32_createwindow:
> >
> >       /* When called with RES_TYPE_NUMBER, w32_get_arg will return zero
> >         for anything that is not a number and is not Qunbound.  */
> >       left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
> RES_TYPE_NUMBER);
> >       top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top",
> RES_TYPE_NUMBER);
> >
> > and anything not a number was turned to 0.
> >
> > In that commit you moved the code to my_create_window and used XINT:
> >
> >  +  /* When called with RES_TYPE_NUMBER, x_get_arg will return zero for
> >  +     anything that is not a number and is not Qunbound.  */
> >  +  left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
> RES_TYPE_NUMBER);
> >  +  top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top", RES_TYPE_NUMBER);
> >  +  if (EQ (left, Qunbound))
> >  +    coords[0] = CW_USEDEFAULT;
> >  +  else
> >  +    coords[0] = XINT (left);
> >  +  if (EQ (top, Qunbound))
> >  +    coords[1] = CW_USEDEFAULT;
> >  +  else
> >  +    coords[1] = XINT (top);
> >  +
>
> Yes, but contrary to the comment, in the case in point we don't get
> zero from x_get_arg.  Which is why I asked all those questions.
>

[-- Attachment #2: Type: text/html, Size: 3130 bytes --]

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 > Thanks, but I probably should have explain the nature of my confusion
 > better (and would have done that, should I know you will act upon it
 > so seriously).  Sorry about that.

I hope you now better understand why I didn't respond to Juanma's
report immediately.  There are too many layers at work here.

 > Here's what confused me in this problem:
 >
 >    . the FIXNUMP assertion is probably there for a reason; what is that
 >      reason?

Which assertion?  The one in XFIXNUM?

 >    . how come we don't hit this assertion when the same expression is
 >      in the init file, only in the early-init file?

I do hit it here.  Unless some sort of bug happens before.  For
example, when using (left . foo).

 >    . why doesn't the X build hit the same assertion in the same
 >      scenario?

The X build sets 'left' only in 'gui_figure_window_size' whereafter it
immediately sanitizes it into f->left_pos which is later on used by
XCreateWindow.

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >> Before 572fe798cd0a00ad4a9050a7962cf8e8fbcc209b (from 2014-09-30), the computation of left and top
 >> was done in w32_createwindow:
 >>
 >>        /* When called with RES_TYPE_NUMBER, w32_get_arg will return zero
 >>          for anything that is not a number and is not Qunbound.  */
 >>        left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left", RES_TYPE_NUMBER);
 >>        top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top", RES_TYPE_NUMBER);
 >>
 >> and anything not a number was turned to 0.
 >>
 >> In that commit you moved the code to my_create_window and used XINT:
 >>
 >>   +  /* When called with RES_TYPE_NUMBER, x_get_arg will return zero for
 >>   +     anything that is not a number and is not Qunbound.  */
 >>   +  left = x_get_arg (dpyinfo, Qnil, Qleft, "left", "Left", RES_TYPE_NUMBER);
 >>   +  top = x_get_arg (dpyinfo, Qnil, Qtop, "top", "Top", RES_TYPE_NUMBER);
 >>   +  if (EQ (left, Qunbound))
 >>   +    coords[0] = CW_USEDEFAULT;
 >>   +  else
 >>   +    coords[0] = XINT (left);
 >>   +  if (EQ (top, Qunbound))
 >>   +    coords[1] = CW_USEDEFAULT;
 >>   +  else
 >>   +    coords[1] = XINT (top);
 >>   +
 >
 > Yes, but contrary to the comment, in the case in point we don't get
 > zero from x_get_arg.  Which is why I asked all those questions.

Well, now you really asked it ...

When called with PARAM Qleft and ALIST Qnil x_get_arg

{
   Lisp_Object tem;

   tem = Fassq (param, alist);

   if (!NILP (tem))
     ...
	  XSETCAR (XCAR (tail), Qnil);
     }
   else
     tem = Fassq (param, Vdefault_frame_alist);

   if (NILP (tem))
     ...

   return Fcdr (tem);
}

returns the cdr of the 'default-frame-alist' entry for 'left' which
can be anything.  So that comment is wrong and using x_get_arg here
probably too.  Any reason why we cannot just use f->left_pos

my_create_window (struct frame * f)
{
   MSG msg;
   static int coords[2];

   coords[0] = f->left_pos;
   coords[1] = f->top_pos;
   if (!PostThreadMessage (dwWindowsThreadId, WM_EMACS_CREATEWINDOW,
			  (WPARAM)f, (LPARAM)coords))
     emacs_abort ();
   GetMessage (&msg, NULL, WM_EMACS_DONE, WM_EMACS_DONE);
}

like the X build does?

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >     emacs --eval "(let ((default-frame-alist '((left . (- 0)))))
 > (make-frame-command))"
 >
 > lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)
 >
 > Or, equivalently,
 >
 >    emacs -Q --no-site-file
 >
 >    M-: (let ((default-frame-alist '((left . (- 0))))) (make-frame-command))
 > <RET>
 >
 > so the bit about early-init.el seems like a red herring.

Right.  Here I can simply put that in my .emacs to get the error.

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sun, 22 Sep 2019 07:54:07 +0200
> Cc: martin rudalics <rudalics@gmx.at>, 37415@debbugs.gnu.org
> 
> With no init.el or early-init.el:
> 
>    emacs --eval "(let ((default-frame-alist '((left . 1000)))) (make-frame-command))"
> 
> works (it creates an additional frame, displaced to the right).
> 
>    emacs --eval "(let ((default-frame-alist '((left . (- 0))))) (make-frame-command))"
>  
> lisp.h:1231: Emacs fatal error: assertion failed: FIXNUMP (a)
> 
> Or, equivalently,
> 
>   emacs -Q --no-site-file
> 
>   M-: (let ((default-frame-alist '((left . (- 0))))) (make-frame-command)) <RET>
> 
> so the bit about early-init.el seems like a red herring.

Indeed.  As always, it's my fault: the change you mentioned up-thread
modified the order of code execution: where previously, if
f->size_hint_flags were set, we didn't look at the left and top frame
parameters, now we examine the frame parameters _before_ looking at
size_hint_flags, and thus can try to interpret values of top and left
we never did before.

So I think Martin's patch is going in the right direction, but we
could do it even cleaner, see my other mail.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 22 Sep 2019 10:08:04 +0200
> 
>  >    . how come we don't hit this assertion when the same expression is
>  >      in the init file, only in the early-init file?
> 
> I do hit it here.  Unless some sort of bug happens before.  For
> example, when using (left . foo).

But not with (left . (- 0)), right?

>  >    . why doesn't the X build hit the same assertion in the same
>  >      scenario?
> 
> The X build sets 'left' only in 'gui_figure_window_size' whereafter it
> immediately sanitizes it into f->left_pos which is later on used by
> XCreateWindow.

So we should do something similar.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 22 Sep 2019 10:08:53 +0200
> 
> When called with PARAM Qleft and ALIST Qnil x_get_arg
> 
> {
>    Lisp_Object tem;
> 
>    tem = Fassq (param, alist);
> 
>    if (!NILP (tem))
>      ...
> 	  XSETCAR (XCAR (tail), Qnil);
>      }
>    else
>      tem = Fassq (param, Vdefault_frame_alist);
> 
>    if (NILP (tem))
>      ...
> 
>    return Fcdr (tem);
> }
> 
> returns the cdr of the 'default-frame-alist' entry for 'left' which
> can be anything.  So that comment is wrong and using x_get_arg here
> probably too.

No, I don't think using x_get_arg is wrong, because we still want to
determine whether to use CW_USEDEFAULT.

> Any reason why we cannot just use f->left_pos
> 
> my_create_window (struct frame * f)
> {
>    MSG msg;
>    static int coords[2];
> 
>    coords[0] = f->left_pos;
>    coords[1] = f->top_pos;
>    if (!PostThreadMessage (dwWindowsThreadId, WM_EMACS_CREATEWINDOW,
> 			  (WPARAM)f, (LPARAM)coords))
>      emacs_abort ();
>    GetMessage (&msg, NULL, WM_EMACS_DONE, WM_EMACS_DONE);
> }
> 
> like the X build does?

We cannot do this unless f->size_hint_flags are set so as to tell
w32_createwindow to use f->top_pos and/or f->left_pos.  Otherwise, we
should put CW_USEDEFAULT in coords[].  IOW, how about the below?

diff --git a/src/w32fns.c b/src/w32fns.c
index 34abd02..2ae84fc 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -5421,20 +5421,28 @@ my_create_window (struct frame * f)
   Lisp_Object left, top;
   struct w32_display_info *dpyinfo = &one_w32_display_info;
 
-  /* When called with RES_TYPE_NUMBER, gui_display_get_arg will return
-     zero for anything that is not a number and is not Qunbound.  */
-  left = gui_display_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
-                              RES_TYPE_NUMBER);
-  top = gui_display_get_arg (dpyinfo, Qnil, Qtop, "top", "Top",
-                             RES_TYPE_NUMBER);
-  if (EQ (left, Qunbound))
-    coords[0] = CW_USEDEFAULT;
-  else
-    coords[0] = XFIXNUM (left);
-  if (EQ (top, Qunbound))
-    coords[1] = CW_USEDEFAULT;
-  else
-    coords[1] = XFIXNUM (top);
+  if (!(f->size_hint_flags & USPosition || f->size_hint_flags & PPosition))
+    {
+      /* When called with RES_TYPE_NUMBER, and there's no 'top' or
+	 'left' parameters in the frame's parameter alist,
+	 gui_display_get_arg will return zero for anything that is
+	 neither a number nor Qunbound.  If frame parameter alist does
+	 have 'left' or 'top', they are interpreted by
+	 gui_figure_window_size, which was already called, and which
+	 sets f->size_hint_flags.  */
+      left = gui_display_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
+				  RES_TYPE_NUMBER);
+      top = gui_display_get_arg (dpyinfo, Qnil, Qtop, "top", "Top",
+				 RES_TYPE_NUMBER);
+      if (EQ (left, Qunbound))
+	coords[0] = CW_USEDEFAULT;
+      else
+	coords[0] = XFIXNUM (left);
+      if (EQ (top, Qunbound))
+	coords[1] = CW_USEDEFAULT;
+      else
+	coords[1] = XFIXNUM (top);
+    }
 
   if (!PostThreadMessage (dwWindowsThreadId, WM_EMACS_CREATEWINDOW,
 			  (WPARAM)f, (LPARAM)coords))

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >> I do hit it here.  Unless some sort of bug happens before.  For
 >> example, when using (left . foo).
 >
 > But not with (left . (- 0)), right?

With (left . (+ 0)) but maybe not with emacs -Q.  In the backtrace below
note that it gets called from 'frame-notice-user-settings':

#0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at ../../src/emacs.c:375
#1  0x00000004002bb1d2 in die (msg=0x4009fdf3e <DEFAULT_REHASH_SIZE+5690> "FIXNUMP (a)", file=0x4009fdee0 <DEFAULT_REHASH_SIZE+5596> "../../src/lisp.h", line=1231) at ../../src/alloc.c:7245
#2  0x0000000400447414 in XFIXNUM (a=XIL(0x76d7e13)) at ../../src/lisp.h:1231
#3  0x0000000400458b94 in my_create_window (f=0x9c92820) at ../../src/w32fns.c:5433
#4  0x0000000400458f5e in w32_window (f=0x9c92820, window_prompting=3, minibuffer_only=false) at ../../src/w32fns.c:5505
#5  0x000000040045af89 in Fx_create_frame (parameters=XIL(0xa03d5f3)) at ../../src/w32fns.c:6010
#6  0x0000000400319b96 in funcall_subr (subr=0x400957e00 <Sx_create_frame>, numargs=1, args=0xbfc898) at ../../src/eval.c:2867
#7  0x000000040031966f in Ffuncall (nargs=2, args=0xbfc890) at ../../src/eval.c:2794
#8  0x0000000400396404 in exec_byte_code (bytestr=XIL(0x63f586c), vector=XIL(0x63f40c5), maxdepth=make_fixnum(13), args_template=make_fixnum(256), nargs=1, args=0xbfcdd0) at ../../src/bytecode.c:633
#9  0x000000040031a2c7 in funcall_lambda (fun=XIL(0x63f4095), nargs=1, arg_vector=0xbfcdc8) at ../../src/eval.c:2989
#10 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfcdc0) at ../../src/eval.c:2796
#11 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x63f58ac), vector=XIL(0x63f4055), maxdepth=make_fixnum(3), args_template=make_fixnum(257), nargs=1, args=0xbfd440) at ../../src/bytecode.c:633
#12 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x63f4005), nargs=1, arg_vector=0xbfd438) at ../../src/eval.c:2989
#13 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfd430) at ../../src/eval.c:2796
#14 0x00000004003184c5 in Fapply (nargs=2, args=0xbfd430) at ../../src/eval.c:2381
#15 0x0000000400319a8c in funcall_subr (subr=0x400952540 <Sapply>, numargs=2, args=0xbfd430) at ../../src/eval.c:2847
#16 0x000000040031966f in Ffuncall (nargs=3, args=0xbfd428) at ../../src/eval.c:2794
#17 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x6293844), vector=XIL(0x63f1325), maxdepth=make_fixnum(15), args_template=make_fixnum(128), nargs=1, args=0xbfd978) at ../../src/bytecode.c:633
#18 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x63f12f5), nargs=1, arg_vector=0xbfd978) at ../../src/eval.c:2989
#19 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfd970) at ../../src/eval.c:2796
#20 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x640239c), vector=XIL(0x61d9a0d), maxdepth=make_fixnum(14), args_template=make_fixnum(256), nargs=1, args=0xbfdf90) at ../../src/bytecode.c:633
#21 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x61d99d5), nargs=1, arg_vector=0xbfdf88) at ../../src/eval.c:2989
#22 0x00000004003196b3 in Ffuncall (nargs=2, args=0xbfdf80) at ../../src/eval.c:2796
#23 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x63c8c74), vector=XIL(0x61e98fd), maxdepth=make_fixnum(15), args_template=make_fixnum(0), nargs=0, args=0xbfe750) at ../../src/bytecode.c:633
#24 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x61e98cd), nargs=0, arg_vector=0xbfe750) at ../../src/eval.c:2989
#25 0x00000004003196b3 in Ffuncall (nargs=1, args=0xbfe748) at ../../src/eval.c:2796
#26 0x0000000400396404 in exec_byte_code (bytestr=XIL(0x65b3c9c), vector=XIL(0x5ead275), maxdepth=make_fixnum(6), args_template=make_fixnum(0), nargs=0, args=0xbfec88) at ../../src/bytecode.c:633
#27 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x5ea78e5), nargs=0, arg_vector=0xbfec88) at ../../src/eval.c:2989
#28 0x00000004003196b3 in Ffuncall (nargs=1, args=0xbfec80) at ../../src/eval.c:2796
#29 0x0000000400395746 in bcall0 (f=XIL(0x5ea78e5)) at ../../src/bytecode.c:328
#30 0x000000040031bab1 in do_one_unbind (this_binding=0xbfed00, unwinding=true, bindflag=SET_INTERNAL_UNBIND) at ../../src/eval.c:3444
#31 0x000000040031beed in unbind_to (count=5, value=XIL(0)) at ../../src/eval.c:3574
#32 0x00000004003964bd in exec_byte_code (bytestr=XIL(0x65b401c), vector=XIL(0x65b3605), maxdepth=make_fixnum(12), args_template=make_fixnum(0), nargs=0, args=0xbff3d0) at ../../src/bytecode.c:653
#33 0x000000040031a2c7 in funcall_lambda (fun=XIL(0x65b35d5), nargs=0, arg_vector=0xbff3d0) at ../../src/eval.c:2989
#34 0x0000000400319f4d in apply_lambda (fun=XIL(0x65b35d5), args=XIL(0), count=4) at ../../src/eval.c:2926
#35 0x0000000400318106 in eval_sub (form=XIL(0x6709d2b)) at ../../src/eval.c:2318
#36 0x00000004003173b0 in Feval (form=XIL(0x6709d2b), lexical=XIL(0)) at ../../src/eval.c:2102
#37 0x00000004001aa0fb in top_level_2 () at ../../src/keyboard.c:1100
#38 0x00000004003154e2 in internal_condition_case (bfun=0x4001aa0d0 <top_level_2>, handlers=XIL(0x90), hfun=0x4001a9948 <cmd_error>) at ../../src/eval.c:1355
#39 0x00000004001aa153 in top_level_1 (ignore=XIL(0)) at ../../src/keyboard.c:1108
#40 0x000000040031493b in internal_catch (tag=XIL(0xdc80), func=0x4001aa101 <top_level_1>, arg=XIL(0)) at ../../src/eval.c:1116
#41 0x00000004001a9fcf in command_loop () at ../../src/keyboard.c:1069
#42 0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Lisp Backtrace:
"x-create-frame" (0xbfc898)
"x-create-frame-with-faces" (0xbfcdc8)
0x63f4000 PVEC_COMPILED
"apply" (0xbfd430)
"frame-creation-function" (0xbfd978)
"make-frame" (0xbfdf88)
"frame-notice-user-settings" (0xbfe750)
0x5ea78e0 PVEC_COMPILED
"normal-top-level" (0xbff3d0)

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 > No, I don't think using x_get_arg is wrong, because we still want to
 > determine whether to use CW_USEDEFAULT.

Hmm ...

 > We cannot do this unless f->size_hint_flags are set so as to tell
 > w32_createwindow to use f->top_pos and/or f->left_pos.  Otherwise, we
 > should put CW_USEDEFAULT in coords[].  IOW, how about the below?
[...]
 > +  if (!(f->size_hint_flags & USPosition || f->size_hint_flags & PPosition))
 > +    {
 > +      /* When called with RES_TYPE_NUMBER, and there's no 'top' or
 > +	 'left' parameters in the frame's parameter alist,
 > +	 gui_display_get_arg will return zero for anything that is
 > +	 neither a number nor Qunbound.  If frame parameter alist does
 > +	 have 'left' or 'top', they are interpreted by
 > +	 gui_figure_window_size, which was already called, and which
 > +	 sets f->size_hint_flags.  */

So you mean when size hints are not set, we are sure that
gui_display_get_arg does not find anything in 'default-frame-alist'
(ignoring, BTW 'initial-frame-alist') and finds a number here.  This
looks a bit fragile to me.  Isn't the fact that left/top are unbound
sufficient that we should use CW_USEDEFAULT and f->left_pos/f->top_pos
otherwise.  Or don't you want to call gui_display_get_arg twice?

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 22 Sep 2019 19:53:32 +0200
> 
>  >> I do hit it here.  Unless some sort of bug happens before.  For
>  >> example, when using (left . foo).
>  >
>  > But not with (left . (- 0)), right?
> 
> With (left . (+ 0)) but maybe not with emacs -Q.  In the backtrace below
> note that it gets called from 'frame-notice-user-settings':

I was asking about X, but the backtrace you show is from Windows.  Am
I missing something?

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Sun, 22 Sep 2019 19:54:00 +0200
> 
>  > No, I don't think using x_get_arg is wrong, because we still want to
>  > determine whether to use CW_USEDEFAULT.
> 
> Hmm ...
> 
>  > We cannot do this unless f->size_hint_flags are set so as to tell
>  > w32_createwindow to use f->top_pos and/or f->left_pos.  Otherwise, we
>  > should put CW_USEDEFAULT in coords[].  IOW, how about the below?
> [...]
>  > +  if (!(f->size_hint_flags & USPosition || f->size_hint_flags & PPosition))
>  > +    {
>  > +      /* When called with RES_TYPE_NUMBER, and there's no 'top' or
>  > +	 'left' parameters in the frame's parameter alist,
>  > +	 gui_display_get_arg will return zero for anything that is
>  > +	 neither a number nor Qunbound.  If frame parameter alist does
>  > +	 have 'left' or 'top', they are interpreted by
>  > +	 gui_figure_window_size, which was already called, and which
>  > +	 sets f->size_hint_flags.  */
> 
> So you mean when size hints are not set, we are sure that
> gui_display_get_arg does not find anything in 'default-frame-alist'
> (ignoring, BTW 'initial-frame-alist') and finds a number here.  This
> looks a bit fragile to me.

If it's fragile, then we must take a look at gui_figure_window_size, I
think.  It should handle all those cases which you are afraid of.

> Isn't the fact that left/top are unbound sufficient that we should
> use CW_USEDEFAULT and f->left_pos/f->top_pos otherwise.

I don't know, but in any case we don't need more than one evidence;
additional evidence is just redundant.

I prefer using the hint flags as the indicator because that explicitly
tells us we can use f->top and f->left.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >>   >> I do hit it here.  Unless some sort of bug happens before.  For
 >>   >> example, when using (left . foo).
 >>   >
 >>   > But not with (left . (- 0)), right?
 >>
 >> With (left . (+ 0)) but maybe not with emacs -Q.  In the backtrace below
 >> note that it gets called from 'frame-notice-user-settings':
 >
 > I was asking about X, but the backtrace you show is from Windows.  Am
 > I missing something?

You asked

   . how come we don't hit this assertion when the same expression is
     in the init file, only in the early-init file?

and the dialogue continued as above.  So your question was clearly
about Windows because X cannot "hit this assertion".

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 > If it's fragile, then we must take a look at gui_figure_window_size, I
 > think.  It should handle all those cases which you are afraid of.

What bothers me more is that we base the Windows code on a concept
that it can neither understand nor control.

 > I prefer using the hint flags as the indicator because that explicitly
 > tells us we can use f->top and f->left.

But we do not use them in my_create_window with your patch.  We just
use left and top which are zero when a notation like (- 100) is used.

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 23 Sep 2019 09:32:00 +0200
> 
>  >>   >> I do hit it here.  Unless some sort of bug happens before.  For
>  >>   >> example, when using (left . foo).
>  >>   >
>  >>   > But not with (left . (- 0)), right?
>  >>
>  >> With (left . (+ 0)) but maybe not with emacs -Q.  In the backtrace below
>  >> note that it gets called from 'frame-notice-user-settings':
>  >
>  > I was asking about X, but the backtrace you show is from Windows.  Am
>  > I missing something?
> 
> You asked
> 
>    . how come we don't hit this assertion when the same expression is
>      in the init file, only in the early-init file?
> 
> and the dialogue continued as above.  So your question was clearly
> about Windows because X cannot "hit this assertion".

Sorry, I thought you were answering another of my questions.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 23 Sep 2019 09:32:23 +0200
> 
>  > If it's fragile, then we must take a look at gui_figure_window_size, I
>  > think.  It should handle all those cases which you are afraid of.
> 
> What bothers me more is that we base the Windows code on a concept
> that it can neither understand nor control.

Which concept is that?

>  > I prefer using the hint flags as the indicator because that explicitly
>  > tells us we can use f->top and f->left.
> 
> But we do not use them in my_create_window with your patch.

my_create_window just prepares the coordinates for w32_createwindow,
and the latter does use f->top_pos and f->left_pos when appropriate.

> We just use left and top which are zero when a notation like (- 100)
> is used.

When such a notation is used, gui_figure_window_size will have already
computed the coordinates in f->top_pos and f->left_pos, and set the
hint flags, before my_create_window is called.  Which is why we don't
need to do anything in my_create_window when the hint flags are set.

But if you will feel safer with the alternative patch below, I don't
mind:

diff --git a/src/w32fns.c b/src/w32fns.c
index 34abd02..4581015 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -5421,20 +5421,33 @@ my_create_window (struct frame * f)
   Lisp_Object left, top;
   struct w32_display_info *dpyinfo = &one_w32_display_info;
 
-  /* When called with RES_TYPE_NUMBER, gui_display_get_arg will return
-     zero for anything that is not a number and is not Qunbound.  */
-  left = gui_display_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
-                              RES_TYPE_NUMBER);
-  top = gui_display_get_arg (dpyinfo, Qnil, Qtop, "top", "Top",
-                             RES_TYPE_NUMBER);
-  if (EQ (left, Qunbound))
-    coords[0] = CW_USEDEFAULT;
-  else
-    coords[0] = XFIXNUM (left);
-  if (EQ (top, Qunbound))
-    coords[1] = CW_USEDEFAULT;
+  if (!(f->size_hint_flags & USPosition || f->size_hint_flags & PPosition))
+    {
+      /* When called with RES_TYPE_NUMBER, and there's no 'top' or
+	 'left' parameters in the frame's parameter alist,
+	 gui_display_get_arg will return zero for anything that is
+	 neither a number nor Qunbound.  If frame parameter alist does
+	 have 'left' or 'top', they are interpreted by
+	 gui_figure_window_size, which was already called, and which
+	 sets f->size_hint_flags.  */
+      left = gui_display_get_arg (dpyinfo, Qnil, Qleft, "left", "Left",
+				  RES_TYPE_NUMBER);
+      top = gui_display_get_arg (dpyinfo, Qnil, Qtop, "top", "Top",
+				 RES_TYPE_NUMBER);
+      if (EQ (left, Qunbound))
+	coords[0] = CW_USEDEFAULT;
+      else
+	coords[0] = XFIXNUM (left);
+      if (EQ (top, Qunbound))
+	coords[1] = CW_USEDEFAULT;
+      else
+	coords[1] = XFIXNUM (top);
+    }
   else
-    coords[1] = XFIXNUM (top);
+    {
+      coords[0] = f->left_pos;
+      coords[1] = f->top_pos;
+    }
 
   if (!PostThreadMessage (dwWindowsThreadId, WM_EMACS_CREATEWINDOW,
 			  (WPARAM)f, (LPARAM)coords))

The 'else' block is redundant, because when the hint flags are set,
w32_createwindow will disregard coords[].  But it does no harm, so if
you are more comfortable with it, fine.

Thanks.

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[martin rudalics]

 >> What bothers me more is that we base the Windows code on a concept
 >> that it can neither understand nor control.
 >
 > Which concept is that?

Size hints.  In particular the 'user-position' frame parameter.

 > my_create_window just prepares the coordinates for w32_createwindow,
 > and the latter does use f->top_pos and f->left_pos when appropriate.

OK.  I didn't remember that the code was that convoluted.

 > The 'else' block is redundant, because when the hint flags are set,
 > w32_createwindow will disregard coords[].  But it does no harm, so if
 > you are more comfortable with it, fine.

Thanks but don't bother.  Better leave a short note in a comment
explaining how this is supposed to behave.  On a related note: Do you
have any ideas what the window_prompting argument of w32_window is or
was for?

martin

bug#37415: Asserting failure setting frame parameters to non-fixnum values in early-init.el

[Eli Zaretskii]

> Cc: lekktu@gmail.com, 37415@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Tue, 24 Sep 2019 08:45:43 +0200
> 
>  >> What bothers me more is that we base the Windows code on a concept
>  >> that it can neither understand nor control.
>  >
>  > Which concept is that?
> 
> Size hints.  In particular the 'user-position' frame parameter.

Well, that has been working for far too long to be bothered now, I
think.

>  > The 'else' block is redundant, because when the hint flags are set,
>  > w32_createwindow will disregard coords[].  But it does no harm, so if
>  > you are more comfortable with it, fine.
> 
> Thanks but don't bother.  Better leave a short note in a comment
> explaining how this is supposed to behave.

Done.

> On a related note: Do you have any ideas what the window_prompting
> argument of w32_window is or was for?

It's unused, and was unused since the initial revision of that
function.  I think it's there just to keep the signature compatible
with that of x_window (which itself only keeps that compatibility in
toolkit versions).