GNU bug report logs - #75318
Lisp_Subr header and layout

Previous Next

Full log

Message #41 received at 75318 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acorallo <at> gnu.org, monnier <at> iro.umontreal.ca, 75318 <at> debbugs.gnu.org
Subject: Re: bug#75318: Lisp_Subr header and layout
Date: Wed, 22 Jan 2025 16:38:26 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Wed, 22 Jan 2025 15:30:24 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: Andrea Corallo <acorallo <at> gnu.org>, 75318 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> Cc: monnier <at> iro.umontreal.ca
>> >> Date: Tue, 21 Jan 2025 23:46:45 +0000
>> >> From:  Pip Cet via "Bug reports for GNU Emacs,
>> >>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> >>
>> >> diff --git a/src/lisp.h b/src/lisp.h
>> >> index 9c31630cc1a..46e940dad09 100644
>> >> --- a/src/lisp.h
>> >> +++ b/src/lisp.h
>> >> @@ -2226,12 +2226,12 @@ CHAR_TABLE_SET (Lisp_Object ct, int idx, Lisp_Object val)
>> >>        const char *string;
>> >>        Lisp_Object native;
>> >>      } intspec;
>> >> -    Lisp_Object command_modes;
>> >>      /* Positive values: offset into etc/DOC.  Negative values: one's
>> >>         complement of index into the native comp unit's vector of
>> >>         documentation strings.  */
>> >>      EMACS_INT doc;
>> >>  #ifdef HAVE_NATIVE_COMP
>> >> +    Lisp_Object command_modes;
>> >>      Lisp_Object native_comp_u;
>> >>      char *native_c_name;
>> >>      Lisp_Object lambda_list;
>> >
>> > IME, having different definitions of data structures under different
>> > build conditions is a problematic solution: it makes it harder to make
>>
>> That statement applies to the status quo before the patch just as much
>> as it does to the new status, though, right?
>
> Could be; I didn't check.  It's a general statement.
>
>> > safe changes that don't break some build.  Many/most people don't try
>>
>> This doesn't; it's impossible ta safely use the command_modes field for
>> the reasons I explained.
>
> Can you point me to the explanation, or explain again?

GC doesn't trace the field, pdumper doesn't save it; the sole remaining
user expects (and insists) on a Qnil value.  Iff we wanted to fix the
first we'd have to add new users for testing, if we want to fix and test
the second we'd have to add pre-dump users, which is harder.  Pdumper
also asserts that the field isn't used, so it's possible other things
will break, because I'm sure that assertion was put there for a reason.

>> And making it safe without using it at all seems both wasteful and all
>> but impossible: we'd have to test the code somehow, making up usages of
>> a field that hasn't been in real use for a long time.
>
> I just wanted to avoid breaking a build by referencing the struct
> member in a build that doesn't have it.  There's no need to ensure the
> member is always used safely.  Doesn't initializing it to nil solve
> this?

I don't understand that at all, sorry.  If anyone uses the field for
anything but checking its nil-ness in a non-nativecomp build, they
either get a compile-time error or a very frustrating inexplicable
crash.  Changing from the latter to the former is the best we can do.

>> The new patch does still make it possible for someone using only
>> nativecomp builds to accidentally use the field for non-nativecomp
>> subrs.  If someone tried to build that tree without nativecomp, they
>> would get a compiler error, which is the best we can do to warn them of
>> this situation, given the constraint that we use the same structure for
>> nativecomp subrs and primitive ones.
>
> OTOH, adding code that references the field without #ifdef, and
> installing it without trying a non-nativecomp build, will break
> everybody who builds --without-native-compilation.

Indeed. It'd be nice to statically check more things before we push to
master (and, generally, compiling a C program and checking it for errors
are two separate things; the same is true for texinfo, it seems).  I
honestly think we should add a separate "lint" pass (by this or any
other name) sooner rather than later, but right now, we don't.  The best
we can do is CI and breaking the build rather than crashing
inexplicably.

(Random example: print.c exposes the string "unreadeable-function"
(meaning "unreadable-function") to Lisp and expects Lisp to provide it
in some contexts.  Obviously not something GCC can check for during
every build, but if we'd run a weekly typo lint, we could have caught it
in time.  I've let the resident typo expert know, but this probably
requires a bug report and discussion now).

But, again, pie-in-the-sky dreams of a better build system aside, this
is strictly about compile-time errors vs run-time bugs.

>> People might use it, and run into the various bugs that have
>> accumulated.  It's better to remove the unusable field (and there's all
>> that talk about not wasting two words of staticvec, but wasting 1,000
>> words is suddenly okay?).
>
> If this leads to such a significant memory waste, then indeed what I
> proposed is a non-starter, unfortunately.

Insignificant for my current system, unfortunately, which aligns
static Lisp_Subr to 32-byte boundaries (for good reason).  However, as
there are many systems with different alignments and sizes, saving a
32-byte line every fourth time we remove a word has the same expected
benefit as regaining 8 bytes the second we do so.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.