Subject: bug#75318: Lisp_Subr header and layout

From: Eli Zaretskii <[email-masked]>
Date: Wed, 22 Jan 2025 18:07:43 +0200

> Date: Wed, 22 Jan 2025 15:30:24 +0000
> From: Pip Cet <[email-masked]>
> Cc: Andrea Corallo <[email-masked]>, [email-masked], [email-masked]
> 
> "Eli Zaretskii" <[email-masked]> writes:
> 
> >> Cc: [email-masked]
> >> 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" <[email-masked]>
> >>
> >> 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?

> 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?

> 
> > 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.

Well, both problems exist, they just push us in opposite directions.

> 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.

> 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.