HAVE_FAST_UNALIGNED_ACCESS

HAVE_FAST_UNALIGNED_ACCESS

[Robert Pluim]

Sending this on behalf of Po Lu, whoʼs still having email problems:

Fstring_lessp has:

/* Check whether the platform allows access to unaligned addresses for
   size_t integers without trapping or undue penalty (a few cycles is OK).

   This whitelist is incomplete but since it is only used to improve
   performance, omitting cases is safe.  */
#if defined __x86_64__|| defined __amd64__	\
    || defined __i386__ || defined __i386	\
    || defined __arm64__ || defined __aarch64__	\
    || defined __powerpc__ || defined __powerpc	\
    || defined __ppc__ || defined __ppc		\
    || defined __s390__ || defined __s390x__
#define HAVE_FAST_UNALIGNED_ACCESS 1
#else
#define HAVE_FAST_UNALIGNED_ACCESS 0
#endif

but even if unaligned access is normally permitted by a machine, it is
still undefined behavior to dereference an unaligned pointer.

Instead, HAVE_FAST_UNALIGNED_ACCESS and UNALIGNED_LOAD_SIZE should be
removed and memcpy used instead:

  word_t a, c;

  memcpy (&a, w1 + b / ws, sizeof a);
  memcpy (&c, w2 + b / ws, sizeof c);

doing so will make the compiler itself generate the right sequence of
instructions for performing unaligned accesses, normally with only a few
cycles penalty.  Some RISCs have explicit ``load unaligned''
instructions, and others (such as MIPS and the Alpha) need special
sequences of instructions to perform such loads, and the compiler will
DTRT.

I would like to install such a change on emacs-29.  Emacs currently
crashes when built with various compilers performing pointer alignment
checks.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Po Lu <luangruo@yahoo.com>
> Date: Thu, 30 Mar 2023 11:34:42 +0200
> 
> Fstring_lessp has:
> 
> /* Check whether the platform allows access to unaligned addresses for
>    size_t integers without trapping or undue penalty (a few cycles is OK).
> 
>    This whitelist is incomplete but since it is only used to improve
>    performance, omitting cases is safe.  */
> #if defined __x86_64__|| defined __amd64__	\
>     || defined __i386__ || defined __i386	\
>     || defined __arm64__ || defined __aarch64__	\
>     || defined __powerpc__ || defined __powerpc	\
>     || defined __ppc__ || defined __ppc		\
>     || defined __s390__ || defined __s390x__
> #define HAVE_FAST_UNALIGNED_ACCESS 1
> #else
> #define HAVE_FAST_UNALIGNED_ACCESS 0
> #endif
> 
> but even if unaligned access is normally permitted by a machine, it is
> still undefined behavior to dereference an unaligned pointer.

This is incorrect.  There's nothing undefined about x86 unaligned
accesses.  C standards can regard this as UB, but we are using
machine-specific knowledge here (and Emacs cannot be built with a
strict adherence to C standards anyway).

> Instead, HAVE_FAST_UNALIGNED_ACCESS and UNALIGNED_LOAD_SIZE should be
> removed and memcpy used instead:
> 
>   word_t a, c;
> 
>   memcpy (&a, w1 + b / ws, sizeof a);
>   memcpy (&c, w2 + b / ws, sizeof c);
> 
> doing so will make the compiler itself generate the right sequence of
> instructions for performing unaligned accesses, normally with only a few
> cycles penalty.

We don't want that penalty here, that's all.

> I would like to install such a change on emacs-29.

No, please don't.

> Emacs currently crashes when built with various compilers performing
> pointer alignment checks.

Details, please.  Which compilers, on what platforms, for what target
architectures, etc.  Unconditionally removing the fast copy there is a
non-starter.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 2483 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: Po Lu <luangruo@yahoo.com>
>> Date: Thu, 30 Mar 2023 11:34:42 +0200
>> 
>> Fstring_lessp has:
>> 
>> /* Check whether the platform allows access to unaligned addresses for
>>    size_t integers without trapping or undue penalty (a few cycles is OK).
>> 
>>    This whitelist is incomplete but since it is only used to improve
>>    performance, omitting cases is safe.  */
>> #if defined __x86_64__|| defined __amd64__	\
>>     || defined __i386__ || defined __i386	\
>>     || defined __arm64__ || defined __aarch64__	\
>>     || defined __powerpc__ || defined __powerpc	\
>>     || defined __ppc__ || defined __ppc		\
>>     || defined __s390__ || defined __s390x__
>> #define HAVE_FAST_UNALIGNED_ACCESS 1
>> #else
>> #define HAVE_FAST_UNALIGNED_ACCESS 0
>> #endif
>> 
>> but even if unaligned access is normally permitted by a machine, it is
>> still undefined behavior to dereference an unaligned pointer.
>
> This is incorrect.  There's nothing undefined about x86 unaligned
> accesses.  C standards can regard this as UB, but we are using
> machine-specific knowledge here (and Emacs cannot be built with a
> strict adherence to C standards anyway).

