Package: emacs;
Reported by: Pip Cet <pipcet <at> protonmail.com>
Date: Fri, 3 Jan 2025 14:31:02 UTC
Severity: normal
Message #11 received at 75318 <at> debbugs.gnu.org (full text, mbox):
From: Pip Cet <pipcet <at> protonmail.com> To: 75318 <at> debbugs.gnu.org Cc: Po Lu <luangruo <at> yahoo.com>, monnier <at> iro.umontreal.ca Subject: Re: bug#75318: Lisp_Subr header and layout Date: Tue, 21 Jan 2025 23:11:13 +0000
"Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text editors\"" <bug-gnu-emacs <at> gnu.org> writes:
> I'll prepare patches for those two changes (remove command_modes;
> correct pvec headers for subrs); not the third (reorder Lisp_Subr
> fields) because it's a nativecomp change, and there may be a small
> performance impact.
The second patch fixes Lisp_Subr initialization: the old code set the
header to an incorrect value first (PVEC_SUBR was in the type field, but
the PSEUDOVECTOR_FLAG was not set, making it look like a huge ordinary
vector). Then defsubr wrote to the Lisp_Subr structure to fix that, by
calling XSETPVECTYPE: this didn't affect the type field, but did set the
PSEUDOVECTOR_FLAG. The size fields always indicated a size of zero.
But three static Lisp_Subr structures weren't defined via defsubr, and
did set the PSEUDOVECTOR_FLAG initially, but otherwise contained
verbatim copies of the DEFUN macro's initialization code.
Ideally, we'd turn these three into plain DEFUNs, with docstrings
indicating they're internal functions. I'd also like to turn the
underscores into ordinary dashes in the Lisp function names if we do
that. If we don't, we might at least want to move them to the .subrs
section, or move them out of function scope (function-scope static
variables aren't easy to debug with gdb).
This patch splits up the macros so they're reusable, initializes using
LISPSYM_INITIALLY rather than lisp_h_Qnil, ensures that all Lisp_Object
fields are initialized, uses explicit field-name initializers, and uses
c-mode default indentation (this is incorrect in one place, but it's
hardly noticeable).
This change alone is not sufficient to make the .subrs section read-only.
See store_function_docstring and pdumper.
Like the first one, this second patch fixes things that appear to be
unintended, but that never caused any bugs on the master branch. The
oddities did cause problems when playing with feature/igc and other GC
stuff, though.
From a67f72e44f5cc9b4af48f0ac3503d3286b0c91a0 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH 2/2] Fix Lisp_Subr initialization peculiarities (bug#75318)
* src/lisp.h (SUBR_HEADER, SUBR_INITIALIZER)
(SUBR_NATIVECOMP_INITIALIZER): New macros.
(DEFUN): Use them. Initialize the header directly to a correct value.
* src/alloc.c (Swatch_gc_cons_threshold, Swatch_gc_cons_percentage):
* src/sfntfont.c (Scompare_font_entities): Use SUBR_INITIALIZER.
* src/lread.c (defsubr): Don't call XSETPVECTYPE to initialize
pseudovector flag.
---
src/alloc.c | 10 ++++------
src/lisp.h | 29 +++++++++++++++++++++++++----
src/lread.c | 1 -
src/sfntfont.c | 10 +++-------
4 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/src/alloc.c b/src/alloc.c
index c4e2ff52015..3425f913362 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -8208,16 +8208,14 @@ syms_of_alloc (void)
Lisp_Object watcher;
static union Aligned_Lisp_Subr Swatch_gc_cons_threshold =
- {{{ PSEUDOVECTOR_FLAG | (PVEC_SUBR << PSEUDOVECTOR_AREA_BITS) },
- { .a4 = watch_gc_cons_threshold },
- 4, 4, "watch_gc_cons_threshold", {0}, lisp_h_Qnil}};
+ SUBR_INITIALIZER ("watch_gc_cons_threshold", watch_gc_cons_threshold,
+ 4, 4, NULL);
XSETSUBR (watcher, &Swatch_gc_cons_threshold.s);
Fadd_variable_watcher (Qgc_cons_threshold, watcher);
static union Aligned_Lisp_Subr Swatch_gc_cons_percentage =
- {{{ PSEUDOVECTOR_FLAG | (PVEC_SUBR << PSEUDOVECTOR_AREA_BITS) },
- { .a4 = watch_gc_cons_percentage },
- 4, 4, "watch_gc_cons_percentage", {0}, lisp_h_Qnil}};
+ SUBR_INITIALIZER ("watch_gc_cons_percentage", watch_gc_cons_percentage,
+ 4, 4, NULL);
XSETSUBR (watcher, &Swatch_gc_cons_percentage.s);
Fadd_variable_watcher (Qgc_cons_percentage, watcher);
DEFSYM (Qalloc, "alloc");
diff --git a/src/lisp.h b/src/lisp.h
index 8b870119315..e9c975b81e8 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3481,15 +3481,36 @@ CHECK_SUBR (Lisp_Object x)
A null string means call interactively with no arguments.
`doc' is documentation for the user. */
+#define SUBR_HEADER { .size = \
+ PVECHEADERSIZE (PVEC_SUBR, 0, VECSIZE (union Aligned_Lisp_Subr)) }
+
+#ifdef HAVE_NATIVE_COMP
+#define SUBR_NATIVECOMP_INITIALIZER \
+ .native_comp_u = LISPSYM_INITIALLY (Qnil), \
+ .lambda_list = LISPSYM_INITIALLY (Qnil), \
+ .type = LISPSYM_INITIALLY (Qnil),
+#else
+#define SUBR_NATIVECOMP_INITIALIZER
+#endif
+
+#define SUBR_INITIALIZER(lname, fnname, minargs, maxargs, intspec_value) \
+ {{ .header = SUBR_HEADER, \
+ .function = { .a ## maxargs = fnname }, \
+ .min_args = minargs, \
+ .max_args = maxargs, \
+ .symbol_name = lname, \
+ .intspec = { .string = intspec_value }, \
+ .command_modes = LISPSYM_INITIALLY (Qnil), \
+ SUBR_NATIVECOMP_INITIALIZER \
+ }}
+
/* This version of DEFUN declares a function prototype with the right
arguments, so we can catch errors with maxargs at compile-time. */
#define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc) \
SUBR_SECTION_ATTRIBUTE \
static union Aligned_Lisp_Subr sname = \
- {{{ PVEC_SUBR << PSEUDOVECTOR_AREA_BITS }, \
- { .a ## maxargs = fnname }, \
- minargs, maxargs, lname, {intspec}, lisp_h_Qnil}}; \
- Lisp_Object fnname
+ SUBR_INITIALIZER(lname, fnname, minargs, maxargs, intspec); \
+ Lisp_Object fnname
/* defsubr (Sname);
is how we define the symbol for function `name' at start-up time. */
diff --git a/src/lread.c b/src/lread.c
index e5d1ba0e6bb..279d384eed6 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -5475,7 +5475,6 @@ defsubr (union Aligned_Lisp_Subr *aname)
struct Lisp_Subr *sname = &aname->s;
Lisp_Object sym, tem;
sym = intern_c_string (sname->symbol_name);
- XSETPVECTYPE (sname, PVEC_SUBR);
XSETSUBR (tem, sname);
set_symbol_function (sym, tem);
#ifdef HAVE_NATIVE_COMP
diff --git a/src/sfntfont.c b/src/sfntfont.c
index 5752ff81c1c..b7a8b7c2671 100644
--- a/src/sfntfont.c
+++ b/src/sfntfont.c
@@ -1985,13 +1985,9 @@ sfntfont_compare_font_entities (Lisp_Object a, Lisp_Object b)
fields are set within the first than in the second. */
static union Aligned_Lisp_Subr Scompare_font_entities =
- {
- {
- { PSEUDOVECTOR_FLAG | (PVEC_SUBR << PSEUDOVECTOR_AREA_BITS), },
- { .a2 = sfntfont_compare_font_entities, },
- 2, 2, "sfntfont_compare_font_entities", {0}, lisp_h_Qnil,
- },
- };
+ SUBR_INITIALIZER ("sfntfont_compare_font_entities",
+ sfntfont_compare_font_entities,
+ 2, 2, NULL);
/* Return a list of font-entities matching the specified
FONT_SPEC. */
--
2.47.1
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.