bug#72802: 31.0.50; Crash in (equal sub-char-table-a sub-char-table-b)

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sun, 25 Aug 2024 14:11:17 +0000
> From:  Pip Cet via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Pip Cet <pipcet@protonmail.com> writes:
> > The code in internal_equal compares sub-char-tables incorrectly
> 
> This patch should fix things for the release branch:

I don't see a reason to install this on the release branch now.  Not
even on master, I think, unless we see a bug related to it that is not
caused by a specially-concocted Lisp program or GDB command.

If this is required for the no-pure-space branch, I think you should
install this on that branch.  Then it will be merged together with the
branch.

If you see some urgent need to fix this ASAP on master, please tell
why you think so.

> +	    /* Bug#72802.  See 'mark_char_table' in alloc.c.  */
> +	    if (SUB_CHAR_TABLE_P (o1))
> +	      {
> +		i = SUB_CHAR_TABLE_OFFSET;
> +		if (XSUB_CHAR_TABLE (o1)->depth !=
> +		    XSUB_CHAR_TABLE (o2)->depth)
> +		  return false;
> +		if (XSUB_CHAR_TABLE (o1)->min_char !=
> +		    XSUB_CHAR_TABLE (o2)->min_char)
> +		  return false;
> +	      }

I looked at mark_char_table, trying to understand what the above
comments wants to say, and couldn't.  So I think the comment should
be improved, so that it would be more clear what it wants to say.

Also, please move the assignment of SUB_CHAR_TABLE_OFFSET to i to
after the two early 'return's.

> For the master branch, I think the right thing to do is to turn the
> first two, non-Lisp members of Lisp_Sub_Char_Table ('depth' and
> 'min_char') into 'Lisp_Object's.  Then we can simplify the code and
> compare sub char tables as we do ordinary vectors, at the cost of eight
> bytes of extra storage per sub char table on machines with 64-bit
> EMACS_INTs.

I'm not sure we want to pay this cost.  What bothers me is mainly the
run-time cost of extracting integers from Lisp objects.  char-table is
supposed to be very efficient, both memory-wise and CPU-wise, and I
think the performance here trumps simplicity.

> BTW, I'm surprised this code returns nil; I think that should be
> documented.
> 
> (setq a (make-char-table nil))
> (setq b (make-char-table nil))
> (aset a 1 nil)
> (dotimes (i (max-char))
>   (unless (equal (aref a i) (aref b i))
>     (error "i = %S" i)))
> (equal a b)

Why are you surprised?  Setting a single cell of a char-table changes
its structure, usually in quite a radical way.  'aref' does some very
special things for char-tables; the semantics of accessing an element
of a vector is only correct superficially, not in the details.

The internals of a char-table are not really documented in the ELisp
manual; the description there is mostly phenomenological, without any
details.  If you want to document the internals, I think the proper
place is to add a comment at the beginning of chartab.c with these
details (and there are a lot of details not really documented
anywhere, at least not explicitly), and then this nit should be part
of that.