A possible small tiny bug in win32 FULL_DEBUG build and some questions

A possible small tiny bug in win32 FULL_DEBUG build and some questions

[arthur miller]

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

Tiny bug: line 2447 in w32proc.c should probably read: cp->child_procs; not
cp-child_procs.

Possible tiny opt/simplification: SetConsoleCP & SetConsoleOutputCP
already do check for validity themselves (as far as I have tested with invalid
codepages), so it is possible to simplify those two:

DEFUN ("w32-set-console-codepage", Fw32_set_console_codepage,
       Sw32_set_console_codepage, 1, 1, 0,
       doc: /* Make Windows codepage CP be the codepage for Emacs tty keyboard input.
This codepage setting affects keyboard input in tty mode.
If successful, the new CP is returned, otherwise nil.  */)
  (Lisp_Object cp)
{
  CHECK_FIXNUM (cp);

  if (SetConsoleCP (XFIXNUM (cp)) != 0)
    return make_fixnum (GetConsoleCP ());
  return Qnil;
}

DEFUN ("w32-set-console-output-codepage", Fw32_set_console_output_codepage,
       Sw32_set_console_output_codepage, 1, 1, 0,
       doc: /* Make Windows codepage CP be the codepage for Emacs console output.
This codepage setting affects display in tty mode.
If successful, the new CP is returned, otherwise nil.  */)
  (Lisp_Object cp)
{
  CHECK_FIXNUM (cp);

  if (SetConsoleOutputCP (XFIXNUM (cp)) != 0)
    return make_fixnum (GetConsoleOutputCP ());
  return Qnil;
}

Msdn (or whatever they call it now) docs says it returns 0 if it does not
succeed. I have tested with few invalid numbers for CPs and those functions seem
to work fine for me. I am not expert on win32, perhaps there is something I miss
there? Nothing important, but if you want I can give you a patch.

Some questions:

Does GC-protected variables, those delcared as "staticpro", works the way I
think they work: GC never collects them?

If I enum all codepages with w32-get-valid-codepages, than the list will forever
stay alive? Even if the result from the function is never assigned to a variable
int the user code? I don't see any usage of this function, neither in C code nor
in Lisp (I grepped through); so I guess it is just eventually only called from
the external packages or just interactively by the user. Is it worth to copy
the returned list into a local list and return that so that the global list can
be freed so not to leak that list in the case the user does not want to save
that list anyway?

Another question: can list of valid codepages on the system change during the
runtime? Would it be OK to check if the list is alreay built, and just return
it, instead of setting the list to nil on each run and re-computing it? I guess
it is not something that changes often? I don't know really how it works on the
system level, so just a question.

Also, is it important that this list is in some particular order? Why reversing
it before returning it?

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

Re: A possible small tiny bug in win32 FULL_DEBUG build and some questions

[Eli Zaretskii]

> From: arthur miller <arthur.miller@live.com>
> Date: Thu, 2 Jan 2025 15:59:32 +0000
> 
> Tiny bug: line 2447 in w32proc.c should probably read: cp->child_procs; not
> cp-child_procs.

Why do you think so?  child_procs is not a member of 'struct
_child_process', as you can see in w32.h, so cp->child_procs will not
compile.  'cp-child_procs' yields the ordinal number of the child
process in the child_procs[] array, which is what that debugging
printf wants to produce.

> Possible tiny opt/simplification: SetConsoleCP & SetConsoleOutputCP
> already do check for validity themselves (as far as I have tested with invalid
> codepages), so it is possible to simplify those two:
> 
> DEFUN ("w32-set-console-codepage", Fw32_set_console_codepage,
>        Sw32_set_console_codepage, 1, 1, 0,
>        doc: /* Make Windows codepage CP be the codepage for Emacs tty keyboard input.
> This codepage setting affects keyboard input in tty mode.
> If successful, the new CP is returned, otherwise nil.  */)
>   (Lisp_Object cp)
> {
>   CHECK_FIXNUM (cp);
> 
>   if (SetConsoleCP (XFIXNUM (cp)) != 0)
>     return make_fixnum (GetConsoleCP ());
>   return Qnil;
> }
> 
> DEFUN ("w32-set-console-output-codepage", Fw32_set_console_output_codepage,
>        Sw32_set_console_output_codepage, 1, 1, 0,
>        doc: /* Make Windows codepage CP be the codepage for Emacs console output.
> This codepage setting affects display in tty mode.
> If successful, the new CP is returned, otherwise nil.  */)
>   (Lisp_Object cp)
> {
>   CHECK_FIXNUM (cp);
> 
>   if (SetConsoleOutputCP (XFIXNUM (cp)) != 0)
>     return make_fixnum (GetConsoleOutputCP ());
>   return Qnil;
> }
> 
> Msdn (or whatever they call it now) docs says it returns 0 if it does not
> succeed. I have tested with few invalid numbers for CPs and those functions seem
> to work fine for me. I am not expert on win32, perhaps there is something I miss
> there? Nothing important, but if you want I can give you a patch.

