Subject: Re: bug#75318: Lisp_Subr header and layout

From: Pip Cet <[email-masked]>
Date: Wed, 22 Jan 2025 14:25:22 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Cc: Po Lu <[email-masked]>, [email-masked]
>> Date: Tue, 21 Jan 2025 23:11:13 +0000
>> From:  Pip Cet via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <[email-masked]>
>>
>> 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).
>
> While I realize that this is a battle lost long ago, I cannot resist
> lamenting the once-simple way many lisp.h macros and inline functions
> were defined, which is all but gone now, replaced by the never-ending
> rabbit hole of macros that call macros that call macros that call...
> Add to this the fact that each macro/inline function has several
> different definitions, whose conditions are sometimes so complicated
> that the only reasonable way to quickly understand what definition a
> particular build uses is to fire up good-old GDB and evaluate the
> macros there, and you get the picture.

I do.  I agree completely.  I don't know how to fix it.

XIL is necessary for CHECK_LISP_OBJECT_TYPE (and there seems to be a
consensus we can't unconditionally enable that one yet), but I don't see
why we can't make an exception for Qnil and make the simple definition
of lisp_h_Qnil apply to this specific symbol in all circumstances:

#define Qnil XIL(0)

If we used that instead, it would make LISPSYM_INITIALLY slightly harder
to find because fewer places would use it.

> For example, in the below:
>
>>    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}};
>
> what is lisp_h_Qnil is relatively easy to understand, whereas the
> replacement, viz.:

>> +#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),			\
>
> requires one to look up LISPSYM_INITIALLY, which is

If you're saying that LISPSYM_INITIALLY is bad, I agree completely.

If you're saying a single LISPSYM_INITIALLY in DEFUN, which is
well-documented to the point that users do not have to read the
definition, is worse than three of them in C files, I don't.

>   #define LISPSYM_INITIALLY(name) \
>     TAG_PTR_INITIALLY (Lisp_Symbol, (intptr_t) ((i##name) * sizeof *lispsym))
>
> and then you need to go down the rabbit hole:
>
>   TAG_PTR_INITIALLY
>    => LISP_INITIALLY
>        => LISP_WORD_TAG
>            => VALBITS
>
> Wonderful, right?

I had forgotten how hard it was to get used to this.

I think we should definitely avoid making the situation worse, even if
that means we can't do fun stuff.

> (No, I don't expect you to change anything in your patch, just realize
> the plight.)

Thank you, in any case.

>> +#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						\
>> +    }}
>> +
>
> To make the depth of the proverbial rabbit hole at least to some
> extent smaller here, could we please refrain from defining
> SUBR_NATIVECOMP_INITIALIZER, and instead include the necessary stuff
> directly in SUBR_INITIALIZER, with an #ifdef?  Or do you envision
> enough other users of SUBR_NATIVECOMP_INITIALIZER to justify yet
> another macro?

Do you mean

#ifdef HAVE_NATIVE_COMP
#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),				\
    .native_comp_u = LISPSYM_INITIALLY (Qnil),		\
    .lambda_list = LISPSYM_INITIALLY (Qnil),		\
    .type = LISPSYM_INITIALLY (Qnil),
    }}
#else
#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 },				\
    }}
#endif

(and, I assume, the same thing for SUBR_HEADER?)

Avoiding the repetition appears to require a second macro no matter what
I do.

While *I* realize *that* battle was lost long ago, I still think
nativecomp subrs should have had their own PVEC type.

It's too late for that now; let's not revisit a discussion that I
remember as very fair, and which definitely concluded that we'd share
the PVEC type.

I'm thinking (very vaguely; no patch expected in 2025) about proposing
to reduce all Lisp_Objects in the subr object to a simple plist, though,
or a vector if we have to.  I think 32 bytes is a very good size for a
subr to have, and that's header, plist (Lisp_Object so first),
minargs+maxargs+4-byte gap, function pointer union.

It's easier to GC-trace an ordinary PVEC with lisplen=1 and restlen=2
without worrying about whether it is a subr, I tihnk.

Pip