Things can still go wrong on x86, particularly with SIMD:
https://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html.

>
>> Instead, HAVE_FAST_UNALIGNED_ACCESS and UNALIGNED_LOAD_SIZE should be
>> removed and memcpy used instead:
>> 
>>   word_t a, c;
>> 
>>   memcpy (&a, w1 + b / ws, sizeof a);
>>   memcpy (&c, w2 + b / ws, sizeof c);
>> 
>> doing so will make the compiler itself generate the right sequence of
>> instructions for performing unaligned accesses, normally with only a few
>> cycles penalty.
>
> We don't want that penalty here, that's all.
>
>> I would like to install such a change on emacs-29.
>
> No, please don't.
>
>> Emacs currently crashes when built with various compilers performing
>> pointer alignment checks.
>
> Details, please.  Which compilers, on what platforms, for what target
> architectures, etc.  Unconditionally removing the fast copy there is a
> non-starter.

I imagine they're referring to UBSAN. Emacs may want to add an
annotation where HAVE_FAST_UNALIGNED_ACCESS is used.

But for modern compilers, they will indeed DTRT anyway when they
see memcpy.

See https://github.com/WayneD/rsync/pull/428 for an example.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]

Re: HAVE_FAST_UNALIGNED_ACCESS

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 4978 bytes --]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: Po Lu <luangruo@yahoo.com>
>> Date: Thu, 30 Mar 2023 11:34:42 +0200
>> 
>> Fstring_lessp has:
>> 
>> /* Check whether the platform allows access to unaligned addresses for
>>    size_t integers without trapping or undue penalty (a few cycles is OK).
>> 
>>    This whitelist is incomplete but since it is only used to improve
>>    performance, omitting cases is safe.  */
>> #if defined __x86_64__|| defined __amd64__	\
>>     || defined __i386__ || defined __i386	\
>>     || defined __arm64__ || defined __aarch64__	\
>>     || defined __powerpc__ || defined __powerpc	\
>>     || defined __ppc__ || defined __ppc		\
>>     || defined __s390__ || defined __s390x__
>> #define HAVE_FAST_UNALIGNED_ACCESS 1
>> #else
>> #define HAVE_FAST_UNALIGNED_ACCESS 0
>> #endif
>> 
>> but even if unaligned access is normally permitted by a machine, it is
>> still undefined behavior to dereference an unaligned pointer.
>
> This is incorrect.  There's nothing undefined about x86 unaligned
> accesses.  C standards can regard this as UB, but we are using
> machine-specific knowledge here

You're making a faulty assumption here, there's no guarantee that such
an access happens at all.

You're, of course, right in that an x86 CPU will have no (visible)
qualms about making such a mov, but you're also assuming that the
compiler emits a mov.  This is not guaranteed anywhere, and guaranteeing
so would be terrible for optimization in general.

As an example, the compiler is free to, for instance, vectorize a loop,
emitting instructions that very much have alignment checking even on
x86 (the loop in question is very much parallelizable and vectorizable,
as it feels like a textbook example of such operations).

> (and Emacs cannot be built with a strict adherence to C standards
> anyway).

That is indeed correct; there's, however, a difference in how necessary
it is here (and I argue it is not, with reasoning presented below).

>> Instead, HAVE_FAST_UNALIGNED_ACCESS and UNALIGNED_LOAD_SIZE should be
>> removed and memcpy used instead:
>> 
>>   word_t a, c;
>> 
>>   memcpy (&a, w1 + b / ws, sizeof a);
>>   memcpy (&c, w2 + b / ws, sizeof c);
>> 
>> doing so will make the compiler itself generate the right sequence of
>> instructions for performing unaligned accesses, normally with only a few
>> cycles penalty.
>
> We don't want that penalty here, that's all.

At any optimization level, you don't get one (on x86_64).  I haven't
checked -O0, as it's not worth using (rather, one should use
-O2/-O3/-Og/-Oz).

>> I would like to install such a change on emacs-29.
>
> No, please don't.
>
>> Emacs currently crashes when built with various compilers performing
>> pointer alignment checks.
>
> Details, please.  Which compilers, on what platforms, for what target
> architectures, etc.

Sam presented a decent example (though, sanitizers seem to have been
taken into account in this particular example).

