bug#65491: [PATCH] Improve performance allocating vectors

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sat, 16 Sep 2023 12:46:34 -0700
> Cc: 65491@debbugs.gnu.org, monnier@iro.umontreal.ca
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 2023-09-16 10:09, Eli Zaretskii wrote:
> 
> > I also added Paul to the discussion, since he wrote most of the
> > involved macros.
> 
> Mattias and I had a private email discussion about XUNTAG last month, 
> and he's correct that Emacs's current code, which #defines XUNTAG(a, 
> type, ctype) to ((ctype *) ((char *) XLP (a) - LISP_WORD_TAG (type))), 
> violates the C standard due to calculating a pointer outside its 
> containing object's bounds, whereas the patch P that you just reverted, 
> which defines the same macro to ((ctype *) ((uintptr_t) XLP (a) - 
> (uintptr_t) LISP_WORD_TAG (type))), does not have that particular 
> undefined behavior.

First, may I ask that in the future such discussions be held on the
list, or at least their summary be posted?  You can see right here and
now how this practice of discussing purely technical issues in private
causes unnecessary friction and aggravation, let alone breakage.

Next, even if this was discussed, due to our experience with changes
in this area, it would have been prudent to post the proposed patch
before installing it, so that it could be tested in all the supported
configuration before risking such a fundamental breakage of the master
branch.  You personally do that all the time, and for good reasons,
but Mattias has yet to learn that, it seems.

More to the point: can you explain how this part

   ((uintptr_t) XLP (a) - (uintptr_t) LISP_WORD_TAG (type))

works in a 32-bit build --with-wide-int?  AFAIU, XLP(a) yields a
32-bit value, and LISP_WORD_TAG has all of its low 32 bits zero, so
why do we need the subtraction at all?  Aren't we subtracting a zero
from what XLP yields?  It seems to me that in a 32-bit build with wide
ints just

  #define XUNTAG(a, type, ctype) ((ctype *) XLP (a))

should be enough, since XLP yields a 'void *', no?

> I would not favor a two-pronged approach, in which patch P is present 
> but is used optionally. This would likely cause more trouble than it'd 
> cure, due to lack of testing of the extra combinations. Let's use just 
> one approach and keep it simple and more testable.

Not sure what you are alluding to here, but if by "two-pronged
approach" you mean my suggestion to use cpp conditionals, then what I
meant was to have a separate definition of XUNTAG for 32-bit builds
with wide ints (which could still remove the undefined behavior), just
without all the juggling of integer-to-pointer-and-back conversions,
which completely obscure what should be a simple operation of
extracting a pointer from an integer by masking some of its bits.  Why
do I need to fight with these macros each time I read this code, when
the job it does is so simple?

Thanks.