We generally prefer not to call Win32 APIs with invalid parameters, if
that could be avoided, because doing that could cause an abort.  So I
don't think this optimization is worth the risk.

> Does GC-protected variables, those delcared as "staticpro", works the way I
> think they work: GC never collects them?

Yes.

> If I enum all codepages with w32-get-valid-codepages, than the list will forever
> stay alive?

Yes.  But its value could change if you call the function again, in
which case the old value will be GC'ed at some point.

> Even if the result from the function is never assigned to a variable
> int the user code?

Yes, even so.

> I don't see any usage of this function, neither in C code nor
> in Lisp (I grepped through); so I guess it is just eventually only called from
> the external packages or just interactively by the user. Is it worth to copy
> the returned list into a local list and return that so that the global list can
> be freed so not to leak that list in the case the user does not want to save
> that list anyway?

I don't understand where did you see a leak, and what kind of a leak
is that, please elaborate.

> Another question: can list of valid codepages on the system change during the
> runtime?

I suspect yes, if the user installs additional codepages.  But I don't
really know.

> Would it be OK to check if the list is alreay built, and just return
> it, instead of setting the list to nil on each run and re-computing it? I guess
> it is not something that changes often? I don't know really how it works on the
> system level, so just a question.

I don't see why risk the possibility that the list changes behind our
back.  What important advantages will we gain?

> Also, is it important that this list is in some particular order? Why reversing
> it before returning it?

I guess we want to return the list in the order the OS enumerates
them, not in the reverse order.  Again, why is this important?  I
don't think this function is likely to be called inside tight loops,
is it?

Sv: A possible small tiny bug in win32 FULL_DEBUG build and some questions

[arthur miller]

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

>> Tiny bug: line 2447 in w32proc.c should probably read: cp->child_procs; not
>> cp-child_procs.
>
>Why do you think so?  child_procs is not a member of 'struct
>_child_process', as you can see in w32.h, so cp->child_procs will not
>compile.  'cp-child_procs' yields the ordinal number of the child
>process in the child_procs[] array, which is what that debugging
>printf wants to produce.

:-) Ah that was a minus :-). Sorry my bad; I should have looked into the
structure. I was looking through the file and the eye just  catched the
'kebak-case' name. I just assumed it was a possible typo, since I saw it
surroneded by those two idefs. Sorry for the alarm there.


>> Possible tiny opt/simplification: SetConsoleCP & SetConsoleOutputCP
>> already do check for validity themselves (as far as I have tested with invalid
>> codepages), so it is possible to simplify those two:
>>
>> DEFUN ("w32-set-console-codepage", Fw32_set_console_codepage,
>>        Sw32_set_console_codepage, 1, 1, 0,
>>        doc: /* Make Windows codepage CP be the codepage for Emacs tty keyboard input.
>> This codepage setting affects keyboard input in tty mode.
>> If successful, the new CP is returned, otherwise nil.  */)
>>   (Lisp_Object cp)
>> {
>>   CHECK_FIXNUM (cp);
>>
>>   if (SetConsoleCP (XFIXNUM (cp)) != 0)
>>     return make_fixnum (GetConsoleCP ());
>>   return Qnil;
>> }
>>
>> DEFUN ("w32-set-console-output-codepage", Fw32_set_console_output_codepage,
>>        Sw32_set_console_output_codepage, 1, 1, 0,
>>        doc: /* Make Windows codepage CP be the codepage for Emacs console output.
>> This codepage setting affects display in tty mode.
>> If successful, the new CP is returned, otherwise nil.  */)
>>   (Lisp_Object cp)
>> {
>>   CHECK_FIXNUM (cp);
>>
>>   if (SetConsoleOutputCP (XFIXNUM (cp)) != 0)
>>     return make_fixnum (GetConsoleOutputCP ());
>>   return Qnil;
>> }
>>
>> Msdn (or whatever they call it now) docs says it returns 0 if it does not
>> succeed. I have tested with few invalid numbers for CPs and those functions seem
>> to work fine for me. I am not expert on win32, perhaps there is something I miss
>> there? Nothing important, but if you want I can give you a patch.
>
>We generally prefer not to call Win32 APIs with invalid parameters, if
>that could be avoided, because doing that could cause an abort.  So I
>don't think this optimization is worth the risk.