> Unconditionally removing the fast copy there is a non-starter.

You're assuming that alternatives to these "fast" accesses are slow -
they are not.  The following code...

  int
  f_broken (void* x)
  {
      return *((int*)x);
  }
  
  int
  f (void* x)
  {
      int v;
      memcpy (&v, x, sizeof (v));
      return v;
  }

... generates the following code on gcc 12.2.0 with -O1...

  f_broken:
          movl    (%rdi), %eax
          ret
  f:
          movl    (%rdi), %eax
          ret

As a matter of fact, implementing a "skip common prefix" loop with just
chars results in code /shorter/ code on the same compiler (and does not
violate aliasing rules, since the data FAM is a char one).  Some other
portable methods could include Duff's device (using memcpy loads), or
word-size memcmp calls in a loop.

IMO, it is quite a fault in the compiler if Emacs needs to resort to
such hacks (and even if we accept that as something that is our problem,
we should have an abstraction boundary on it).

Note that I did not try hacking Emacs code to benchmark the actual thing
being discussed (as I am not in a position to do so conveniently at the
moment), but I invite you to try that and reconsider removing such code.
Even in the case there is a penalty to this change, I'd argue it is far
better for us to fix that in GCC or implement it a "skip common prefix"
function in Gnulib (so that it's behind a layer of abstraction) rather
than placing this assumption implicitly in this function.

I suspect the least intrusive change possible would emit the same code
as the current implementation, that change being merely using memcpy to
load the words rather than direct dereferences, except in the cases
where the current code is entirely broken, and correct code isn't.

Thanks in advance, have a lovely day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

Re: HAVE_FAST_UNALIGNED_ACCESS

[Robert Pluim]

>>>>> On Fri, 31 Mar 2023 09:06:59 +0800, Po Lu <luangruo@yahoo.com> said:

    Po Lu> Robert, Arsen, did you get my earlier replies to Mattias and Eli?
    Po Lu> They included one example of GCC generating code with alignment
    Po Lu> requirements on x86_64.

    Po Lu> This morning I woke up to messages from the mailer saying that they
    Po Lu> could not be delivered to Eli and the list.  So if you got them, would
    Po Lu> you please forward them to their intended recipients?

I got them, but theyʼve not shown up in gmane. Iʼve forwarded them.

Perhaps itʼs time to change email provider.

Robert
-- 

Re: HAVE_FAST_UNALIGNED_ACCESS

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

Morning Po,

Po Lu <luangruo@yahoo.com> writes:

> Robert, Arsen, did you get my earlier replies to Mattias and Eli?
> They included one example of GCC generating code with alignment
> requirements on x86_64.
>
> This morning I woke up to messages from the mailer saying that they
> could not be delivered to Eli and the list.  So if you got them, would
> you please forward them to their intended recipients?

I've also not received them at all, and don't see them on GNU archives.
I did receive the forward from Robert.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: HAVE_FAST_UNALIGNED_ACCESS

[Mattias Engdegård]

30 mars 2023 kl. 14.18 skrev Arsen Arsenović <arsen@aarsen.me>:

> As an example, the compiler is free to, for instance, vectorize a loop,
> emitting instructions that very much have alignment checking even on
> x86 (the loop in question is very much parallelizable and vectorizable,
> as it feels like a textbook example of such operations).

If only. Then we wouldn't have this problem, would we.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]


Mattias Engdegård <mattiase@acm.org> writes:

> 30 mars 2023 kl. 14.18 skrev Arsen Arsenović <arsen@aarsen.me>:
>
>> As an example, the compiler is free to, for instance, vectorize a loop,
>> emitting instructions that very much have alignment checking even on
>> x86 (the loop in question is very much parallelizable and vectorizable,
>> as it feels like a textbook example of such operations).
>
> If only. Then we wouldn't have this problem, would we.

Free to, not required to ;)

Thanks for pushing your patch.  I'm still unsure if the machine specific
knowledge here helps, but this is an improvement nonetheless.

Have a lovely day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: HAVE_FAST_UNALIGNED_ACCESS

[Mattias Engdegård]

30 mars 2023 kl. 11.34 skrev Robert Pluim <rpluim@gmail.com>:

> but even if unaligned access is normally permitted by a machine, it is
> still undefined behavior to dereference an unaligned pointer.

That's not necessarily a problem; we have plenty of formally undefined behaviour. What's important is whether we can be confident that it works.

> Instead, HAVE_FAST_UNALIGNED_ACCESS and UNALIGNED_LOAD_SIZE should be
> removed and memcpy used instead:

No, this isn't something we should be doing at all unless the platform allows fast unaligned access.

