Re: master 2c8b09b06e7: Fix crash on Windows 9X

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Stefan Monnier]

> --- a/src/emacs.c
> +++ b/src/emacs.c
> @@ -1924,6 +1924,12 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem
>  	 Vcoding_system_hash_table.  */
>        syms_of_coding ();	/* This should be after syms_of_fileio.  */
>        init_frame_once ();       /* Before init_window_once.  */
> +      /* init_window_once calls make_initial_frame, which calls
> +	 Fcurrent_time and bset_display_time, both of which allocate
> +	 bignums.  Without the following call to init_bignums, crashes
> +	 happen on Windows 9X after dumping when GC tries to free a
> +	 pointer allocated on the system heap.  */
> +      init_bignum ();
>        init_window_once ();	/* Init the window system.  */
>  #ifdef HAVE_WINDOW_SYSTEM
>        init_fringe_once ();	/* Swap bitmaps if necessary.  */

I feel like I'm missing something: this adds a call to `init_bignum`
whereas I expected the patch to *move* the call.
Was this call simply missing?


        Stefan

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Po Lu]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> --- a/src/emacs.c
>> +++ b/src/emacs.c
>> @@ -1924,6 +1924,12 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem
>>  	 Vcoding_system_hash_table.  */
>>        syms_of_coding ();	/* This should be after syms_of_fileio.  */
>>        init_frame_once ();       /* Before init_window_once.  */
>> +      /* init_window_once calls make_initial_frame, which calls
>> +	 Fcurrent_time and bset_display_time, both of which allocate
>> +	 bignums.  Without the following call to init_bignums, crashes
>> +	 happen on Windows 9X after dumping when GC tries to free a
>> +	 pointer allocated on the system heap.  */
>> +      init_bignum ();
>>        init_window_once ();	/* Init the window system.  */
>>  #ifdef HAVE_WINDOW_SYSTEM
>>        init_fringe_once ();	/* Swap bitmaps if necessary.  */
>
> I feel like I'm missing something: this adds a call to `init_bignum`
> whereas I expected the patch to *move* the call.
> Was this call simply missing?
>
>
>         Stefan

No.  The call I added is only called before dumping, while the second is
called after dumping, AFAIK.

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Tue, 06 Dec 2022 12:56:46 +0800
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> --- a/src/emacs.c
> >> +++ b/src/emacs.c
> >> @@ -1924,6 +1924,12 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem
> >>  	 Vcoding_system_hash_table.  */
> >>        syms_of_coding ();	/* This should be after syms_of_fileio.  */
> >>        init_frame_once ();       /* Before init_window_once.  */
> >> +      /* init_window_once calls make_initial_frame, which calls
> >> +	 Fcurrent_time and bset_display_time, both of which allocate
> >> +	 bignums.  Without the following call to init_bignums, crashes
> >> +	 happen on Windows 9X after dumping when GC tries to free a
> >> +	 pointer allocated on the system heap.  */
> >> +      init_bignum ();
> >>        init_window_once ();	/* Init the window system.  */
> >>  #ifdef HAVE_WINDOW_SYSTEM
> >>        init_fringe_once ();	/* Swap bitmaps if necessary.  */
> >
> > I feel like I'm missing something: this adds a call to `init_bignum`
> > whereas I expected the patch to *move* the call.
> > Was this call simply missing?
> >
> >
> >         Stefan
> 
> No.  The call I added is only called before dumping, while the second is
> called after dumping, AFAIK.

No, the second one is done both before and after dumping.  Only the first
call is conditioned.

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org
>> Date: Tue, 06 Dec 2022 12:56:46 +0800
>> 
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> 
>> >> --- a/src/emacs.c
>> >> +++ b/src/emacs.c
>> >> @@ -1924,6 +1924,12 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem
>> >>  	 Vcoding_system_hash_table.  */
>> >>        syms_of_coding ();	/* This should be after syms_of_fileio.  */
>> >>        init_frame_once ();       /* Before init_window_once.  */
>> >> +      /* init_window_once calls make_initial_frame, which calls
>> >> +	 Fcurrent_time and bset_display_time, both of which allocate
>> >> +	 bignums.  Without the following call to init_bignums, crashes
>> >> +	 happen on Windows 9X after dumping when GC tries to free a
>> >> +	 pointer allocated on the system heap.  */
>> >> +      init_bignum ();
>> >>        init_window_once ();	/* Init the window system.  */
>> >>  #ifdef HAVE_WINDOW_SYSTEM
>> >>        init_fringe_once ();	/* Swap bitmaps if necessary.  */
>> >
>> > I feel like I'm missing something: this adds a call to `init_bignum`
>> > whereas I expected the patch to *move* the call.
>> > Was this call simply missing?
>> >
>> >
>> >         Stefan
>> 
>> No.  The call I added is only called before dumping, while the second is
>> called after dumping, AFAIK.
>
> No, the second one is done both before and after dumping.  Only the first
> call is conditioned.