I understand; that is the safe practice. In those particular two cases,
they do give guarantee to return zero if they fail; according to their
docs. I guess they are actually calling the same function themselves. I
did this little test with the patched functions just for the curiosity
and it didn't abort anything:

(let ((cp (w32-get-console-codepage))
      (cpo (w32-get-console-output-codepage)))
  (dotimes (i 100000)
    (w32-set-console-codepage i)
    (w32-set-console-output-codepage i)
    (w32-set-console-codepage (- i 10000))
    (w32-set-console-output-codepage (- i 10000)))
  (w32-set-console-codepage cp)
  (w32-set-console-output-codepage cpo))

Anyway, in the world of utf8 I guess nobody is using that code anyway,
so it does not really matter. Was just a little curiosa while I was
looking through the code for the reference.

>> Does GC-protected variables, those delcared as "staticpro", works the way I
>> think they work: GC never collects them?
>
>Yes.
>
>> If I enum all codepages with w32-get-valid-codepages, than the list will forever
>> stay alive?
>
>Yes.  But its value could change if you call the function again, in
>which case the old value will be GC'ed at some point.
>
>> Even if the result from the function is never assigned to a variable
>> int the user code?
>
>Yes, even so.

Thanks.

>> I don't see any usage of this function, neither in C code nor
>> in Lisp (I grepped through); so I guess it is just eventually only called from
>> the external packages or just interactively by the user. Is it worth to copy
>> the returned list into a local list and return that so that the global list can
>> be freed so not to leak that list in the case the user does not want to save
>> that list anyway?
>
>I don't understand where did you see a leak, and what kind of a leak
>is that, please elaborate.

I am assuming that this function can't be used very often. If the user
calls it for some reason (debugging, curiosity, checking if a codepage
is loaded?), the list will be constructed, but probably not used in a
single usage. But the space allocated for it will be allocated until
Emacs exists. If space is used but not needed than in a sense, it is a
memory leak.

>> Another question: can list of valid codepages on the system change during the
>> runtime?
>
>I suspect yes, if the user installs additional codepages.  But I don't
>really know.

Sounds reasonable; in that case the list should be recomputed every time.

>> Would it be OK to check if the list is alreay built, and just return
>> it, instead of setting the list to nil on each run and re-computing it? I guess
>> it is not something that changes often? I don't know really how it works on the
>> system level, so just a question.
>
>I don't see why risk the possibility that the list changes behind our
>back.  What important advantages will we gain?

Yes, per above, I was just unsure if that is something constant in the
system, or can chnage during the runtime.

>I guess we want to return the list in the order the OS enumerates

MS does not state any order of enumaration for this list, and the return
value does not seem to be sorted in some order, so any code that would
actually consult this list can't rely on any order and should probably
use functions like member or find; not elt or nth.

>them, not in the reverse order.  Again, why is this important?  I
>don't think this function is likely to be called inside tight loops,
>is it?

Of course not. Actually I don't think myself this function will be
called much at all. Codepages are more or less passé in the world of
utf8; so that is probably old cruft that is used on old systems, or is
that still needed?

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

Re: A possible small tiny bug in win32 FULL_DEBUG build and some questions

[Eli Zaretskii]

> From: arthur miller <arthur.miller@live.com>
> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Fri, 3 Jan 2025 00:24:58 +0000
> 
> (let ((cp (w32-get-console-codepage))
>       (cpo (w32-get-console-output-codepage)))
>   (dotimes (i 100000)
>     (w32-set-console-codepage i)
>     (w32-set-console-output-codepage i)
>     (w32-set-console-codepage (- i 10000))
>     (w32-set-console-output-codepage (- i 10000)))
>   (w32-set-console-codepage cp)
>   (w32-set-console-output-codepage cpo))
> 
> Anyway, in the world of utf8 I guess nobody is using that code anyway,
> so it does not really matter. Was just a little curiosa while I was
> looking through the code for the reference.