Ideally we shouldn't do any of this nonsense but I haven't worked out how to get the autovectorisers to kick in.

> Emacs currently
> crashes when built with various compilers performing pointer alignment
> checks.

Which compilers, exactly?

Re: HAVE_FAST_UNALIGNED_ACCESS

[Vibhav Pant]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On Thu, 2023-03-30 at 11:34 +0200, Robert Pluim wrote:
> Instead, HAVE_FAST_UNALIGNED_ACCESS and UNALIGNED_LOAD_SIZE should be
> removed and memcpy used instead:
> 
>   word_t a, c;
> 
>   memcpy (&a, w1 + b / ws, sizeof a);
>   memcpy (&c, w2 + b / ws, sizeof c);
> 

I had recently made a few modifications to this on master. There,
Fstring_lessp in uses the macro UNALIGNED_LOAD_SIZE instead, which is
defined to __sanitizer_unaligned_loadXX in lisp.h if:

* We're building with AddressSaniziter,
* <sanitizer/common_interface_defs.h> is available, and
* USE_SANITIZER_UNALIGNED_LOAD is defined.

> I would like to install such a change on emacs-29.  Emacs currently
> crashes when built with various compilers performing pointer
> alignment
> checks.

Instead of removing the code entirely, you could try modifying the
macro definition by getting rid of the AddressSanitizer ifdef, and
building with USE_SANITIZER_UNALIGNED_LOAD.  In theory, this should
make the load not crash with other sanitizers as well. If that works, I
imagine that would be a slightly more acceptable change to install into
emacs-29.

Best,
Vibhav
-- 
Vibhav Pant
vibhavp@gmail.com
GPG: 7ED1 D48C 513C A024 BE3A  785F E3FB 28CB 6AB5 9598

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: HAVE_FAST_UNALIGNED_ACCESS

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

30 mars 2023 kl. 13.38 skrev Vibhav Pant <vibhavp@gmail.com>:

> I had recently made a few modifications to this on master. There,
> Fstring_lessp in uses the macro UNALIGNED_LOAD_SIZE instead, which is
> defined to __sanitizer_unaligned_loadXX in lisp.h if:
> 
> * We're building with AddressSaniziter,
> * <sanitizer/common_interface_defs.h> is available, and
> * USE_SANITIZER_UNALIGNED_LOAD is defined.

Thank you for making this improvement earlier -- I think using memcpy would subsume __sanitizer_unaligned_loadXX, so I'm going with that in my patch.
Another approach would be to disable this poor man's vectorisation when the sanitiser is enabled but that shouldn't be necessary.

The vectorisation is still only made on specific platforms, of course.

Eli, would this patch be acceptable for emacs-29?