Ah, thanks for the clarification.  In any case, it must come before
init_window_once.  Does calling it twice hurt?

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Tue, 06 Dec 2022 20:51:58 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> No.  The call I added is only called before dumping, while the second is
> >> called after dumping, AFAIK.
> >
> > No, the second one is done both before and after dumping.  Only the first
> > call is conditioned.
> 
> Ah, thanks for the clarification.  In any case, it must come before
> init_window_once.

init_window_once is called only if (!initialized), so this order is only
relevant for when dumping.

> Does calling it twice hurt?

It might, so I'd prefer not to risk such duplicate calls.  It should be easy
to make sure it is called only once when dumping for unexec, and only once
in the pdumper build (both when dumping and when not dumping).

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Tue, 06 Dec 2022 20:51:58 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> No.  The call I added is only called before dumping, while the second is
>> >> called after dumping, AFAIK.
>> >
>> > No, the second one is done both before and after dumping.  Only the first
>> > call is conditioned.
>> 
>> Ah, thanks for the clarification.  In any case, it must come before
>> init_window_once.
>
> init_window_once is called only if (!initialized), so this order is only
> relevant for when dumping.
>
>> Does calling it twice hurt?
>
> It might, so I'd prefer not to risk such duplicate calls.  It should be easy
> to make sure it is called only once when dumping for unexec, and only once
> in the pdumper build (both when dumping and when not dumping).

Something like this?

diff --git a/src/emacs.c b/src/emacs.c
index d8a2863fd9c..f0d20f8eb8c 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -1937,7 +1937,10 @@ main (int argc, char **argv)
     }
 
   init_alloc ();
-  init_bignum ();
+#ifndef HAVE_UNEXEC
+  if (!initialized)
+    init_bignum ();
+#endif
   init_threads ();
   init_eval ();
   running_asynch_code = 0;

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Wed, 07 Dec 2022 08:58:06 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It might, so I'd prefer not to risk such duplicate calls.  It should be easy
> > to make sure it is called only once when dumping for unexec, and only once
> > in the pdumper build (both when dumping and when not dumping).
> 
> Something like this?

Without the "if (!initialized)" part, since in the pdumper build we
want it invoked both when dumping and when starting after dumping.

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Wed, 07 Dec 2022 08:58:06 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > It might, so I'd prefer not to risk such duplicate calls.  It should be easy
>> > to make sure it is called only once when dumping for unexec, and only once
>> > in the pdumper build (both when dumping and when not dumping).
>> 
>> Something like this?
>
> Without the "if (!initialized)" part, since in the pdumper build we
> want it invoked both when dumping and when starting after dumping.

So, just like this?  Thanks; now comes the question of how to install
this on the release branch without causing conflicts with master, as
part of the change is already on master.

diff --git a/src/emacs.c b/src/emacs.c
index d8a2863fd9c..2d924da1cdb 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -1937,7 +1937,9 @@ main (int argc, char **argv)
     }
 
   init_alloc ();
+#ifndef HAVE_UNEXEC
   init_bignum ();
+#endif
   init_threads ();
   init_eval ();
   running_asynch_code = 0;

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Wed, 07 Dec 2022 21:13:51 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Without the "if (!initialized)" part, since in the pdumper build we
> > want it invoked both when dumping and when starting after dumping.
> 
> So, just like this?

Yes.

> Thanks; now comes the question of how to install this on the release
> branch without causing conflicts with master, as part of the change
> is already on master.

Cherry-pick the change you installed on master to emacs-29, and then
install this additional change on emacs-29.

Re: master 2c8b09b06e7: Fix crash on Windows 9X

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Cherry-pick the change you installed on master to emacs-29, and then
> install this additional change on emacs-29.

Ok, thanks.