GNU bug report logs -
#75521
scratch/igc: Delete unused macro DEFVAR_LISP_NOPROX
Previous Next
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 75521 in the body.
You can then email your comments to 75521 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
gerd <at> gnu.org, bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Sun, 12 Jan 2025 17:56:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Stefan Kangas <stefankangas <at> gmail.com>:
New bug report received and forwarded. Copy sent to
gerd <at> gnu.org, bug-gnu-emacs <at> gnu.org.
(Sun, 12 Jan 2025 17:56:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Severity: wishlist
Can we delete the unused macro DEFVAR_LISP_NOPROX?
We can always resurrect it again if it turns out that we need it.
[0001-Delete-unused-macro-DEFVAR_LISP_NOPROX.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Sun, 12 Jan 2025 18:08:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefankangas <at> gmail.com> writes:
> Severity: wishlist
>
> Can we delete the unused macro DEFVAR_LISP_NOPROX?
+1
Reply sent
to
Stefan Kangas <stefankangas <at> gmail.com>:
You have taken responsibility.
(Sun, 12 Jan 2025 18:20:05 GMT)
Full text and
rfc822 format available.
Notification sent
to
Stefan Kangas <stefankangas <at> gmail.com>:
bug acknowledged by developer.
(Sun, 12 Jan 2025 18:20:06 GMT)
Full text and
rfc822 format available.
Message #13 received at 75521-done <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Stefan Kangas <stefankangas <at> gmail.com> writes:
>
>> Severity: wishlist
>>
>> Can we delete the unused macro DEFVAR_LISP_NOPROX?
>
> +1
Thanks, done, closing.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Sun, 12 Jan 2025 22:37:02 GMT)
Full text and
rfc822 format available.
Message #16 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Stefan Kangas" <stefankangas <at> gmail.com> writes:
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>
>> Stefan Kangas <stefankangas <at> gmail.com> writes:
>>
>>> Severity: wishlist
>>>
>>> Can we delete the unused macro DEFVAR_LISP_NOPROX?
>>
>> +1
>
> Thanks, done, closing.
Thanks!
On master, this comment might need revising (staticpro now easserts that
the same variable isn't added twice):
/* Similar but define a variable whose value is the Lisp Object stored
at address. Two versions: with and without gc-marking of the C
variable. The nopro version is used when that variable will be
gc-marked for some other reason, since marking the same slot twice
can cause trouble with strings. */
void
defvar_lisp_nopro (struct Lisp_Fwd const *o_fwd, char const *namestring)
{
eassert (o_fwd->type == Lisp_Fwd_Obj);
Lisp_Object sym = intern_c_string (namestring);
XBARE_SYMBOL (sym)->u.s.declared_special = true;
XBARE_SYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED;
SET_SYMBOL_FWD (XBARE_SYMBOL (sym), o_fwd);
}
Also on master, the single user of DEFVAR_LISP_NOPRO is questionable:
font.c doesn't staticpro Vfont_weight_table because it appears in
font_style_table, but then font_style_table is sometimes modified so it
points to a new vector, and I don't see how Vfont_weight_table is
protected then.
GDB experiment seems to indicate it's not protected at all, but it's in
the dump so it isn't freed either. When --enable-checking is in use,
however, pdumper.c will abort the next time it sees an object that
wasn't marked during a previous GC run.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Sun, 12 Jan 2025 22:54:02 GMT)
Full text and
rfc822 format available.
Message #19 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Pip Cet <pipcet <at> protonmail.com> writes:
> "Stefan Kangas" <stefankangas <at> gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
>>
>>> Stefan Kangas <stefankangas <at> gmail.com> writes:
>>>
>>>> Severity: wishlist
>>>>
>>>> Can we delete the unused macro DEFVAR_LISP_NOPROX?
>>>
>>> +1
>>
>> Thanks, done, closing.
>
> Thanks!
>
> On master, this comment might need revising (staticpro now easserts that
> the same variable isn't added twice):
>
> /* Similar but define a variable whose value is the Lisp Object stored
> at address. Two versions: with and without gc-marking of the C
> variable. The nopro version is used when that variable will be
> gc-marked for some other reason, since marking the same slot twice
> can cause trouble with strings. */
> void
> defvar_lisp_nopro (struct Lisp_Fwd const *o_fwd, char const *namestring)
> {
> eassert (o_fwd->type == Lisp_Fwd_Obj);
> Lisp_Object sym = intern_c_string (namestring);
> XBARE_SYMBOL (sym)->u.s.declared_special = true;
> XBARE_SYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED;
> SET_SYMBOL_FWD (XBARE_SYMBOL (sym), o_fwd);
> }
>
> Also on master, the single user of DEFVAR_LISP_NOPRO is questionable:
> font.c doesn't staticpro Vfont_weight_table because it appears in
> font_style_table, but then font_style_table is sometimes modified so it
> points to a new vector, and I don't see how Vfont_weight_table is
> protected then.
>
> GDB experiment seems to indicate it's not protected at all, but it's in
> the dump so it isn't freed either. When --enable-checking is in use,
> however, pdumper.c will abort the next time it sees an object that
> wasn't marked during a previous GC run.
Patch:
From 4ba048bad6ff21ab95a5aeb2cbf33dc825890949 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Date: Sun, 12 Jan 2025 22:45:55 +0000
Subject: [PATCH] Remove DEFVAR_LISP_NOPRO
* src/font.c (syms_of_font): Use 'DEFVAR_LISP', not 'DEFVAR_LISP_NOPRO'.
* src/lisp.h (DEFVAR_LISP_NOPRO): Macro removed.
* src/lread.c (defvar_lisp_nopro): Function removed.
(defvar_lisp): Inline defvar_lisp_nopro here.
---
src/font.c | 6 +++---
src/lisp.h | 7 -------
src/lread.c | 13 ++-----------
3 files changed, 5 insertions(+), 21 deletions(-)
diff --git a/src/font.c b/src/font.c
index 86382267a4a..e6c41e41258 100644
--- a/src/font.c
+++ b/src/font.c
@@ -5954,7 +5954,7 @@ syms_of_font (void)
table used by the font display code. So we make them read-only,
to avoid this confusing situation. */
- DEFVAR_LISP_NOPRO ("font-weight-table", Vfont_weight_table,
+ DEFVAR_LISP ("font-weight-table", Vfont_weight_table,
doc: /* Vector of valid font weight values.
Each element has the form:
[NUMERIC-VALUE SYMBOLIC-NAME ALIAS-NAME ...]
@@ -5963,14 +5963,14 @@ syms_of_font (void)
Vfont_weight_table = BUILD_STYLE_TABLE (weight_table);
make_symbol_constant (intern_c_string ("font-weight-table"));
- DEFVAR_LISP_NOPRO ("font-slant-table", Vfont_slant_table,
+ DEFVAR_LISP ("font-slant-table", Vfont_slant_table,
doc: /* Vector of font slant symbols vs the corresponding numeric values.
See `font-weight-table' for the format of the vector.
This variable cannot be set; trying to do so will signal an error. */);
Vfont_slant_table = BUILD_STYLE_TABLE (slant_table);
make_symbol_constant (intern_c_string ("font-slant-table"));
- DEFVAR_LISP_NOPRO ("font-width-table", Vfont_width_table,
+ DEFVAR_LISP ("font-width-table", Vfont_width_table,
doc: /* Alist of font width symbols vs the corresponding numeric values.
See `font-weight-table' for the format of the vector.
This variable cannot be set; trying to do so will signal an error. */);
diff --git a/src/lisp.h b/src/lisp.h
index e3142f3b8cc..7646a3dd5f1 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3532,7 +3532,6 @@ call0 (Lisp_Object fn)
}
extern void defvar_lisp (struct Lisp_Objfwd const *, char const *);
-extern void defvar_lisp_nopro (struct Lisp_Objfwd const *, char const *);
extern void defvar_bool (struct Lisp_Boolfwd const *, char const *);
extern void defvar_int (struct Lisp_Intfwd const *, char const *);
extern void defvar_kboard (struct Lisp_Kboard_Objfwd const *, char const *);
@@ -3560,12 +3559,6 @@ #define DEFVAR_LISP(lname, vname, doc) \
= {Lisp_Fwd_Obj, &globals.f_##vname}; \
defvar_lisp (&o_fwd, lname); \
} while (false)
-#define DEFVAR_LISP_NOPRO(lname, vname, doc) \
- do { \
- static struct Lisp_Objfwd const o_fwd \
- = {Lisp_Fwd_Obj, &globals.f_##vname}; \
- defvar_lisp_nopro (&o_fwd, lname); \
- } while (false)
#define DEFVAR_BOOL(lname, vname, doc) \
do { \
static struct Lisp_Boolfwd const b_fwd \
diff --git a/src/lread.c b/src/lread.c
index ab900b3bee6..d6fa84bb826 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -5510,23 +5510,14 @@ defvar_bool (struct Lisp_Boolfwd const *b_fwd, char const *namestring)
}
/* Similar but define a variable whose value is the Lisp Object stored
- at address. Two versions: with and without gc-marking of the C
- variable. The nopro version is used when that variable will be
- gc-marked for some other reason, since marking the same slot twice
- can cause trouble with strings. */
+ at address. */
void
-defvar_lisp_nopro (struct Lisp_Objfwd const *o_fwd, char const *namestring)
+defvar_lisp (struct Lisp_Objfwd const *o_fwd, char const *namestring)
{
Lisp_Object sym = intern_c_string (namestring);
XBARE_SYMBOL (sym)->u.s.declared_special = true;
XBARE_SYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED;
SET_SYMBOL_FWD (XBARE_SYMBOL (sym), o_fwd);
-}
-
-void
-defvar_lisp (struct Lisp_Objfwd const *o_fwd, char const *namestring)
-{
- defvar_lisp_nopro (o_fwd, namestring);
staticpro (o_fwd->objvar);
}
--
2.47.1
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 12:17:02 GMT)
Full text and
rfc822 format available.
Message #22 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Cc: stefankangas <at> gmail.com
> Date: Sun, 12 Jan 2025 22:36:12 +0000
> From: Pip Cet via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>
> Also on master, the single user of DEFVAR_LISP_NOPRO is questionable:
> font.c doesn't staticpro Vfont_weight_table because it appears in
> font_style_table, but then font_style_table is sometimes modified so it
> points to a new vector, and I don't see how Vfont_weight_table is
> protected then.
According to this comment:
/* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
static Lisp_Object font_style_table;
Vfont_weight_table is actually an element of font_style_table, and
that one is protected:
DEFVAR_LISP_NOPRO ("font-width-table", Vfont_width_table,
doc: /* Alist of font width symbols vs the corresponding numeric values.
See `font-weight-table' for the format of the vector.
This variable cannot be set; trying to do so will signal an error. */);
Vfont_width_table = BUILD_STYLE_TABLE (width_table);
make_symbol_constant (intern_c_string ("font-width-table"));
staticpro (&font_style_table);
font_style_table = CALLN (Fvector, Vfont_weight_table, Vfont_slant_table,
Vfont_width_table);
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 12:58:02 GMT)
Full text and
rfc822 format available.
Message #25 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Cc: stefankangas <at> gmail.com
>> Date: Sun, 12 Jan 2025 22:36:12 +0000
>> From: Pip Cet via "Bug reports for GNU Emacs,
>> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>> Also on master, the single user of DEFVAR_LISP_NOPRO is questionable:
>> font.c doesn't staticpro Vfont_weight_table because it appears in
>> font_style_table, but then font_style_table is sometimes modified so it
>> points to a new vector, and I don't see how Vfont_weight_table is
>> protected then.
>
> According to this comment:
>
> /* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
> static Lisp_Object font_style_table;
>
> Vfont_weight_table is actually an element of font_style_table, and
> that one is protected:
But then font_style_table is sometimes modified so it points to
(i.e. contains as its element) a new vector, and I don't see how
Vfont_weight_table is protected then.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 13:31:01 GMT)
Full text and
rfc822 format available.
Message #28 received at submit <at> debbugs.gnu.org (full text, mbox):
Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs <at> gnu.org> writes:
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
>>> Cc: stefankangas <at> gmail.com
>>> Date: Sun, 12 Jan 2025 22:36:12 +0000
>>> From: Pip Cet via "Bug reports for GNU Emacs,
>>> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>>
>>> Also on master, the single user of DEFVAR_LISP_NOPRO is questionable:
>>> font.c doesn't staticpro Vfont_weight_table because it appears in
>>> font_style_table, but then font_style_table is sometimes modified so it
>>> points to a new vector, and I don't see how Vfont_weight_table is
>>> protected then.
>>
>> According to this comment:
>>
>> /* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
>> static Lisp_Object font_style_table;
>>
>> Vfont_weight_table is actually an element of font_style_table, and
>> that one is protected:
>
> But then font_style_table is sometimes modified so it points to
> (i.e. contains as its element) a new vector, and I don't see how
> Vfont_weight_table is protected then.
>
> Pip
BTW, this is _exactly_ how the old GCPRO macro was used. "We don't need
to GCPRO this local variable here because it's already protected by
something else". Horrible nightmare! That caused me such pain debugging
that I added conservative stack marking, split off the SDATA from
strings and so on. I'd hope Emacs overcomes this.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 13:31:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 13:48:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 12:57:03 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> > According to this comment:
> >
> > /* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
> > static Lisp_Object font_style_table;
> >
> > Vfont_weight_table is actually an element of font_style_table, and
> > that one is protected:
>
> But then font_style_table is sometimes modified so it points to
> (i.e. contains as its element) a new vector, and I don't see how
> Vfont_weight_table is protected then.
Modified where?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 14:17:02 GMT)
Full text and
rfc822 format available.
Message #37 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Mon, 13 Jan 2025 12:57:03 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> > According to this comment:
>> >
>> > /* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
>> > static Lisp_Object font_style_table;
>> >
>> > Vfont_weight_table is actually an element of font_style_table, and
>> > that one is protected:
>>
>> But then font_style_table is sometimes modified so it points to
>> (i.e. contains as its element) a new vector, and I don't see how
>> Vfont_weight_table is protected then.
>
> Modified where?
font_style_to_value:
if (! noerror)
return -1;
eassert (len < 255);
elt = make_vector (2, make_fixnum (100));
ASET (elt, 1, val);
ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
CALLN (Fvconcat, table, make_vector (1, elt)));
return (100 << 8) | (i << 4);
It's not clear to me when this code is reached (usually, noerror is
false and we don't modify font_style_table), but if it ever is reached,
we lose (at least when --enable-checking or in temacs).
The crash is easy to fix, but it's not at all clear to me that modifying
font_style_table shouldn't also modify Vfont_weight_table etc.
However, it is clear that the _NOPRO isn't required or helpful, and
since that's the last usage, let's just remove this remnant of GCPRO
times.
If, in addition, we want to keep Vfont_weight_table synchronized with
the table used by the font code to resolve weight symbols, we can do
this (this would change behavior in those cases that previously resulted
in a crash):
diff --git a/src/font.c b/src/font.c
index e6c41e41258..9b3d294640e 100644
--- a/src/font.c
+++ b/src/font.c
@@ -47,7 +47,22 @@ Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011
#define DEFAULT_ENCODING Qiso8859_1
/* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
-static Lisp_Object font_style_table;
+static Lisp_Object *font_style_table (int index)
+{
+ Lisp_Object *ret;
+ if (index == FONT_WEIGHT_INDEX)
+ ret = &Vfont_weight_table;
+ else if (index == FONT_SLANT_INDEX)
+ ret = &Vfont_slant_table;
+ else if (index == FONT_WIDTH_INDEX)
+ ret = &Vfont_width_table;
+ else
+ emacs_abort ();
+
+ CHECK_VECTOR (*ret);
+
+ return ret;
+}
/* Structure used for tables mapping weight, slant, and width numeric
values and their names. */
@@ -376,10 +391,9 @@ font_pixel_size (struct frame *f, Lisp_Object spec)
font_style_to_value (enum font_property_index prop, Lisp_Object val,
bool noerror)
{
- Lisp_Object table = AREF (font_style_table, prop - FONT_WEIGHT_INDEX);
+ Lisp_Object table = *font_style_table (prop);
int len;
- CHECK_VECTOR (table);
len = ASIZE (table);
if (SYMBOLP (val))
@@ -418,8 +432,8 @@ font_style_to_value (enum font_property_index prop, Lisp_Object val,
eassert (len < 255);
elt = make_vector (2, make_fixnum (100));
ASET (elt, 1, val);
- ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
- CALLN (Fvconcat, table, make_vector (1, elt)));
+ *font_style_table(prop) =
+ CALLN (Fvconcat, table, make_vector (1, elt));
return (100 << 8) | (i << 4);
}
else
@@ -461,8 +475,7 @@ font_style_symbolic (Lisp_Object font, enum font_property_index prop,
if (NILP (val))
return Qnil;
- table = AREF (font_style_table, prop - FONT_WEIGHT_INDEX);
- CHECK_VECTOR (table);
+ table = *font_style_table (prop);
i = XFIXNUM (val) & 0xFF;
eassert (((i >> 4) & 0xF) < ASIZE (table));
elt = AREF (table, ((i >> 4) & 0xF));
@@ -588,13 +601,12 @@ font_prop_validate_style (Lisp_Object style, Lisp_Object val)
if (FIXNUMP (val))
{
EMACS_INT n = XFIXNUM (val);
- CHECK_VECTOR (AREF (font_style_table, prop - FONT_WEIGHT_INDEX));
if (((n >> 4) & 0xF)
- >= ASIZE (AREF (font_style_table, prop - FONT_WEIGHT_INDEX)))
+ >= ASIZE (*font_style_table (prop)))
val = Qerror;
else
{
- Lisp_Object elt = AREF (AREF (font_style_table, prop - FONT_WEIGHT_INDEX), (n >> 4) & 0xF);
+ Lisp_Object elt = AREF (*font_style_table (prop), (n >> 4) & 0xF);
CHECK_VECTOR (elt);
if ((n & 0xF) + 1 >= ASIZE (elt))
@@ -5949,11 +5961,6 @@ syms_of_font (void)
gets the repertory information by an opened font and ENCODING. */);
Vfont_encoding_alist = Qnil;
- /* FIXME: These 3 vars are not quite what they appear: setq on them
- won't have any effect other than disconnect them from the style
- table used by the font display code. So we make them read-only,
- to avoid this confusing situation. */
-
DEFVAR_LISP ("font-weight-table", Vfont_weight_table,
doc: /* Vector of valid font weight values.
Each element has the form:
@@ -5961,25 +5968,18 @@ syms_of_font (void)
NUMERIC-VALUE is an integer, and SYMBOLIC-NAME and ALIAS-NAME are symbols.
This variable cannot be set; trying to do so will signal an error. */);
Vfont_weight_table = BUILD_STYLE_TABLE (weight_table);
- make_symbol_constant (intern_c_string ("font-weight-table"));
DEFVAR_LISP ("font-slant-table", Vfont_slant_table,
doc: /* Vector of font slant symbols vs the corresponding numeric values.
See `font-weight-table' for the format of the vector.
This variable cannot be set; trying to do so will signal an error. */);
Vfont_slant_table = BUILD_STYLE_TABLE (slant_table);
- make_symbol_constant (intern_c_string ("font-slant-table"));
DEFVAR_LISP ("font-width-table", Vfont_width_table,
doc: /* Alist of font width symbols vs the corresponding numeric values.
See `font-weight-table' for the format of the vector.
This variable cannot be set; trying to do so will signal an error. */);
Vfont_width_table = BUILD_STYLE_TABLE (width_table);
- make_symbol_constant (intern_c_string ("font-width-table"));
-
- staticpro (&font_style_table);
- font_style_table = CALLN (Fvector, Vfont_weight_table, Vfont_slant_table,
- Vfont_width_table);
DEFVAR_LISP ("font-log", Vfont_log, doc: /*
A list that logs font-related actions and results, for debugging.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 14:28:02 GMT)
Full text and
rfc822 format available.
Message #40 received at submit <at> debbugs.gnu.org (full text, mbox):
Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>>>> Cc: stefankangas <at> gmail.com
>>>> Date: Sun, 12 Jan 2025 22:36:12 +0000
>>>> From: Pip Cet via "Bug reports for GNU Emacs,
>>>> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>>>
>>>> Also on master, the single user of DEFVAR_LISP_NOPRO is questionable:
>>>> font.c doesn't staticpro Vfont_weight_table because it appears in
>>>> font_style_table, but then font_style_table is sometimes modified so it
>>>> points to a new vector, and I don't see how Vfont_weight_table is
>>>> protected then.
>>>
>>> According to this comment:
>>>
>>> /* Vector of Vfont_weight_table, Vfont_slant_table, and Vfont_width_table. */
>>> static Lisp_Object font_style_table;
>>>
>>> Vfont_weight_table is actually an element of font_style_table, and
>>> that one is protected:
>>
>> But then font_style_table is sometimes modified so it points to
>> (i.e. contains as its element) a new vector, and I don't see how
>> Vfont_weight_table is protected then.
>>
>> Pip
>
> BTW, this is _exactly_ how the old GCPRO macro was used. "We don't need
> to GCPRO this local variable here because it's already protected by
> something else".
Indeed. I played around with GC shortly before GCPRO was removed,
thinking (naively) that almost all Lisp_Objects that needed to survive
GC were GCPRO'd, and we would be able to use that for precise GC. But
GCPRO only ever protected the value of a Lisp_Object, never its address,
making it useless for other purposes.
> That caused me such pain debugging
> that I added conservative stack marking, split off the SDATA from
> strings and so on. I'd hope Emacs overcomes this.
I'm thinking of these changes as removing the last (hopefully) remnants
of GCPRO:
1. Remove DEFVAR_LISP_NOPRO
2. Conservatively mark SAFE_ALLOCA'd memory
3. Conservatively mark SDATA pointers so they remain valid
The problem with (3) is that pin_string becomes unnecessary for
correctness then, and it would be nice to remove it, but I suspect that
it is still necessary for performance: there are a lot of bytecode
objects, and while we might hope that most of them live in the "old"
sblocks that aren't copied, I don't think that's always true. (My
implementation reorders sdata a little but not by more than one sblock.
It'd be easy to change that not to reorder sdata at all, but then we'd
see fragmentation).
However, that still doesn't fix make_environment_block, which uses
xmalloc. So we'd have to xstrdup there; or make some xmalloc'd memory
conservatively scanned; or rewrite it to be a macro which calls
SAFE_ALLOCA.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 14:28:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 16:42:01 GMT)
Full text and
rfc822 format available.
Message #46 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 14:16:04 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> But then font_style_table is sometimes modified so it points to
> >> (i.e. contains as its element) a new vector, and I don't see how
> >> Vfont_weight_table is protected then.
> >
> > Modified where?
>
> font_style_to_value:
>
> if (! noerror)
> return -1;
> eassert (len < 255);
> elt = make_vector (2, make_fixnum (100));
> ASET (elt, 1, val);
> ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
> CALLN (Fvconcat, table, make_vector (1, elt)));
> return (100 << 8) | (i << 4);
OK, but the values gets plugged int font_style_table, right? And my
reading of mark_object_root_visitor is that it marks its argument
recursively, so any object reachable from font_style_table should also
be protected? Am I wrong?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 16:45:02 GMT)
Full text and
rfc822 format available.
Message #49 received at submit <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 14:27:01 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: "Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text editors\"" <bug-gnu-emacs <at> gnu.org>, Eli Zaretskii <eliz <at> gnu.org>, 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> I'm thinking of these changes as removing the last (hopefully) remnants
> of GCPRO:
>
> 1. Remove DEFVAR_LISP_NOPRO
> 2. Conservatively mark SAFE_ALLOCA'd memory
> 3. Conservatively mark SDATA pointers so they remain valid
Let's not waste our time on improving the old GC, except where we have
real bugs people actually bump into. Let's instead focus on making
the igc branch ready sooner.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 16:45:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 16:47:02 GMT)
Full text and
rfc822 format available.
Message #55 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Mon, 13 Jan 2025 14:16:04 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> But then font_style_table is sometimes modified so it points to
>> >> (i.e. contains as its element) a new vector, and I don't see how
>> >> Vfont_weight_table is protected then.
>> >
>> > Modified where?
>>
>> font_style_to_value:
>>
>> if (! noerror)
>> return -1;
>> eassert (len < 255);
>> elt = make_vector (2, make_fixnum (100));
>> ASET (elt, 1, val);
>> ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
>> CALLN (Fvconcat, table, make_vector (1, elt)));
>> return (100 << 8) | (i << 4);
>
> OK, but the values gets plugged int font_style_table, right? And my
> reading of mark_object_root_visitor is that it marks its argument
> recursively, so any object reachable from font_style_table should also
> be protected? Am I wrong?
No, you're not. Initially, Vfont_weight_table is protected because it's
reached recursively from font_style_table. If font_style_table is
modified no longer to contain Vfont_weight_table (by the code above),
Vfont_weight_table loses its protection. Accessing Vfont_weight_table
from Lisp when it's no longer protected will eventually cause a crash.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 17:17:02 GMT)
Full text and
rfc822 format available.
Message #58 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 16:46:39 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> Date: Mon, 13 Jan 2025 14:16:04 +0000
> >> From: Pip Cet <pipcet <at> protonmail.com>
> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> >>
> >> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> >>
> >> >> But then font_style_table is sometimes modified so it points to
> >> >> (i.e. contains as its element) a new vector, and I don't see how
> >> >> Vfont_weight_table is protected then.
> >> >
> >> > Modified where?
> >>
> >> font_style_to_value:
> >>
> >> if (! noerror)
> >> return -1;
> >> eassert (len < 255);
> >> elt = make_vector (2, make_fixnum (100));
> >> ASET (elt, 1, val);
> >> ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
> >> CALLN (Fvconcat, table, make_vector (1, elt)));
> >> return (100 << 8) | (i << 4);
> >
> > OK, but the values gets plugged int font_style_table, right? And my
> > reading of mark_object_root_visitor is that it marks its argument
> > recursively, so any object reachable from font_style_table should also
> > be protected? Am I wrong?
>
> No, you're not. Initially, Vfont_weight_table is protected because it's
> reached recursively from font_style_table. If font_style_table is
> modified no longer to contain Vfont_weight_table (by the code above),
> Vfont_weight_table loses its protection. Accessing Vfont_weight_table
> from Lisp when it's no longer protected will eventually cause a crash.
But the above code just uses ASET to put in one of font_style_table's
slots a Lisp vector. That vector is therefore reachable from
font_style_table (which is also a Lisp vector). No?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 17:20:02 GMT)
Full text and
rfc822 format available.
Message #61 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Mon, 13 Jan 2025 16:46:39 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> Date: Mon, 13 Jan 2025 14:16:04 +0000
>> >> From: Pip Cet <pipcet <at> protonmail.com>
>> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> >>
>> >> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>> >>
>> >> >> But then font_style_table is sometimes modified so it points to
>> >> >> (i.e. contains as its element) a new vector, and I don't see how
>> >> >> Vfont_weight_table is protected then.
>> >> >
>> >> > Modified where?
>> >>
>> >> font_style_to_value:
>> >>
>> >> if (! noerror)
>> >> return -1;
>> >> eassert (len < 255);
>> >> elt = make_vector (2, make_fixnum (100));
>> >> ASET (elt, 1, val);
>> >> ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
>> >> CALLN (Fvconcat, table, make_vector (1, elt)));
>> >> return (100 << 8) | (i << 4);
>> >
>> > OK, but the values gets plugged int font_style_table, right? And my
>> > reading of mark_object_root_visitor is that it marks its argument
>> > recursively, so any object reachable from font_style_table should also
>> > be protected? Am I wrong?
>>
>> No, you're not. Initially, Vfont_weight_table is protected because it's
>> reached recursively from font_style_table. If font_style_table is
>> modified no longer to contain Vfont_weight_table (by the code above),
>> Vfont_weight_table loses its protection. Accessing Vfont_weight_table
>> from Lisp when it's no longer protected will eventually cause a crash.
>
> But the above code just uses ASET to put in one of font_style_table's
> slots a Lisp vector. That vector is therefore reachable from
> font_style_table (which is also a Lisp vector). No?
But the old vector, which was in the slot before, is no longer reachable
after the ASET. That old vector can still be referenced from Lisp as
font-weight-table (or one of the other two). That means an object is
reachable from Lisp but not protected from GC.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 17:37:02 GMT)
Full text and
rfc822 format available.
Message #64 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 17:18:58 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> Date: Mon, 13 Jan 2025 16:46:39 +0000
> >> From: Pip Cet <pipcet <at> protonmail.com>
> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> >>
> >> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> >>
> >> >> Date: Mon, 13 Jan 2025 14:16:04 +0000
> >> >> From: Pip Cet <pipcet <at> protonmail.com>
> >> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> >> >>
> >> >> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> >> >>
> >> >> >> But then font_style_table is sometimes modified so it points to
> >> >> >> (i.e. contains as its element) a new vector, and I don't see how
> >> >> >> Vfont_weight_table is protected then.
> >> >> >
> >> >> > Modified where?
> >> >>
> >> >> font_style_to_value:
> >> >>
> >> >> if (! noerror)
> >> >> return -1;
> >> >> eassert (len < 255);
> >> >> elt = make_vector (2, make_fixnum (100));
> >> >> ASET (elt, 1, val);
> >> >> ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
> >> >> CALLN (Fvconcat, table, make_vector (1, elt)));
> >> >> return (100 << 8) | (i << 4);
> >> >
> >> > OK, but the values gets plugged int font_style_table, right? And my
> >> > reading of mark_object_root_visitor is that it marks its argument
> >> > recursively, so any object reachable from font_style_table should also
> >> > be protected? Am I wrong?
> >>
> >> No, you're not. Initially, Vfont_weight_table is protected because it's
> >> reached recursively from font_style_table. If font_style_table is
> >> modified no longer to contain Vfont_weight_table (by the code above),
> >> Vfont_weight_table loses its protection. Accessing Vfont_weight_table
> >> from Lisp when it's no longer protected will eventually cause a crash.
> >
> > But the above code just uses ASET to put in one of font_style_table's
> > slots a Lisp vector. That vector is therefore reachable from
> > font_style_table (which is also a Lisp vector). No?
>
> But the old vector, which was in the slot before, is no longer reachable
> after the ASET.
AFAIU, we copy the old vector to the new one when we call vconcat.
> That old vector can still be referenced from Lisp as
> font-weight-table (or one of the other two). That means an object is
> reachable from Lisp but not protected from GC.
I don't understand: if it's reachable from Lisp, it must be protected
by definition, no? GC can only free objects that are not reachable
from any other object.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 18:14:02 GMT)
Full text and
rfc822 format available.
Message #67 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Date: Mon, 13 Jan 2025 14:27:01 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: "Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text editors\"" <bug-gnu-emacs <at> gnu.org>, Eli Zaretskii <eliz <at> gnu.org>, 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>
>> I'm thinking of these changes as removing the last (hopefully) remnants
>> of GCPRO:
>>
>> 1. Remove DEFVAR_LISP_NOPRO
>> 2. Conservatively mark SAFE_ALLOCA'd memory
>> 3. Conservatively mark SDATA pointers so they remain valid
>
> Let's not waste our time on improving the old GC, except where we have
> real bugs people actually bump into. Let's instead focus on making
> the igc branch ready sooner.
I didn't look into 2-3, but at least 1 seems aligned with the goal of
merging the igc branch sooner. It's not a huge deal, but it's one less
thing to worry about.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 18:22:02 GMT)
Full text and
rfc822 format available.
Message #70 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Mon, 13 Jan 2025 17:18:58 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> Date: Mon, 13 Jan 2025 16:46:39 +0000
>> >> From: Pip Cet <pipcet <at> protonmail.com>
>> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> >>
>> >> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>> >>
>> >> >> Date: Mon, 13 Jan 2025 14:16:04 +0000
>> >> >> From: Pip Cet <pipcet <at> protonmail.com>
>> >> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> >> >>
>> >> >> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>> >> >>
>> >> >> >> But then font_style_table is sometimes modified so it points to
>> >> >> >> (i.e. contains as its element) a new vector, and I don't see how
>> >> >> >> Vfont_weight_table is protected then.
>> >> >> >
>> >> >> > Modified where?
>> >> >>
>> >> >> font_style_to_value:
>> >> >>
>> >> >> if (! noerror)
>> >> >> return -1;
>> >> >> eassert (len < 255);
>> >> >> elt = make_vector (2, make_fixnum (100));
>> >> >> ASET (elt, 1, val);
>> >> >> ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
>> >> >> CALLN (Fvconcat, table, make_vector (1, elt)));
>> >> >> return (100 << 8) | (i << 4);
>> >> >
>> >> > OK, but the values gets plugged int font_style_table, right? And my
>> >> > reading of mark_object_root_visitor is that it marks its argument
>> >> > recursively, so any object reachable from font_style_table should also
>> >> > be protected? Am I wrong?
>> >>
>> >> No, you're not. Initially, Vfont_weight_table is protected because it's
>> >> reached recursively from font_style_table. If font_style_table is
>> >> modified no longer to contain Vfont_weight_table (by the code above),
>> >> Vfont_weight_table loses its protection. Accessing Vfont_weight_table
>> >> from Lisp when it's no longer protected will eventually cause a crash.
>> >
>> > But the above code just uses ASET to put in one of font_style_table's
>> > slots a Lisp vector. That vector is therefore reachable from
>> > font_style_table (which is also a Lisp vector). No?
>>
>> But the old vector, which was in the slot before, is no longer reachable
>> after the ASET.
>
> AFAIU, we copy the old vector to the new one when we call vconcat.
Fvconcat creates a new vector, with some of its contents copied from the
old vector. It does not destroy the old vector or make it reachable
from the new vector. The old vector remains a valid Lisp object, and
it's still reachable from Lisp by evaluating font-weight-table, but it's
not marked during GC. (When using pdumper, it's not usually freed
because it's in the dump, and we never free dumped objects, but pdumper
also eassert()s that a dump object which was unreachable during a GC
cycle never becomes reachable again. If you build with checking
enabled, it's easy enough to get a crash if font_style_to_value ever
modifies font_style_table, but I don't know for which font backends it
ever does that by itself: doing it in GDB triggers the bug just as I
described, though).
>> That old vector can still be referenced from Lisp as
>> font-weight-table (or one of the other two). That means an object is
>> reachable from Lisp but not protected from GC.
>
> I don't understand: if it's reachable from Lisp, it must be protected
> by definition, no?
No, that's not correct. GC only protects global Lisp_Object variables
if they're staticpro'd. A non-staticpro'd Lisp_Object variable must be
protected in another way, or we have an unprotected reachable object.
> GC can only free objects that are not reachable from any other object.
No, that's not correct: the value of a forwarded symbol is not marked by
alloc.c GC.
case SYMBOL_FORWARDED:
/* If the value is forwarded to a buffer or keyboard field,
these are marked when we see the corresponding object.
And if it's forwarded to a C variable, either it's not
a Lisp_Object var, or it's staticpro'd already, or it's
reachable from font_style_table which is also
staticpro'd. */
break;
That comment is incorrect, because font_style_table may contain a
newly-consed vector, not the one that's still referred to as
font-weight-table.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 18:56:01 GMT)
Full text and
rfc822 format available.
Message #73 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 18:21:05 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> But the old vector, which was in the slot before, is no longer reachable
> >> after the ASET.
> >
> > AFAIU, we copy the old vector to the new one when we call vconcat.
>
> Fvconcat creates a new vector, with some of its contents copied from the
> old vector. It does not destroy the old vector or make it reachable
> from the new vector. The old vector remains a valid Lisp object, and
> it's still reachable from Lisp by evaluating font-weight-table, but it's
> not marked during GC. (When using pdumper, it's not usually freed
> because it's in the dump, and we never free dumped objects, but pdumper
> also eassert()s that a dump object which was unreachable during a GC
> cycle never becomes reachable again. If you build with checking
> enabled, it's easy enough to get a crash if font_style_to_value ever
> modifies font_style_table, but I don't know for which font backends it
> ever does that by itself: doing it in GDB triggers the bug just as I
> described, though).
I think there's a misunderstanding here. Can you please walk me
through the code below:
Lisp_Object table = AREF (font_style_table, prop - FONT_WEIGHT_INDEX);
...
elt = make_vector (2, make_fixnum (100));
ASET (elt, 1, val);
ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
CALLN (Fvconcat, table, make_vector (1, elt)));
and tell what is the "old vector" here that you are talking about?
And if that old vector is reachable from font_style_table, why do you
say it is not marked, if mark_object_root_visitor marks its argument
recursively?
> >> That old vector can still be referenced from Lisp as
> >> font-weight-table (or one of the other two). That means an object is
> >> reachable from Lisp but not protected from GC.
> >
> > I don't understand: if it's reachable from Lisp, it must be protected
> > by definition, no?
>
> No, that's not correct. GC only protects global Lisp_Object variables
> if they're staticpro'd. A non-staticpro'd Lisp_Object variable must be
> protected in another way, or we have an unprotected reachable object.
So you are saying that the problem that we don't assign the new value
(received from vconcat) to the appropriate variable
(Vfont_weight_table etc.)?
> > GC can only free objects that are not reachable from any other object.
>
> No, that's not correct: the value of a forwarded symbol is not marked by
> alloc.c GC.
>
> case SYMBOL_FORWARDED:
> /* If the value is forwarded to a buffer or keyboard field,
> these are marked when we see the corresponding object.
> And if it's forwarded to a C variable, either it's not
> a Lisp_Object var, or it's staticpro'd already, or it's
> reachable from font_style_table which is also
> staticpro'd. */
> break;
>
> That comment is incorrect, because font_style_table may contain a
> newly-consed vector, not the one that's still referred to as
> font-weight-table.
Sorry, you lost me here.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 19:09:02 GMT)
Full text and
rfc822 format available.
Message #76 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Pip Cet <pipcet <at> protonmail.com> writes:
> Patch:
Thanks! Your patch seems to make things both safer and simpler.
I'm curious why this was considered worth optimizing in the first place.
Is it just a remnant from the olden days when doing that type of thing
might have been more worthwhile? These days, I'd doubt that it would
make a measurable difference.
DEFVAR_LISP_NOPRO was introduced at least as early as 1991.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 19:17:02 GMT)
Full text and
rfc822 format available.
Message #79 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Mon, 13 Jan 2025 19:07:57 +0000
>
> Pip Cet <pipcet <at> protonmail.com> writes:
>
> > Patch:
>
> Thanks! Your patch seems to make things both safer and simpler.
We are still discussing what should be the minimal change in this case
and where exactly.
> I'm curious why this was considered worth optimizing in the first place.
What is the optimization here? DEFVAR_LISP_NOPRO or font_style_table?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 19:20:02 GMT)
Full text and
rfc822 format available.
Message #82 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Mon, 13 Jan 2025 18:21:05 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> But the old vector, which was in the slot before, is no longer reachable
>> >> after the ASET.
>> >
>> > AFAIU, we copy the old vector to the new one when we call vconcat.
>>
>> Fvconcat creates a new vector, with some of its contents copied from the
>> old vector. It does not destroy the old vector or make it reachable
>> from the new vector. The old vector remains a valid Lisp object, and
>> it's still reachable from Lisp by evaluating font-weight-table, but it's
>> not marked during GC. (When using pdumper, it's not usually freed
>> because it's in the dump, and we never free dumped objects, but pdumper
>> also eassert()s that a dump object which was unreachable during a GC
>> cycle never becomes reachable again. If you build with checking
>> enabled, it's easy enough to get a crash if font_style_to_value ever
>> modifies font_style_table, but I don't know for which font backends it
>> ever does that by itself: doing it in GDB triggers the bug just as I
>> described, though).
>
> I think there's a misunderstanding here. Can you please walk me
> through the code below:
>
> Lisp_Object table = AREF (font_style_table, prop - FONT_WEIGHT_INDEX);
At this point, table, AREF (font_style_table, 0), and Vfont_weight_table
all refer to the same Lisp object T.
> elt = make_vector (2, make_fixnum (100));
> ASET (elt, 1, val);
elt is a new Lisp object.
> ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
> CALLN (Fvconcat, table, make_vector (1, elt)));
1. make_vector creates a temporary one-element vector
2. vconcat creates a new Lisp object, T2
3. AREF (font_style_table, 0) is changed to refer to T2
Vfont_weight_table still refers to T.
> and tell what is the "old vector" here that you are talking about?
The old vector is T. It is no longer visible to GC once the function
terminates and table goes out of scope. It is still the value of
Vfont_weight_table.
> And if that old vector is reachable from font_style_table, why do you
> say it is not marked, if mark_object_root_visitor marks its argument
> recursively?
mark_object_root_visitor is never called for Vfont_weight_table because
it is not staticpro'd. It is called for font_style_table, but
font_style_table no longer refers to T; it refers to T2 only.
>> >> That old vector can still be referenced from Lisp as
>> >> font-weight-table (or one of the other two). That means an object is
>> >> reachable from Lisp but not protected from GC.
>> >
>> > I don't understand: if it's reachable from Lisp, it must be protected
>> > by definition, no?
>>
>> No, that's not correct. GC only protects global Lisp_Object variables
>> if they're staticpro'd. A non-staticpro'd Lisp_Object variable must be
>> protected in another way, or we have an unprotected reachable object.
>
> So you are saying that the problem that we don't assign the new value
> (received from vconcat) to the appropriate variable
> (Vfont_weight_table etc.)?
My first patch assumes that behavior was intentional, and retains it; my
second patch assumes it wasn't, and changes things so font_style_table
is no longer needed, because we just use Vfont_weight_table directly in
its place.
We should not attempt to keep two different memory locations
synchronized by trying to catch all modifications. That never works,
and there's no reason for this complication.
>> > GC can only free objects that are not reachable from any other object.
>>
>> No, that's not correct: the value of a forwarded symbol is not marked by
>> alloc.c GC.
>>
>> case SYMBOL_FORWARDED:
>> /* If the value is forwarded to a buffer or keyboard field,
>> these are marked when we see the corresponding object.
>> And if it's forwarded to a C variable, either it's not
>> a Lisp_Object var, or it's staticpro'd already, or it's
>> reachable from font_style_table which is also
>> staticpro'd. */
>> break;
>>
>> That comment is incorrect, because font_style_table may contain a
>> newly-consed vector, not the one that's still referred to as
>> font-weight-table.
>
> Sorry, you lost me here.
As I've tried to explain a number of times, the problem is that AREF
(font_style_table, 0) isn't necessarily equal to Vfont_weight_table.
The comment claims it is, but that's incorrect.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 19:36:01 GMT)
Full text and
rfc822 format available.
Message #85 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 19:18:59 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> > So you are saying that the problem that we don't assign the new value
> > (received from vconcat) to the appropriate variable
> > (Vfont_weight_table etc.)?
>
> My first patch assumes that behavior was intentional, and retains it; my
> second patch assumes it wasn't, and changes things so font_style_table
> is no longer needed, because we just use Vfont_weight_table directly in
> its place.
If we remove font_style_table, then all the code in font.c which uses
that table and accesses Vfont_weight_table etc. by indexing into
font_style_table, will stop working, no? So I think I'd prefer to
just assign the new value of the table, as returned by vconcat, to the
corresponding variable (Vfont_weight_table etc.), and leave the rest
of the code intact. That sounds like a much smaller change which only
touches a single place, where the problem is. Are there any problems
with this?
> We should not attempt to keep two different memory locations
> synchronized by trying to catch all modifications. That never works,
> and there's no reason for this complication.
I don't think I follow: which modifications are you talking about? If
that's Vfont_weight_table etc., then these 3 are documented to be
unmodifiable directly.
> >> > GC can only free objects that are not reachable from any other object.
> >>
> >> No, that's not correct: the value of a forwarded symbol is not marked by
> >> alloc.c GC.
> >>
> >> case SYMBOL_FORWARDED:
> >> /* If the value is forwarded to a buffer or keyboard field,
> >> these are marked when we see the corresponding object.
> >> And if it's forwarded to a C variable, either it's not
> >> a Lisp_Object var, or it's staticpro'd already, or it's
> >> reachable from font_style_table which is also
> >> staticpro'd. */
> >> break;
> >>
> >> That comment is incorrect, because font_style_table may contain a
> >> newly-consed vector, not the one that's still referred to as
> >> font-weight-table.
> >
> > Sorry, you lost me here.
>
> As I've tried to explain a number of times, the problem is that AREF
> (font_style_table, 0) isn't necessarily equal to Vfont_weight_table.
> The comment claims it is, but that's incorrect.
Ah, so it's a separate issue with that comment.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 19:38:02 GMT)
Full text and
rfc822 format available.
Message #88 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Stefan Kangas" <stefankangas <at> gmail.com> writes:
> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> Patch:
>
> Thanks! Your patch seems to make things both safer and simpler.
That's the first patch, right? If Eli wants to keep never-used Lisp
APIs stable and prefers a minimal change, we should do that, but we
should also fix the comment in alloc.c:
diff --git a/src/alloc.c b/src/alloc.c
index 8307c74c106..6a72e8aa087 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -7405,9 +7405,7 @@ #define CHECK_ALLOCATED_AND_LIVE_SYMBOL() ((void) 0)
/* If the value is forwarded to a buffer or keyboard field,
these are marked when we see the corresponding object.
And if it's forwarded to a C variable, either it's not
- a Lisp_Object var, or it's staticpro'd already, or it's
- reachable from font_style_table which is also
- staticpro'd. */
+ a Lisp_Object var or it's staticpro'd already. */
break;
default: emacs_abort ();
}
> I'm curious why this was considered worth optimizing in the first place.
So am I. It seems particularly strange to me that this was considered
an optimization at all: make-docfile.c could simply have moved all
global variables into a contiguous section, and then we wouldn't need
to staticpro them individually. Also irrelevant on today's machines,
of course.
(staticpro is a bit strange for another reason: are you supposed to call
it while the variable is uninitialized? If the initialization
expression calls Lisp, that might GC, and GC shouldn't see uninitialized
values. But if you call it after initializing the variable, you're
putting data into unprotected memory, which is also something that
should raise an alarm. Right now, both approaches are okay and in use,
but that's inconsistent. That's why I proposed changing staticpro to
have two arguments quite a while back.)
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 19:58:01 GMT)
Full text and
rfc822 format available.
Message #91 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Mon, 13 Jan 2025 19:18:59 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> > So you are saying that the problem that we don't assign the new value
>> > (received from vconcat) to the appropriate variable
>> > (Vfont_weight_table etc.)?
>>
>> My first patch assumes that behavior was intentional, and retains it; my
>> second patch assumes it wasn't, and changes things so font_style_table
>> is no longer needed, because we just use Vfont_weight_table directly in
>> its place.
>
> If we remove font_style_table, then all the code in font.c which uses
> that table and accesses Vfont_weight_table etc. by indexing into
> font_style_table, will stop working, no?
See my second patch, it's not a huge change.
> So I think I'd prefer to
> just assign the new value of the table, as returned by vconcat, to the
> corresponding variable (Vfont_weight_table etc.), and leave the rest
> of the code intact. That sounds like a much smaller change which only
> touches a single place, where the problem is. Are there any problems
> with this?
Yes, of course there are. It's a very fragile approach, as evidenced by
the fact that it was broken for so long, and there's absolutely no
benefit to it. The next person to attempt to change this code won't
understand why we use a non-staticpro'd variable which is kept in sync
with a staticpro'd one (because there is no reason to do so), and
probably break things again.
>> We should not attempt to keep two different memory locations
>> synchronized by trying to catch all modifications. That never works,
>> and there's no reason for this complication.
>
> I don't think I follow: which modifications are you talking about? If
> that's Vfont_weight_table etc., then these 3 are documented to be
> unmodifiable directly.
You mean the comment starting with "FIXME"? I thought it'd be good to
fix that. (It's also inaccurate, because the problematic workaround it
describes does not, actually, work).
The vectors, of course, can be modified, and that will have very strange
and unexpected results.
>> >> > GC can only free objects that are not reachable from any other object.
>> >>
>> >> No, that's not correct: the value of a forwarded symbol is not marked by
>> >> alloc.c GC.
>> >>
>> >> case SYMBOL_FORWARDED:
>> >> /* If the value is forwarded to a buffer or keyboard field,
>> >> these are marked when we see the corresponding object.
>> >> And if it's forwarded to a C variable, either it's not
>> >> a Lisp_Object var, or it's staticpro'd already, or it's
>> >> reachable from font_style_table which is also
>> >> staticpro'd. */
>> >> break;
>> >>
>> >> That comment is incorrect, because font_style_table may contain a
>> >> newly-consed vector, not the one that's still referred to as
>> >> font-weight-table.
>> >
>> > Sorry, you lost me here.
>>
>> As I've tried to explain a number of times, the problem is that AREF
>> (font_style_table, 0) isn't necessarily equal to Vfont_weight_table.
>> The comment claims it is, but that's incorrect.
>
> Ah, so it's a separate issue with that comment.
No, it's not! The comment is about the very same issue we've been
"discussing" for so long.
We tried avoiding a staticpro, it failed to work, the right thing to do
is to add the staticpro rather than complicating a fragile workaround
further so it will work for a brief period until the next change breaks
it again.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 20:02:02 GMT)
Full text and
rfc822 format available.
Message #94 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> I'm curious why this was considered worth optimizing in the first place.
>
> What is the optimization here? DEFVAR_LISP_NOPRO or font_style_table?
I was thinking of DEFVAR_LISP_NOPRO:
https://lists.gnu.org/r/emacs-devel/2024-05/msg00896.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 20:08:02 GMT)
Full text and
rfc822 format available.
Message #97 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Mon, 13 Jan 2025 19:57:17 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> We should not attempt to keep two different memory locations
> >> synchronized by trying to catch all modifications. That never works,
> >> and there's no reason for this complication.
> >
> > I don't think I follow: which modifications are you talking about? If
> > that's Vfont_weight_table etc., then these 3 are documented to be
> > unmodifiable directly.
>
> You mean the comment starting with "FIXME"?
No, I mean this part of the doc string:
This variable cannot be set; trying to do so will signal an error.
What two different memory locations did you have in mind above?
> >> As I've tried to explain a number of times, the problem is that AREF
> >> (font_style_table, 0) isn't necessarily equal to Vfont_weight_table.
> >> The comment claims it is, but that's incorrect.
> >
> > Ah, so it's a separate issue with that comment.
>
> No, it's not! The comment is about the very same issue we've been
> "discussing" for so long.
The main issue is the code, not the comment. So fixing the comment is
a separate issue.
Anyway, please don't install anything yet. I want to take a closer
look at this code and its usage, and I've ran out of time for today.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 20:12:02 GMT)
Full text and
rfc822 format available.
Message #100 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Mon, 13 Jan 2025 20:01:24 +0000
> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> I'm curious why this was considered worth optimizing in the first place.
> >
> > What is the optimization here? DEFVAR_LISP_NOPRO or font_style_table?
>
> I was thinking of DEFVAR_LISP_NOPRO:
AFAIU, it's there to avoid protecting something that is already
protected. staticvec[] is a static vector, so wasting slots there is
best avoided.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 20:39:02 GMT)
Full text and
rfc822 format available.
Message #103 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
> Anyway, please don't install anything yet. I want to take a closer
> look at this code and its usage, and I've ran out of time for today.
Okay. It's probably best to take a break from this issue, anyway.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Mon, 13 Jan 2025 21:10:02 GMT)
Full text and
rfc822 format available.
Message #106 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Stefan Kangas <stefankangas <at> gmail.com>
>> Date: Mon, 13 Jan 2025 20:01:24 +0000
>> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>> >> I'm curious why this was considered worth optimizing in the first place.
>> >
>> > What is the optimization here? DEFVAR_LISP_NOPRO or font_style_table?
>>
>> I was thinking of DEFVAR_LISP_NOPRO:
>
> AFAIU, it's there to avoid protecting something that is already
> protected. staticvec[] is a static vector, so wasting slots there is
> best avoided.
Yes, indeed, but that seems like an optimization that is not worth
doing. Not these days, at any rate.
It complicates the code, leading to confusion (case in point: this long
thread where many cycles were spent discussing it), and the performance
gains are not likely to even be measurable.
Note that with MPS, DEFVAR_LISP and DEFVAR_LISP_NOPRO are equivalent.
So by getting rid of DEFVAR_LISP_NOPRO, we would reduce the delta
between igc and master as a nice bonus.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 03:31:02 GMT)
Full text and
rfc822 format available.
Message #109 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Mon, 13 Jan 2025 21:09:02 +0000
> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> From: Stefan Kangas <stefankangas <at> gmail.com>
> >> Date: Mon, 13 Jan 2025 20:01:24 +0000
> >> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
> >>
> >> Eli Zaretskii <eliz <at> gnu.org> writes:
> >>
> >> >> I'm curious why this was considered worth optimizing in the first place.
> >> >
> >> > What is the optimization here? DEFVAR_LISP_NOPRO or font_style_table?
> >>
> >> I was thinking of DEFVAR_LISP_NOPRO:
> >
> > AFAIU, it's there to avoid protecting something that is already
> > protected. staticvec[] is a static vector, so wasting slots there is
> > best avoided.
>
> Yes, indeed, but that seems like an optimization that is not worth
> doing. Not these days, at any rate.
If you try to staticpro too many variables, the build will fail
because Emacs runs out of space in staticvec. Not everyone knows what
to do in this case, and it's an annoyance when this happens. So I
would not say so easily that it's an optimization not worth doing, no.
> It complicates the code, leading to confusion (case in point: this long
> thread where many cycles were spent discussing it), and the performance
> gains are not likely to even be measurable.
That's normal in Emacs. Some stuff is complex, and given enough
comments can be dealt with without too many problems.
And this thread is in a way a self-inflicted shoot-in-the-foot
problem: we could have left the code alone, as it works for us for
decades without any visible problems.
> Note that with MPS, DEFVAR_LISP and DEFVAR_LISP_NOPRO are equivalent.
When the igc branch lands, this will be a non-issue, yes. One more
reason not to waste too much effort on this code now. But since the
genie is out of the bottle, we must.
> So by getting rid of DEFVAR_LISP_NOPRO, we would reduce the delta
> between igc and master as a nice bonus.
Why do we need to reduce the delta? what does that get us?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 05:08:01 GMT)
Full text and
rfc822 format available.
Message #112 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefankangas <at> gmail.com> writes:
>> AFAIU, it's there to avoid protecting something that is already
>> protected. staticvec[] is a static vector, so wasting slots there is
>> best avoided.
>
> Yes, indeed, but that seems like an optimization that is not worth
> doing. Not these days, at any rate.
>
> It complicates the code, leading to confusion (case in point: this long
> thread where many cycles were spent discussing it), and the performance
> gains are not likely to even be measurable.
+1000
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 10:58:02 GMT)
Full text and
rfc822 format available.
Message #115 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> From: Stefan Kangas <stefankangas <at> gmail.com>
>> Date: Mon, 13 Jan 2025 21:09:02 +0000
>> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>> >> From: Stefan Kangas <stefankangas <at> gmail.com>
>> >> Date: Mon, 13 Jan 2025 20:01:24 +0000
>> >> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>> >>
>> >> Eli Zaretskii <eliz <at> gnu.org> writes:
>> >>
>> >> >> I'm curious why this was considered worth optimizing in the first place.
>> >> >
>> >> > What is the optimization here? DEFVAR_LISP_NOPRO or font_style_table?
>> >>
>> >> I was thinking of DEFVAR_LISP_NOPRO:
>> >
>> > AFAIU, it's there to avoid protecting something that is already
>> > protected. staticvec[] is a static vector, so wasting slots there is
>> > best avoided.
>>
>> Yes, indeed, but that seems like an optimization that is not worth
>> doing. Not these days, at any rate.
>
> If you try to staticpro too many variables, the build will fail
> because Emacs runs out of space in staticvec. Not everyone knows what
> to do in this case, and it's an annoyance when this happens. So I
> would not say so easily that it's an optimization not worth doing, no.
Running out of NSTATICS is not and never has been a significant problem.
If you want to change the error message (which seems quite clear, to
me), feel free to, but saying that we shouldn't staticpro variables that
need to be staticpro'd because we might have to bump NSTATICS is too
ridiculous an argument to entertain.
(On this system, NSTATICS is 2048, but there are only 304 explicit
staticpro calls in the Emacs code base, plus 636 DEFVAR_LISP* calls.
We're very, very far from the point where we need to consider bumping
it.)
>> It complicates the code, leading to confusion (case in point: this long
>> thread where many cycles were spent discussing it), and the performance
>> gains are not likely to even be measurable.
>
> That's normal in Emacs. Some stuff is complex, and given enough
> comments can be dealt with without too many problems.
>
> And this thread is in a way a self-inflicted shoot-in-the-foot
> problem: we could have left the code alone, as it works for us for
> decades without any visible problems.
Keeping unnecessary and broken complexity because "it works for us" (it
doesn't work for anyone using the API; I assume people tried, failed,
and gave up on Emacs) is not the right thing to do, no.
What went wrong here is quite different: it's a simple problem, it was
discovered and fixed, it took quite a while to get you to understand it,
and then you immediately went into "oh, right, it's a bug, but let's not
fix it" mode.
It's a bug, there's a fix, it simplifies things significantly (lines
starting with "DEF" are magic to make-docfile.c, so we should try to
reduce the number of cases that imperfect parser has to handle).
Describing someone's effort to improve Emacs as "shoot-in-the-foot" is
quite offensive, BTW.
>> Note that with MPS, DEFVAR_LISP and DEFVAR_LISP_NOPRO are equivalent.
>
> When the igc branch lands, this will be a non-issue, yes. One more
^^^^
That's still an "if", IMHO.
> reason not to waste too much effort on this code now. But since the
> genie is out of the bottle, we must.
Saying that improving the Emacs code base is a waste of effort only
makes sense if you think Emacs development will stop soon. Even then,
it's also quite offensive.
>> So by getting rid of DEFVAR_LISP_NOPRO, we would reduce the delta
>> between igc and master as a nice bonus.
>
> Why do we need to reduce the delta? what does that get us?
It turns the "if" into more of a "when".
I'll stop here. Yet another simple improvement (and bug fix) to Emacs
which was effectively vetoed.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 14:17:01 GMT)
Full text and
rfc822 format available.
Message #118 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 14 Jan 2025 10:56:55 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 75521 <at> debbugs.gnu.org
>
> I'll stop here. Yet another simple improvement (and bug fix) to Emacs
> which was effectively vetoed.
I have nothing useful to respond to these accusations, except to
reiterate my firm belief that we should prefer investing our energy
and resources in adding new infrastructure and/or replacing old one
with newer and better one, to investing them in cleaning up old
infrastructure. The old infrastructure works and works well; it had
worked for us for decades. Cleaning it up brings only very small
gains, if at all, while running non-negligible risks of introducing
bugs.
So replacing the current GC with a better, more performant and
sophisticated one, is okay and very welcome; cleaning up the old GC is
much less so (especially when we already have plans for replacing it).
Let's be wise in where we decide to invest our resources.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 16:00:02 GMT)
Full text and
rfc822 format available.
Message #121 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> Date: Mon, 13 Jan 2025 22:07:21 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
>
>
> Anyway, please don't install anything yet. I want to take a closer
> look at this code and its usage, and I've ran out of time for today.
WDYT about the patch below? It's supposed to keep the 3 variables in
sync with the corresponding slots in font_style_table, if the slots
are ever modified.
diff --git a/src/font.c b/src/font.c
index 8638226..77fc74b 100644
--- a/src/font.c
+++ b/src/font.c
@@ -418,8 +418,23 @@ font_style_to_value (enum font_property_index prop, Lisp_Object val,
eassert (len < 255);
elt = make_vector (2, make_fixnum (100));
ASET (elt, 1, val);
- ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
- CALLN (Fvconcat, table, make_vector (1, elt)));
+ Lisp_Object new_table = CALLN (Fvconcat, table, make_vector (1, elt));
+ /* Update the corresponding variable with the new value. */
+ switch (prop)
+ {
+ case FONT_WEIGHT_INDEX:
+ Vfont_weight_table = new_table;
+ break;
+ case FONT_SLANT_INDEX:
+ Vfont_slant_table = new_table;
+ break;
+ case FONT_WIDTH_INDEX:
+ Vfont_width_table = new_table;
+ break;
+ default:
+ break;
+ }
+ ASET (font_style_table, prop - FONT_WEIGHT_INDEX, new_table);
return (100 << 8) | (i << 4);
}
else
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 17:03:02 GMT)
Full text and
rfc822 format available.
Message #124 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> Date: Mon, 13 Jan 2025 22:07:21 +0200
>> From: Eli Zaretskii <eliz <at> gnu.org>
>>
>>
>> Anyway, please don't install anything yet. I want to take a closer
>> look at this code and its usage, and I've ran out of time for today.
>
> WDYT about the patch below? It's supposed to keep the 3 variables in
> sync with the corresponding slots in font_style_table, if the slots
> are ever modified.
If you meant to ask whether the patch will momentarily render this
specific bug harmless, I believe it will (but expose other bugs which
will still cause crashes).
However, I don't like it one bit, and reviewing it finds many other
things that seem like bugs here.
I've already answered the question of what I think about this approach:
"It's a very fragile approach, as evidenced by the fact that it was
broken for so long, and there's absolutely no benefit to it. The next
person to attempt to change this code won't understand why we use a
non-staticpro'd variable which is kept in sync with a staticpro'd one
(because there is no reason to do so), and probably break things again."
Note that it's not just in theory that it's fragile: I have experimented
with changes to the nativecomp code which would have broken it, because
make_symbol_constant shouldn't just trap writes, it should also allow
the nativecomp code to generate better code.
It also changes behavior, of course, and the new behavior doesn't make
much sense: with or without --enable-checking, it's trivial to cause
further crashes by modifying the Vfont_weight_table variable's value,
for example.
I think we should fix this code not to crash with or without
--enable-checking. It may be best to simply remove the Lisp variables
entirely.
> diff --git a/src/font.c b/src/font.c
> index 8638226..77fc74b 100644
> --- a/src/font.c
> +++ b/src/font.c
> @@ -418,8 +418,23 @@ font_style_to_value (enum font_property_index prop, Lisp_Object val,
> eassert (len < 255);
I don't see why this eassert makes sense. If we ever reach this code,
why do we assume we can't reach it 256 times? If we reach it 256 times
(or thereabouts), why crash Emacs? If we want to crash Emacs in this
case, why only crash it if --enable-checking has been specified?
font.c needs careful analysis, because there are too many apparent bugs
in it right now (it also assumes that Vfont_encoding_alist isn't
circular, for example). "Minimal" changes are not the right thing to do
here.
> elt = make_vector (2, make_fixnum (100));
This is also strange: 100 is the "medium" weight; we should be using 80,
which is the "normal" one. 100 is the correct default slant and width,
though.
> ASET (elt, 1, val);
> - ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
> - CALLN (Fvconcat, table, make_vector (1, elt)));
> + Lisp_Object new_table = CALLN (Fvconcat, table, make_vector (1, elt));
This is also strange: the tables are documented to be sorted
numerically, but I don't see any code that relies on that. vconcat
certainly doesn't leave the tables sorted.
I do see code, e.g. font_style_symbolic, which assumes that (i>>4) is
the index corresponding to value i; this is not currently true, and
font_style_symbolic will either eassert or fail to find the right entry.
In the absence of checking, it will most likely cause crashes...
> + /* Update the corresponding variable with the new value. */
> + switch (prop)
> + {
> + case FONT_WEIGHT_INDEX:
> + Vfont_weight_table = new_table;
> + break;
> + case FONT_SLANT_INDEX:
> + Vfont_slant_table = new_table;
> + break;
> + case FONT_WIDTH_INDEX:
> + Vfont_width_table = new_table;
> + break;
> + default:
> + break;
"eassume (0);" is the right thing to do here, IMHO. If the default
label isn't optimized out, it's redundant code. If we ever reach the
default label, something else is severely wrong, so the
eassume-as-eassert thing would save us.
> + }
> + ASET (font_style_table, prop - FONT_WEIGHT_INDEX, new_table);
> return (100 << 8) | (i << 4);
I need to check what this return value is used for: if 0x64f0 is the
right return value for i == 0xf, why is it 0x6500 for i == 0x10?
So, sorry, further fixes required, a minimal fix won't work.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 21:10:02 GMT)
Full text and
rfc822 format available.
Message #127 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 14 Jan 2025 17:02:41 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: stefankangas <at> gmail.com, 75521 <at> debbugs.gnu.org
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> >> Date: Mon, 13 Jan 2025 22:07:21 +0200
> >> From: Eli Zaretskii <eliz <at> gnu.org>
> >>
> >>
> >> Anyway, please don't install anything yet. I want to take a closer
> >> look at this code and its usage, and I've ran out of time for today.
> >
> > WDYT about the patch below? It's supposed to keep the 3 variables in
> > sync with the corresponding slots in font_style_table, if the slots
> > are ever modified.
>
> If you meant to ask whether the patch will momentarily render this
> specific bug harmless, I believe it will (but expose other bugs which
> will still cause crashes).
I was asking about the problem where that code makes the values of
Vfont_weight_table and its 2 brethren vulnerable to being GC'ed.
That's the only problem I tried to solve with that patch. That
Vfont_weight_table etc. are also updated to reflect the changes in
font_style_table is a nice benefit.
If you agree that this problem will be solved by the patch, then I'd
like to install it, because it's very simple and localized, so the
risk to break something is IMO very low.
> I've already answered the question of what I think about this approach:
> "It's a very fragile approach, as evidenced by the fact that it was
> broken for so long, and there's absolutely no benefit to it.
I think this wasn't reported because that code is seldom if ever
executed. I couldn't find a way to enter it. But I see no reason to
remove it at this point.
> The next
> person to attempt to change this code won't understand why we use a
> non-staticpro'd variable which is kept in sync with a staticpro'd one
> (because there is no reason to do so), and probably break things again."
That can be alleviated a bit by adding commentary which explains this
subtlety.
> It also changes behavior, of course, and the new behavior doesn't make
> much sense: with or without --enable-checking, it's trivial to cause
> further crashes by modifying the Vfont_weight_table variable's value,
> for example.
Vfont_weight_table etc. are supposed not to be modifiable (it should
cause Emacs to signal an error). If you can show a recipe for causing
a crash by modifying them, I will certainly see why we don't behave as
the doc string says.
In any case, this is a separate issue.
> I think we should fix this code not to crash with or without
> --enable-checking.
Crashes should be avoided, I agree.
> It may be best to simply remove the Lisp variables entirely.
I don't want to remove them because they are used, albeit in a small
number of places.
> > diff --git a/src/font.c b/src/font.c
> > index 8638226..77fc74b 100644
> > --- a/src/font.c
> > +++ b/src/font.c
> > @@ -418,8 +418,23 @@ font_style_to_value (enum font_property_index prop, Lisp_Object val,
> > eassert (len < 255);
>
> I don't see why this eassert makes sense. If we ever reach this code,
> why do we assume we can't reach it 256 times?
The code as implemented cannot allow the value to exceed 255, because
each of the 3 style properties is restricted to be a vector of at most
256 elements. See the comment about that in font.h.
> If we reach it 256 times
> (or thereabouts), why crash Emacs? If we want to crash Emacs in this
> case, why only crash it if --enable-checking has been specified?
The original code (from Emacs 23) called emacs_abort here, so it
aborted in both development and production builds. We could restore
that. I don't think it matters much, since I cannot imagine a
real-life situation where we get to that limit. And it's a separate
issue as well.
> font.c needs careful analysis
That is certainly true (and the same goes for fontset.c, btw).
> because there are too many apparent bugs
> in it right now (it also assumes that Vfont_encoding_alist isn't
> circular, for example).
I don't think it's quite as buggy as you think. It does have quite a
few places which are hard to explain, because the original experts who
developed that part of Emacs are no longer active in Emacs
development.
> "Minimal" changes are not the right thing to do here.
Let's disagree about that. Emacs is a large, old, and very stable
program, so "minimal changes" are IMO exactly the right thing, as long
as we don't change things radically. E.g., if someone wanted to
completely redesign and reimplement how Emacs selects fonts, and could
show convincingly that the new design will give us clear advantages in
speed and/or correctness, then of course "minimal changes" won't
apply. But otherwise making potentially disruptive changes in code we
don't understand well which works quite well on several very different
platforms is not TRT in my book.
> > elt = make_vector (2, make_fixnum (100));
>
> This is also strange: 100 is the "medium" weight; we should be using 80,
> which is the "normal" one. 100 is the correct default slant and width,
> though.
Emacs originally didn't distinguish between "normal" and "medium".
> > ASET (elt, 1, val);
> > - ASET (font_style_table, prop - FONT_WEIGHT_INDEX,
> > - CALLN (Fvconcat, table, make_vector (1, elt)));
> > + Lisp_Object new_table = CALLN (Fvconcat, table, make_vector (1, elt));
>
> This is also strange: the tables are documented to be sorted
> numerically, but I don't see any code that relies on that. vconcat
> certainly doesn't leave the tables sorted.
Probably one more sign that this code is not used in practice.
> I do see code, e.g. font_style_symbolic, which assumes that (i>>4) is
> the index corresponding to value i; this is not currently true, and
> font_style_symbolic will either eassert or fail to find the right entry.
Please elaborate: when and how this could not be true.
> In the absence of checking, it will most likely cause crashes...
>
> > + /* Update the corresponding variable with the new value. */
> > + switch (prop)
> > + {
> > + case FONT_WEIGHT_INDEX:
> > + Vfont_weight_table = new_table;
> > + break;
> > + case FONT_SLANT_INDEX:
> > + Vfont_slant_table = new_table;
> > + break;
> > + case FONT_WIDTH_INDEX:
> > + Vfont_width_table = new_table;
> > + break;
> > + default:
> > + break;
>
> "eassume (0);" is the right thing to do here, IMHO. If the default
> label isn't optimized out, it's redundant code. If we ever reach the
> default label, something else is severely wrong, so the
> eassume-as-eassert thing would save us.
We could do that, but the original code didn't, either. IOW, this is
a separate issue.
> So, sorry, further fixes required, a minimal fix won't work.
I was asking about the fix to the single issue I intended to fix.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 21:47:02 GMT)
Full text and
rfc822 format available.
Message #130 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Stefan Kangas <stefankangas <at> gmail.com>
>> Date: Mon, 13 Jan 2025 21:09:02 +0000
>> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>>
>> Yes, indeed, but that seems like an optimization that is not worth
>> doing. Not these days, at any rate.
>
> If you try to staticpro too many variables, the build will fail
> because Emacs runs out of space in staticvec. Not everyone knows what
> to do in this case, and it's an annoyance when this happens. So I
> would not say so easily that it's an optimization not worth doing, no.
NSTATICS was last increased by Paul in 2013 (4195afc389bb).
I ask to consider again what are the benefits of keeping this macro.
>> Note that with MPS, DEFVAR_LISP and DEFVAR_LISP_NOPRO are equivalent.
>
> When the igc branch lands, this will be a non-issue, yes. One more
> reason not to waste too much effort on this code now. But since the
> genie is out of the bottle, we must.
Are you okay with removing this on the scratch/igc branch only?
>> So by getting rid of DEFVAR_LISP_NOPRO, we would reduce the delta
>> between igc and master as a nice bonus.
>
> Why do we need to reduce the delta? what does that get us?
IME, all things being equal, it's always going to be easier to review
and eventually merge changes if the delta is smaller.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Tue, 14 Jan 2025 21:59:02 GMT)
Full text and
rfc822 format available.
Message #133 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Tue, 14 Jan 2025 17:02:41 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: stefankangas <at> gmail.com, 75521 <at> debbugs.gnu.org
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> Cc: 75521 <at> debbugs.gnu.org, stefankangas <at> gmail.com
>> >> Date: Mon, 13 Jan 2025 22:07:21 +0200
>> >> From: Eli Zaretskii <eliz <at> gnu.org>
>> >>
>> >>
>> >> Anyway, please don't install anything yet. I want to take a closer
>> >> look at this code and its usage, and I've ran out of time for today.
>> >
>> > WDYT about the patch below? It's supposed to keep the 3 variables in
>> > sync with the corresponding slots in font_style_table, if the slots
>> > are ever modified.
>>
>> If you meant to ask whether the patch will momentarily render this
>> specific bug harmless, I believe it will (but expose other bugs which
>> will still cause crashes).
>
> I was asking about the problem where that code makes the values of
> Vfont_weight_table and its 2 brethren vulnerable to being GC'ed.
> That's the only problem I tried to solve with that patch. That
> Vfont_weight_table etc. are also updated to reflect the changes in
> font_style_table is a nice benefit.
Not really a benefit: updating the variables might expose other
(crashable) bugs more.
> If you agree that this problem will be solved by the patch, then I'd
> like to install it, because it's very simple and localized, so the
> risk to break something is IMO very low.
It introduces what are effectively new crashable bugs by attempting to
fix old ones. I'm neutral: installing this patch won't improve things
significantly, but if you think there might be a slight improvement, you
probably have your reasons.
>> I've already answered the question of what I think about this approach:
>> "It's a very fragile approach, as evidenced by the fact that it was
>> broken for so long, and there's absolutely no benefit to it.
>
> I think this wasn't reported because that code is seldom if ever
> executed. I couldn't find a way to enter it. But I see no reason to
> remove it at this point.
No one suggested removing this code.
>> The next
>> person to attempt to change this code won't understand why we use a
>> non-staticpro'd variable which is kept in sync with a staticpro'd one
>> (because there is no reason to do so), and probably break things again."
>
> That can be alleviated a bit by adding commentary which explains this
> subtlety.
The "subtlety" is unnecessary complication, but I repeat myself. We
can't add a comment explaining the reason for it because it's not
necessary.
>> It also changes behavior, of course, and the new behavior doesn't make
>> much sense: with or without --enable-checking, it's trivial to cause
>> further crashes by modifying the Vfont_weight_table variable's value,
>> for example.
>
> Vfont_weight_table etc. are supposed not to be modifiable (it should
> cause Emacs to signal an error). If you can show a recipe for causing
> a crash by modifying them, I will certainly see why we don't behave as
> the doc string says.
??? Of course the vector value can be modified, it's just that the
variable cannot be assigned to. Faset doesn't care, and why would it?
> In any case, this is a separate issue.
Agreed. I'm not asking you not to install your patch (it's code churn,
but then it's possible it'll fix slightly more crashes than it
introduces), and please give me some time to think about whether it's
really worth it to go through another dozen-or-so email cycles just to
end up with another "fix" which makes Emacs even more complicated while
failing to address the actual issues.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Wed, 15 Jan 2025 13:54:01 GMT)
Full text and
rfc822 format available.
Message #136 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Tue, 14 Jan 2025 16:46:06 -0500
> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> From: Stefan Kangas <stefankangas <at> gmail.com>
> >> Date: Mon, 13 Jan 2025 21:09:02 +0000
> >> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
> >>
> >> Yes, indeed, but that seems like an optimization that is not worth
> >> doing. Not these days, at any rate.
> >
> > If you try to staticpro too many variables, the build will fail
> > because Emacs runs out of space in staticvec. Not everyone knows what
> > to do in this case, and it's an annoyance when this happens. So I
> > would not say so easily that it's an optimization not worth doing, no.
>
> NSTATICS was last increased by Paul in 2013 (4195afc389bb).
Actually, it was a year before, but I don't see how that changes the
fact that redundant protecting should be avoided.
> I ask to consider again what are the benefits of keeping this macro.
There are no benefits in keeping it. But there are risks in removing
it, because that requires changes in font.c, in code which we don't
understand well enough (see this discussion as the evidence), and thus
could inadvertently break by some change.
> >> Note that with MPS, DEFVAR_LISP and DEFVAR_LISP_NOPRO are equivalent.
> >
> > When the igc branch lands, this will be a non-issue, yes. One more
> > reason not to waste too much effort on this code now. But since the
> > genie is out of the bottle, we must.
>
> Are you okay with removing this on the scratch/igc branch only?
I have no problems with doing that on the branch conditioned by
"#ifdef HAVE_MPS", if indeed DEFVAR_LISP_NOPRO is equivalent to
DEFVAR_LISP there. But are they indeed equivalent? staticpro still
exists on the branch, and AFAICT we create a root from staticvec. So
why are they equivalent?
> >> So by getting rid of DEFVAR_LISP_NOPRO, we would reduce the delta
> >> between igc and master as a nice bonus.
> >
> > Why do we need to reduce the delta? what does that get us?
>
> IME, all things being equal, it's always going to be easier to review
> and eventually merge changes if the delta is smaller.
For such significant changes, I don't expect anyone to eyeball the
diffs when merging the branch.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Wed, 15 Jan 2025 17:53:02 GMT)
Full text and
rfc822 format available.
Message #139 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> From: Stefan Kangas <stefankangas <at> gmail.com>
>> Date: Tue, 14 Jan 2025 16:46:06 -0500
>> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>> >> From: Stefan Kangas <stefankangas <at> gmail.com>
>> >> Date: Mon, 13 Jan 2025 21:09:02 +0000
>> >> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>> >>
>> >> Yes, indeed, but that seems like an optimization that is not worth
>> >> doing. Not these days, at any rate.
>> >
>> > If you try to staticpro too many variables, the build will fail
>> > because Emacs runs out of space in staticvec. Not everyone knows what
>> > to do in this case, and it's an annoyance when this happens. So I
>> > would not say so easily that it's an optimization not worth doing, no.
>>
>> NSTATICS was last increased by Paul in 2013 (4195afc389bb).
>
> Actually, it was a year before, but I don't see how that changes the
> fact that redundant protecting should be avoided.
That sentence is the straw that broke the camel's back.
It's not a fact, it's an individual opinion which virtually everyone
else disagrees with (for very good reasons). I certainly do. Literally
the only argument that has been advanced to support this *opinion* is
that removing the *three* remaining _NOPROs (which are, at this point,
NOT redundant, but buggy) would somehow cause us to run out of NSTATICS,
which would mean more than doubling our staticpro calls overnight,
adding more than a thousand staticpros.
If diverging opinions are described as contradicting alleged "facts",
there is no point discussing this further.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Wed, 15 Jan 2025 19:47:02 GMT)
Full text and
rfc822 format available.
Message #142 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Are you okay with removing this on the scratch/igc branch only?
>
> I have no problems with doing that on the branch conditioned by
> "#ifdef HAVE_MPS", if indeed DEFVAR_LISP_NOPRO is equivalent to
> DEFVAR_LISP there.
Thanks, done in commit de864e4f3b2.
> But are they indeed equivalent? staticpro still
> exists on the branch, and AFAICT we create a root from staticvec. So
> why are they equivalent?
Yes, they had the same definition:
#define DEFVAR_LISP(lname, vname, doc) \
do { \
static struct Lisp_Fwd const o_fwd \
= {Lisp_Fwd_Obj, .u.objvar = &globals.f_##vname}; \
defvar_lisp (&o_fwd, lname); \
} while (false)
#ifdef HAVE_MPS
#define DEFVAR_LISP_NOPRO(lname, vname, doc) \
do { \
static struct Lisp_Fwd const o_fwd \
= {Lisp_Fwd_Obj, .u.objvar = &globals.f_##vname}; \
defvar_lisp (&o_fwd, lname); \
} while (false)
#else
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Wed, 15 Jan 2025 20:28:02 GMT)
Full text and
rfc822 format available.
Message #145 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Wed, 15 Jan 2025 17:52:13 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 75521 <at> debbugs.gnu.org
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> NSTATICS was last increased by Paul in 2013 (4195afc389bb).
> >
> > Actually, it was a year before, but I don't see how that changes the
> > fact that redundant protecting should be avoided.
>
> That sentence is the straw that broke the camel's back.
>
> It's not a fact, it's an individual opinion which virtually everyone
> else disagrees with (for very good reasons). I certainly do. Literally
> the only argument that has been advanced to support this *opinion* is
> that removing the *three* remaining _NOPROs (which are, at this point,
> NOT redundant, but buggy) would somehow cause us to run out of NSTATICS,
> which would mean more than doubling our staticpro calls overnight,
> adding more than a thousand staticpros.
It isn't an opinion. Wasting memory is bad, period.
We could argue whether 3 slots in staticvec is a significant waste or
not, and that would be a matter of opinion, but Stefan's question was
why do we do it, and I answered that.
All your other arguments are about something else, and are definitely
opinions. (Though I don't understand since when or why "opinion"
became a derogatory word around here.)
> If diverging opinions are described as contradicting alleged "facts",
> there is no point discussing this further.
Then don't.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Wed, 15 Jan 2025 20:46:01 GMT)
Full text and
rfc822 format available.
Message #148 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Wed, 15 Jan 2025 19:45:55 +0000
> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> Are you okay with removing this on the scratch/igc branch only?
> >
> > I have no problems with doing that on the branch conditioned by
> > "#ifdef HAVE_MPS", if indeed DEFVAR_LISP_NOPRO is equivalent to
> > DEFVAR_LISP there.
>
> Thanks, done in commit de864e4f3b2.
Too bad you did that while we are still discussing the issue. Why the
haste? The result is that we now have confusing code, see below.
> > But are they indeed equivalent? staticpro still
> > exists on the branch, and AFAICT we create a root from staticvec. So
> > why are they equivalent?
>
> Yes, they had the same definition:
>
> #define DEFVAR_LISP(lname, vname, doc) \
> do { \
> static struct Lisp_Fwd const o_fwd \
> = {Lisp_Fwd_Obj, .u.objvar = &globals.f_##vname}; \
> defvar_lisp (&o_fwd, lname); \
> } while (false)
> #ifdef HAVE_MPS
> #define DEFVAR_LISP_NOPRO(lname, vname, doc) \
> do { \
> static struct Lisp_Fwd const o_fwd \
> = {Lisp_Fwd_Obj, .u.objvar = &globals.f_##vname}; \
> defvar_lisp (&o_fwd, lname); \
> } while (false)
> #else
Heh, I was looking at these:
void
defvar_lisp_nopro (struct Lisp_Fwd const *o_fwd, char const *namestring)
{
eassert (o_fwd->type == Lisp_Fwd_Obj);
Lisp_Object sym = intern_c_string (namestring);
XBARE_SYMBOL (sym)->u.s.declared_special = true;
XBARE_SYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED;
SET_SYMBOL_FWD (XBARE_SYMBOL (sym), o_fwd);
}
void
defvar_lisp (struct Lisp_Fwd const *o_fwd, char const *namestring)
{
eassert (o_fwd->type == Lisp_Fwd_Obj);
defvar_lisp_nopro (o_fwd, namestring);
staticpro (o_fwd->u.objvar);
}
(which are still different) because I remembered that the macros just
called the functions. And your changes didn't remove
defvar_lisp_nopro. Isn't that confusing?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Wed, 15 Jan 2025 21:05:02 GMT)
Full text and
rfc822 format available.
Message #151 received at 75521 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Wed, 15 Jan 2025 17:52:13 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: Stefan Kangas <stefankangas <at> gmail.com>, 75521 <at> debbugs.gnu.org
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> NSTATICS was last increased by Paul in 2013 (4195afc389bb).
>> >
>> > Actually, it was a year before, but I don't see how that changes the
>> > fact that redundant protecting should be avoided.
>>
>> That sentence is the straw that broke the camel's back.
>>
>> It's not a fact, it's an individual opinion which virtually everyone
>> else disagrees with (for very good reasons). I certainly do. Literally
>> the only argument that has been advanced to support this *opinion* is
>> that removing the *three* remaining _NOPROs (which are, at this point,
>> NOT redundant, but buggy) would somehow cause us to run out of NSTATICS,
>> which would mean more than doubling our staticpro calls overnight,
>> adding more than a thousand staticpros.
>
> It isn't an opinion. Wasting memory is bad, period.
This is too ridiculous to leave uncommented: staticpro does not allocate
or increase memory usage in any way. No memory is wasted. staticvec
has a static size, and memory in it that's unused is not available for
other purposes.
(Of course, both the existence of a separate _NOPRO macro and the
now-redundant font_style_table vector do waste memory, which is one of
the reasons why my patch, which is being rejected because "wasting
memory is bad", removed them.)
> All your other arguments are about something else, and are definitely
> opinions. (Though I don't understand since when or why "opinion"
> became a derogatory word around here.)
You claimed your opinion was a fact. That might have been an honest
mistake, but it was pointed out to you and you responded with another
falsehood.
There's only so much good faith one can assume.
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Wed, 15 Jan 2025 21:56:01 GMT)
Full text and
rfc822 format available.
Message #154 received at 75521 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Stefan Kangas <stefankangas <at> gmail.com>
>> Date: Wed, 15 Jan 2025 19:45:55 +0000
>> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>>
> Too bad you did that while we are still discussing the issue. Why the
> haste? The result is that we now have confusing code, see below.
Sorry, I thought we had agreed already. See below.
> Heh, I was looking at these:
>
> void
> defvar_lisp_nopro (struct Lisp_Fwd const *o_fwd, char const *namestring)
> {
> eassert (o_fwd->type == Lisp_Fwd_Obj);
> Lisp_Object sym = intern_c_string (namestring);
> XBARE_SYMBOL (sym)->u.s.declared_special = true;
> XBARE_SYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED;
> SET_SYMBOL_FWD (XBARE_SYMBOL (sym), o_fwd);
> }
>
> void
> defvar_lisp (struct Lisp_Fwd const *o_fwd, char const *namestring)
> {
> eassert (o_fwd->type == Lisp_Fwd_Obj);
> defvar_lisp_nopro (o_fwd, namestring);
> staticpro (o_fwd->u.objvar);
> }
>
> (which are still different) because I remembered that the macros just
> called the functions. And your changes didn't remove
> defvar_lisp_nopro. Isn't that confusing?
Aha, okay. I misunderstood your question.
I didn't think it was worth ifdef'ing those two functions for HAVE_MPS,
as I didn't consider that someone might find it confusing. Do you think
it would be better if we did?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Thu, 16 Jan 2025 06:08:01 GMT)
Full text and
rfc822 format available.
Message #157 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Wed, 15 Jan 2025 21:04:14 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: stefankangas <at> gmail.com, 75521 <at> debbugs.gnu.org
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> It's not a fact, it's an individual opinion which virtually everyone
> >> else disagrees with (for very good reasons). I certainly do. Literally
> >> the only argument that has been advanced to support this *opinion* is
> >> that removing the *three* remaining _NOPROs (which are, at this point,
> >> NOT redundant, but buggy) would somehow cause us to run out of NSTATICS,
> >> which would mean more than doubling our staticpro calls overnight,
> >> adding more than a thousand staticpros.
> >
> > It isn't an opinion. Wasting memory is bad, period.
>
> This is too ridiculous to leave uncommented:
Please be at least polite.
> staticpro does not allocate or increase memory usage in any way. No
> memory is wasted. staticvec has a static size, and memory in it
> that's unused is not available for other purposes.
AFAIU, it does increase memory because on modern OS only used memory
is actually paged into the process.
> (Of course, both the existence of a separate _NOPRO macro and the
> now-redundant font_style_table vector do waste memory, which is one of
> the reasons why my patch, which is being rejected because "wasting
> memory is bad", removed them.)
Memory "wasted" on code we need to have is not a waste. If we'd
considered it to be a waste, none of the changes we install (including
yours) will be acceptable, since almost all of them add code and/or
data.
> You claimed your opinion was a fact. That might have been an honest
> mistake, but it was pointed out to you and you responded with another
> falsehood.
>
> There's only so much good faith one can assume.
Yes, and you are dangerously close to overstep that line. So please
re-read all your recent messages, which almost always include some
unwarranted personal attack on me, and try to find a way of arguing
about technical matters without ad-hominem. GNU Kind Communications
guidelines are in effect here, mandatory for all of us.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Thu, 16 Jan 2025 06:35:02 GMT)
Full text and
rfc822 format available.
Message #160 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefankangas <at> gmail.com>
> Date: Wed, 15 Jan 2025 13:55:35 -0800
> Cc: pipcet <at> protonmail.com, 75521 <at> debbugs.gnu.org
>
> I didn't think it was worth ifdef'ing those two functions for HAVE_MPS,
> as I didn't consider that someone might find it confusing. Do you think
> it would be better if we did?
I think defvar_lisp_nopro should be a static function in the HAVE_MPS
build, since it will not be called from anywhere but defvar_lisp. And
its commentary should be changed accordingly.
I'd also appreciate a comment for DEFVAR_LISP_NOPRO in lisp.h,
explaining what it does and saying that it is only used in non-MPS
build.
I can make these changes myself, if you prefer.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75521; Package
emacs.
(Thu, 16 Jan 2025 15:55:02 GMT)
Full text and
rfc822 format available.
Message #163 received at 75521 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 14 Jan 2025 21:58:37 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: stefankangas <at> gmail.com, 75521 <at> debbugs.gnu.org
>
> > I was asking about the problem where that code makes the values of
> > Vfont_weight_table and its 2 brethren vulnerable to being GC'ed.
> > That's the only problem I tried to solve with that patch. That
> > Vfont_weight_table etc. are also updated to reflect the changes in
> > font_style_table is a nice benefit.
>
> Not really a benefit: updating the variables might expose other
> (crashable) bugs more.
>
> > If you agree that this problem will be solved by the patch, then I'd
> > like to install it, because it's very simple and localized, so the
> > risk to break something is IMO very low.
>
> It introduces what are effectively new crashable bugs by attempting to
> fix old ones. I'm neutral: installing this patch won't improve things
> significantly, but if you think there might be a slight improvement, you
> probably have your reasons.
Thanks, so I've now installed the patch on the master branch.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Fri, 14 Feb 2025 12:24:13 GMT)
Full text and
rfc822 format available.
This bug report was last modified 122 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.