[-- Attachment #2: string-lessp-uni-multi.diff --]
[-- Type: application/octet-stream, Size: 942 bytes --]

diff --git a/src/fns.c b/src/fns.c
index f5f222c39c..292d6fc405 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -514,7 +514,23 @@ DEFUN ("string-lessp", Fstring_lessp, Sstring_lessp, 2, 2, 0,
   else
     {
       /* string1 unibyte, string2 multibyte */
-      ptrdiff_t i1 = 0, i2 = 0, i2_byte = 0;
+
+      ptrdiff_t nb1 = n;
+      ptrdiff_t nb2 = SBYTES (string2);
+      ptrdiff_t nb = min (nb1, nb2);
+
+      /* Skip identical ASCII-only prefixes, a word at a time.  */
+      typedef size_t word_t;
+      int ws = sizeof (word_t);
+      word_t msbits = (word_t)0x8080808080808080;
+      const word_t *w1 = (const word_t *) SDATA (string1);
+      const word_t *w2 = (const word_t *) SDATA (string2);
+      ptrdiff_t b = 0;
+      while (b < nb - ws + 1 && w1[b / ws] == w2[b / ws]
+	     && !(w1[b / ws] & msbits))
+	b += ws;
+
+      ptrdiff_t i1 = b, i2 = b, i2_byte = b;
       while (i1 < n)
 	{
 	  int c1 = SREF (string1, i1++);

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 31 Mar 2023 18:57:41 +0200
> Cc: Robert Pluim <rpluim@gmail.com>,
>  Eli Zaretskii <eliz@gnu.org>,
>  emacs-devel <emacs-devel@gnu.org>,
>  Po Lu <luangruo@yahoo.com>
> 
> Eli, would this patch be acceptable for emacs-29?

Sorry, no.  It's too late for such adventures on the release branch.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Mattias Engdegård]

31 mars 2023 kl. 19.59 skrev Eli Zaretskii <eliz@gnu.org>:

>> Eli, would this patch be acceptable for emacs-29?
> 
> Sorry, no.  It's too late for such adventures on the release branch.

That's fine with me; I just got the impression that there was a request for building Emacs with a sanitiser on that branch.
I'll apply it to master.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 31 Mar 2023 20:03:52 +0200
> Cc: vibhavp@gmail.com,
>  rpluim@gmail.com,
>  emacs-devel@gnu.org,
>  luangruo@yahoo.com
> 
> 31 mars 2023 kl. 19.59 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> >> Eli, would this patch be acceptable for emacs-29?
> > 
> > Sorry, no.  It's too late for such adventures on the release branch.
> 
> That's fine with me; I just got the impression that there was a request for building Emacs with a sanitiser on that branch.

People who want to do that can always apply the patch locally.  My
main worry is about those who will build the release tarballs, and
those are unlikely to build with sanitizer.

> I'll apply it to master.

Thanks.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Po Lu]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> That's fine with me; I just got the impression that there was a
> request for building Emacs with a sanitiser on that branch.

> I'll apply it to master.

NO!!

My request is for you to avoid such highly problematic undefined
behavior on either branch.  I posted an example of GCC generating
code with alignment requirements on x86_64, and Robert should have sent
it to you as well.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  vibhavp@gmail.com,  rpluim@gmail.com,
>  emacs-devel@gnu.org
> Date: Sat, 01 Apr 2023 08:45:24 +0800
> 
> Mattias Engdegård <mattias.engdegard@gmail.com> writes:
> 
> > That's fine with me; I just got the impression that there was a
> > request for building Emacs with a sanitiser on that branch.
> 
> > I'll apply it to master.
> 
> NO!!
> 
> My request is for you to avoid such highly problematic undefined
> behavior on either branch.

I've read your request, and decided not to apply this to the release
branch.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> I've read your request, and decided not to apply this to the release
> branch.

Even if GCC 13 (or GCC 14) generates incorrect code for the function?

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: mattias.engdegard@gmail.com,  vibhavp@gmail.com,  rpluim@gmail.com,
>   emacs-devel@gnu.org
> Date: Sat, 01 Apr 2023 14:31:06 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I've read your request, and decided not to apply this to the release
> > branch.
> 
> Even if GCC 13 (or GCC 14) generates incorrect code for the function?

I don't want to make changes on the release branch without a positive
evidence that the existing code causes verifiable problems.  And even
then I prefer to make changes only for platforms where those problems
happen.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]

1 apr. 2023 kl. 08.39 skrev Eli Zaretskii <eliz@gnu.org>:

> I don't want to make changes on the release branch without a positive
> evidence that the existing code causes verifiable problems.  And even
> then I prefer to make changes only for platforms where those problems
> happen.

I'm not overly worried about the code, but to put this matter to rest, what about simply dropping the vectorisation altogether, on all platforms, in Emacs 29? That should be very safe.


[-- Attachment #2: drop-string-lessp-vectorisation.diff --]
[-- Type: application/octet-stream, Size: 1818 bytes --]

diff --git a/src/fns.c b/src/fns.c
index ff364c65e26..95c4d2c028c 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -438,22 +438,6 @@ DEFUN ("compare-strings", Fcompare_strings, Scompare_strings, 6, 7, 0,
   return Qt;
 }
 
-/* Check whether the platform allows access to unaligned addresses for
-   size_t integers without trapping or undue penalty (a few cycles is OK).
-
-   This whitelist is incomplete but since it is only used to improve
-   performance, omitting cases is safe.  */
-#if defined __x86_64__|| defined __amd64__	\
-    || defined __i386__ || defined __i386	\
-    || defined __arm64__ || defined __aarch64__	\
-    || defined __powerpc__ || defined __powerpc	\
-    || defined __ppc__ || defined __ppc		\
-    || defined __s390__ || defined __s390x__
-#define HAVE_FAST_UNALIGNED_ACCESS 1
-#else
-#define HAVE_FAST_UNALIGNED_ACCESS 0
-#endif
-
 DEFUN ("string-lessp", Fstring_lessp, Sstring_lessp, 2, 2, 0,
        doc: /* Return non-nil if STRING1 is less than STRING2 in lexicographic order.
 Case is significant.
@@ -491,20 +475,6 @@ DEFUN ("string-lessp", Fstring_lessp, Sstring_lessp, 2, 2, 0,
       ptrdiff_t nb = min (nb1, nb2);
       ptrdiff_t b = 0;
 
-      /* String data is normally allocated with word alignment, but
-	 there are exceptions (notably pure strings) so we restrict the
-	 wordwise skipping to safe architectures.  */
-      if (HAVE_FAST_UNALIGNED_ACCESS)
-	{
-	  /* First compare entire machine words.  */
-	  typedef size_t word_t;
-	  int ws = sizeof (word_t);
-	  const word_t *w1 = (const word_t *) SDATA (string1);
-	  const word_t *w2 = (const word_t *) SDATA (string2);
-	  while (b < nb - ws + 1 && w1[b / ws] == w2[b / ws])
-	    b += ws;
-	}
-
       /* Scan forward to the differing byte.  */
       while (b < nb && SREF (string1, b) == SREF (string2, b))
 	b++;

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sat, 1 Apr 2023 09:42:53 +0200
> Cc: Po Lu <luangruo@yahoo.com>,
>  vibhavp@gmail.com,
>  rpluim@gmail.com,
>  emacs-devel@gnu.org
> 
> 1 apr. 2023 kl. 08.39 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I don't want to make changes on the release branch without a positive
> > evidence that the existing code causes verifiable problems.  And even
> > then I prefer to make changes only for platforms where those problems
> > happen.
> 
> I'm not overly worried about the code, but to put this matter to rest, what about simply dropping the vectorisation altogether, on all platforms, in Emacs 29? That should be very safe.

From my POV, the safest code is the one we have, because that have
lived for many moons and have been used by many people, with no
problems reported so far.  Which is why I said above that I want to
see verifiable problems with the existing code, and understand those
problems well enough, before I will agree to such non-trivial changes
on the release branch

IOW, "best engineering practices" are for master; on the release
branch, using existing code proven by time takes precedence, because
our ability to predict consequences is limited.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> IOW, "best engineering practices" are for master; on the release
> branch, using existing code proven by time takes precedence, because
> our ability to predict consequences is limited.

This problem is not about engineering practices, but basic program
correctness.  Look at the GCC bug tracker: every release, a program
that relies on this undefined behavior becomes subtly broken, a bug
is filed against GCC, and is closed by the GCC developers, stating
that this behavior is unsupported.

Emacs 29 has been in development for less than three years... not nearly
long enough to be sure no subtle miscompilations have taken (or will
take) place.

If that's ``proven by time'', then so is this:

null_terminate (buffer, size)
  char *buffer;
{
  buffer[size] = '\0';
}

It might work for a few months, or a year, then suddenly break with a
new compiler release, or perhaps a change to malloc, or something else.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Mattias Engdegård <mattias.engdegard@gmail.com>,
>   vibhavp@gmail.com,
>   rpluim@gmail.com,  emacs-devel@gnu.org
> Date: Sat, 01 Apr 2023 17:17:07 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > IOW, "best engineering practices" are for master; on the release
> > branch, using existing code proven by time takes precedence, because
> > our ability to predict consequences is limited.
> 
> This problem is not about engineering practices, but basic program
> correctness.  Look at the GCC bug tracker: every release, a program
> that relies on this undefined behavior becomes subtly broken, a bug
> is filed against GCC, and is closed by the GCC developers, stating
> that this behavior is unsupported.
> 
> Emacs 29 has been in development for less than three years... not nearly
> long enough to be sure no subtle miscompilations have taken (or will
> take) place.
> 
> If that's ``proven by time'', then so is this:
> 
> null_terminate (buffer, size)
>   char *buffer;
> {
>   buffer[size] = '\0';
> }
> 
> It might work for a few months, or a year, then suddenly break with a
> new compiler release, or perhaps a change to malloc, or something else.

I'm still unconvinced, and I said already what will have a chance of
convincing me: a specific report about a problem this particular code
causes on a specific existing platform we support in Emacs 29 and with
a specific compiler.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Arsen Arsenović]

[-- Attachment #1.1: Type: text/plain, Size: 733 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

> I'm still unconvinced, and I said already what will have a chance of
> convincing me: a specific report about a problem this particular code
> causes on a specific existing platform we support in Emacs 29 and with
> a specific compiler.

Similar (but not exactly the same) loops as this one have been shown to
generate incorrect code in this thread.  It's not a large leap for it to
happen to this one, introducing subtle errors for a bit of code that is
completely unnecessary (as demonstrated by it being optional),
especially at higher optimization levels, where the compiler could
easily produce better code than the assumption of a 'mov' would.

Is the following trivial enough for 29?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: [PATCH] Remove aliasing violation in Fstring_lessp --]
[-- Type: text/x-patch, Size: 1226 bytes --]

From 96d75e78358d6c2643bfb7cc65744b8a6178c9d2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
Date: Sat, 1 Apr 2023 14:25:12 +0200
Subject: [PATCH] Remove aliasing violation in Fstring_lessp

* src/fns.c (Fstring_lessp) <HAVE_FAST_UNALIGNED_ACCESS>: Remove
strict aliasing violation.
---
 src/fns.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index ff364c6..e3e11e2 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -499,10 +499,16 @@ DEFUN ("string-lessp", Fstring_lessp, Sstring_lessp, 2, 2, 0,
 	  /* First compare entire machine words.  */
 	  typedef size_t word_t;
 	  int ws = sizeof (word_t);
-	  const word_t *w1 = (const word_t *) SDATA (string1);
-	  const word_t *w2 = (const word_t *) SDATA (string2);
-	  while (b < nb - ws + 1 && w1[b / ws] == w2[b / ws])
-	    b += ws;
+	  while (b < nb - ws + 1)
+	    {
+	      word_t w1;
+	      word_t w2;
+	      memcpy (&w1, SDATA (string1) + b, sizeof (w1));
+	      memcpy (&w2, SDATA (string2) + b, sizeof (w2));
+	      if (w1 != w2)
+		break;
+	      b += ws;
+	    }
 	}
 
       /* Scan forward to the differing byte.  */
-- 
2.40.0


[-- Attachment #1.3: Type: text/plain, Size: 492 bytes --]


.. or something similar to it, assuming I made an error, which is likely
given the circumstances.  This does pass the testsuite, anyway.  It
should just expand deferences into explicit memcpys.

No actual memcpy calls are produced, and this is at least functional on
a superset of compilers, and I suspect replacing the whole thing with a
naive-looking while (*(w1++) != *(w2++)); loop would be even better (but
I can settle for that being too experimental).
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Arsen Arsenović <arsen@aarsen.me>
> Cc: Po Lu <luangruo@yahoo.com>, mattias.engdegard@gmail.com,
>  vibhavp@gmail.com, rpluim@gmail.com, emacs-devel@gnu.org
> Date: Sat, 01 Apr 2023 14:59:53 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm still unconvinced, and I said already what will have a chance of
> > convincing me: a specific report about a problem this particular code
> > causes on a specific existing platform we support in Emacs 29 and with
> > a specific compiler.
> 
> Similar (but not exactly the same) loops as this one have been shown to
> generate incorrect code in this thread.  It's not a large leap for it to
> happen to this one, introducing subtle errors for a bit of code that is
> completely unnecessary (as demonstrated by it being optional),
> especially at higher optimization levels, where the compiler could
> easily produce better code than the assumption of a 'mov' would.
> 
> Is the following trivial enough for 29?

You are again trying to push for a change without showing any actual
bug with the existing code.  Please humor me, and please show me an
actual bug due to the existing code before suggesting a solution.  See
above for the description of the details I'd like to know about such
actual bug.

> .. or something similar to it, assuming I made an error, which is likely
> given the circumstances.  This does pass the testsuite, anyway.  It
> should just expand deferences into explicit memcpys.
> 
> No actual memcpy calls are produced, and this is at least functional on
> a superset of compilers, and I suspect replacing the whole thing with a
> naive-looking while (*(w1++) != *(w2++)); loop would be even better (but
> I can settle for that being too experimental).

Sorry, I don't want to risk any errors, and I would like to avoid any
experiments with the release branch.  Which is why I'm asking for hard
evidence.  It isn't that I don't understand what you and others are
saying, or don't believe you.  It's just that we need to see the
problems before we can judge the solutions that must be safe on this
branch.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

> You are again trying to push for a change without showing any actual
> bug with the existing code.  Please humor me, and please show me an
> actual bug due to the existing code before suggesting a solution.  See
> above for the description of the details I'd like to know about such
> actual bug.

For a somewhat contrived example, UBsan flags this code (and, when
running the testsuite, it'd seem that it flags only this code).

  ~/gnu/emacs-29/_build 2 $ gcc --version
  gcc (Gentoo Hardened 13.0.1_pre20230326-r1 p9) 13.0.1 20230326 (experimental)
  Copyright (C) 2023 Free Software Foundation, Inc.
  This is free software; see the source for copying conditions.  There is NO
  warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
  
  ~/gnu/emacs-29/_build$ git rev-parse HEAD
  b39c3cd1125590bf4b77880b41ac08b29cdfcff6
  ~/gnu/emacs-29/_build$ gcc -dumpmachine
  x86_64-pc-linux-gnu

I can't speak to Po's example, but IIUC, he ran into a problem somewhere
(maybe the same example?).

>> .. or something similar to it, assuming I made an error, which is likely
>> given the circumstances.  This does pass the testsuite, anyway.  It
>> should just expand deferences into explicit memcpys.
>> 
>> No actual memcpy calls are produced, and this is at least functional on
>> a superset of compilers, and I suspect replacing the whole thing with a
>> naive-looking while (*(w1++) != *(w2++)); loop would be even better (but
>> I can settle for that being too experimental).
>
> Sorry, I don't want to risk any errors, and I would like to avoid any
> experiments with the release branch.  Which is why I'm asking for hard
> evidence.  It isn't that I don't understand what you and others are
> saying, or don't believe you.  It's just that we need to see the
> problems before we can judge the solutions that must be safe on this
> branch.

I was writing the patch for demonstration purposes more than for actual
application.

I understand why you're arguing this, and I have to thank you for it -
such efforts keep Emacs as stable as it is - but this feels like an
example that is too trivial to apply such judgment to, hence my
position.  I am certain that you have an understanding of the issue at
hand, I'm only trying to provide a solution that's safe enough for 29.

Have a lovely day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: HAVE_FAST_UNALIGNED_ACCESS

[Eli Zaretskii]

> From: Arsen Arsenović <arsen@aarsen.me>
> Cc: luangruo@yahoo.com, mattias.engdegard@gmail.com, vibhavp@gmail.com,
>  rpluim@gmail.com, emacs-devel@gnu.org
> Date: Sat, 01 Apr 2023 17:22:48 +0200
> 
> I understand why you're arguing this, and I have to thank you for it -
> such efforts keep Emacs as stable as it is - but this feels like an
> example that is too trivial to apply such judgment to, hence my
> position.  I am certain that you have an understanding of the issue at
> hand, I'm only trying to provide a solution that's safe enough for 29.

Among other reasons, we'd need a real-life case where this code fails
to test any solutions.

Re: HAVE_FAST_UNALIGNED_ACCESS

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Arsen Arsenović <arsen@aarsen.me>
>> Cc: luangruo@yahoo.com, mattias.engdegard@gmail.com, vibhavp@gmail.com,
>>  rpluim@gmail.com, emacs-devel@gnu.org
>> Date: Sat, 01 Apr 2023 17:22:48 +0200
>> 
>> I understand why you're arguing this, and I have to thank you for it -
>> such efforts keep Emacs as stable as it is - but this feels like an
>> example that is too trivial to apply such judgment to, hence my
>> position.  I am certain that you have an understanding of the issue at
>> hand, I'm only trying to provide a solution that's safe enough for 29.
>
> Among other reasons, we'd need a real-life case where this code fails
> to test any solutions.

-fsanitize=undefined is intended to detect these ``real-life cases''
before they occur.

We are talking about code that is less than three years old, in the
context of a procedure that has not been changed much since the days of
the Unicode branch!  How can you be so certain of its safety?

Re: HAVE_FAST_UNALIGNED_ACCESS

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Arsen Arsenović <arsen@aarsen.me>
>> Cc: Po Lu <luangruo@yahoo.com>, mattias.engdegard@gmail.com,
>>  vibhavp@gmail.com, rpluim@gmail.com, emacs-devel@gnu.org
>> Date: Sat, 01 Apr 2023 14:59:53 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I'm still unconvinced, and I said already what will have a chance of
>> > convincing me: a specific report about a problem this particular code
>> > causes on a specific existing platform we support in Emacs 29 and with
>> > a specific compiler.
>> 
>> Similar (but not exactly the same) loops as this one have been shown to
>> generate incorrect code in this thread.  It's not a large leap for it to
>> happen to this one, introducing subtle errors for a bit of code that is
>> completely unnecessary (as demonstrated by it being optional),
>> especially at higher optimization levels, where the compiler could
>> easily produce better code than the assumption of a 'mov' would.
>> 
>> Is the following trivial enough for 29?
>
> You are again trying to push for a change without showing any actual
> bug with the existing code.  Please humor me, and please show me an
> actual bug due to the existing code before suggesting a solution.  See
> above for the description of the details I'd like to know about such
> actual bug.

> Sorry, I don't want to risk any errors, and I would like to avoid any
> experiments with the release branch.  Which is why I'm asking for hard
> evidence.  It isn't that I don't understand what you and others are
> saying, or don't believe you.  It's just that we need to see the
> problems before we can judge the solutions that must be safe on this
> branch.

The nature of this bug is that there is no ``hard evidence'' of its
presence, until it strikes.  Just like writing outside malloc'ed memory,
or freeing a pointer twice, or signed integer overflow (which falls
apart entirely on the R4000.)

This code has a very high risk for errors.
Removing it entirely will reduce the risk, since it was only installed
late in Emacs 29's development.