Package: emacs;
Reported by: Pip Cet <pipcet <at> protonmail.com>
Date: Fri, 3 Jan 2025 14:31:02 UTC
Severity: normal
Message #32 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: Andrea Corallo <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 15:30:24 +0000
"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? Lisp_Subr had three
conditional fields before, plus one that's unconditionally compiled in
but only usable if one of the other three is non-nil. Now it has four
conditional fields, which are unusable if they don't exist because the
compiler won't let you.
> 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.
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.
> to build all of the configurations Emacs supports when they make
> changes, so it is easy to write code which references a struct member
> that doesn't exist in some build configuration, and thus break it, and
> miss the fact that build is broken by not trying it. (It is
> unrealistic to expect everyone who has write access to the repository
> to test-build all the possible configurations Emacs supports as a
> prerequisite for installing a change.)
I agree, but I don't see how that applies here. Before the patch, if
someone had started using nativecomp-only command_modes, we would have
had very tricky GC/pdumper bugs which would have taken much longer to
resolve than simply removing an unused field.
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.
Of course it would be better if we statically asserted that we're
looking at a nativecomp subr before accessing the field, but we've
discussed that and concluded sharing a C structure is a constraint which
I obeyed in this patch.
If you think this field is significant enough to warrant it (it's used
once in comp.c, once in data.c, and three times by pdumper.c; two of the
pdumper usages can be merged because the buggy one would have to be
replaced by the non-buggy one anyway. The third is an assertion), we
could add accessors, but it's simply not.
> So: do we really need to remove command_modes from the struct in a
> build without native compilation? What bad things will happen if we
Yes.
> leave it there? If we would like to be a bit more efficient by
> avoiding a few function calls, like here:
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?).
>> --- a/src/pdumper.c
>> +++ b/src/pdumper.c
>> @@ -2992,16 +2992,15 @@ dump_subr (struct dump_context *ctx, const struct Lisp_Subr *subr)
>> COLD_OP_NATIVE_SUBR,
>> make_lisp_ptr ((void *) subr, Lisp_Vectorlike));
>> dump_field_lv (ctx, &out, subr, &subr->intspec.native, WEIGHT_NORMAL);
>> - dump_field_lv (ctx, &out, subr, &subr->command_modes, WEIGHT_NORMAL);
>> }
>> else
>> {
>> dump_field_emacs_ptr (ctx, &out, subr, &subr->symbol_name);
>> dump_field_emacs_ptr (ctx, &out, subr, &subr->intspec.string);
>> - dump_field_emacs_ptr (ctx, &out, subr, &subr->command_modes);
>> }
>
> then it should be okay to #ifdef away those calls when
> !HAVE_NATIVE_COMP, as that doesn't run the risks mentioned above.
That is precisely what I did in the code you cite: the two - lines you
show turned into a single + line behind an #ifdef, which is *precisely*
what we do for the other three fields which are usable in equivalent
circumstances. Adding a separate #ifdef for two lines that should have
been identical but weren't, a bug that never became relevant only
because the field was unused.
(The bug would have been avoided by some macro-based
check-this-is-a-non-void-pointer code, see the discussion before).
> I also would like to hear from Andrea about this patch.
Absolutely, let's.
Thanks for your comments.
You have convinced me that the .intspec.native union member also
shouldn't exist if we're building without nativecomp. As we've seen
with Lisp_Subr->doc, using the same field for different purposes is
fragile and leads to bugs.
I won't revise the patch for now as this minor change doesn't really
change things significantly: intspec.native is only set in one place and
read in another one, plus GC/pdumper bookkeeping. A comment would
suffice, too.
Pip
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.