The codepages are still very much used in Emacs on Windows.  The
Windows UTF-8 support is rudimentary and marked "experimental" by MS.
So we still need to consider the codepages when referencing external
stuff.

> >> I don't see any usage of this function, neither in C code nor
> >> in Lisp (I grepped through); so I guess it is just eventually only called from
> >> the external packages or just interactively by the user. Is it worth to copy
> >> the returned list into a local list and return that so that the global list can
> >> be freed so not to leak that list in the case the user does not want to save
> >> that list anyway?
> >
> >I don't understand where did you see a leak, and what kind of a leak
> >is that, please elaborate.
> 
> I am assuming that this function can't be used very often. If the user
> calls it for some reason (debugging, curiosity, checking if a codepage
> is loaded?), the list will be constructed, but probably not used in a
> single usage. But the space allocated for it will be allocated until
> Emacs exists. If space is used but not needed than in a sense, it is a
> memory leak.

That's not a memory leak in my book, no.  A value that is computed
once and left to be used for the entire session is not a leak.

> >I guess we want to return the list in the order the OS enumerates
> 
> MS does not state any order of enumaration for this list, and the return
> value does not seem to be sorted in some order, so any code that would
> actually consult this list can't rely on any order and should probably
> use functions like member or find; not elt or nth.
> 
> >them, not in the reverse order.  Again, why is this important?  I
> >don't think this function is likely to be called inside tight loops,
> >is it?
> 
> Of course not. Actually I don't think myself this function will be
> called much at all. Codepages are more or less passé in the world of
> utf8; so that is probably old cruft that is used on old systems, or is
> that still needed?

Not yet passé, see above.

Sv: A possible small tiny bug in win32 FULL_DEBUG build and some questions

[arthur miller]

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

> > From: arthur miller <arthur.miller@live.com>
> > CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> > Date: Fri, 3 Jan 2025 00:24:58 +0000
> >
> > (let ((cp (w32-get-console-codepage))
> >       (cpo (w32-get-console-output-codepage)))
> >   (dotimes (i 100000)
> >     (w32-set-console-codepage i)
> >     (w32-set-console-output-codepage i)
> >     (w32-set-console-codepage (- i 10000))
> >     (w32-set-console-output-codepage (- i 10000)))
> >   (w32-set-console-codepage cp)
> >   (w32-set-console-output-codepage cpo))
> >
> > Anyway, in the world of utf8 I guess nobody is using that code anyway,
> > so it does not really matter. Was just a little curiosa while I was
> > looking through the code for the reference.
>
> The codepages are still very much used in Emacs on Windows.  The
> Windows UTF-8 support is rudimentary and marked "experimental" by MS.
> So we still need to consider the codepages when referencing external
> stuff.

Ok; thnks. I thought Emacs was doing its own coding/decoding, but I am
not yet familiar with that stuff. I guess I'll have more questions about
that stuff.

> > >> I don't see any usage of this function, neither in C code nor
> > >> in Lisp (I grepped through); so I guess it is just eventually only called from
> > >> the external packages or just interactively by the user. Is it worth to copy
> > >> the returned list into a local list and return that so that the global list can
> > >> be freed so not to leak that list in the case the user does not want to save
> > >> that list anyway?
> > >
> > >I don't understand where did you see a leak, and what kind of a leak
> > >is that, please elaborate.
> >
> > I am assuming that this function can't be used very often. If the user
> > calls it for some reason (debugging, curiosity, checking if a codepage
> > is loaded?), the list will be constructed, but probably not used in a
> > single usage. But the space allocated for it will be allocated until
> > Emacs exists. If space is used but not needed than in a sense, it is a
> > memory leak.
>
> That's not a memory leak in my book, no.  A value that is computed
> once and left to be used for the entire session is not a leak.

Mnjah; of course, I don't know why this particular function was
implemented, but my guess is debugging or just a simple check if a page
is loaded or something? I would guess the usage is rather temporary in a
single place or just occasionally. If some user could have a reason to
save it for the entire duration of an Emacs session they could save it
in a lisp variable themselves. As now, since there is a global
reference protected from GC, in the most probable case, the list will be
left alive for the life of the session, even if they don't use it.

Anyway, thanks for the explanations.

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