bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Ahmed Khanzada]

I'm not an expert on C, OpenBSD, or SPARC64, but I did notice
emacs-current was not compiling.

During a gmake, bootstrap-emacs would get a SIGBUS and fail. Running
gdb led me to the hash_string function where it seemed to be failing.

Reverting the hash_string function to a previous version seems to have
fixed the issue.

If I had to guess, it may have something to do with SPARC64 alignment.

I don't have an updated AMD64 OpenBSD box running at the moment, but if you
would like me to test there, I could find a way.

I understand that reverting this function might not be the best way
forward, but wrote a patch just in case that reverts hash_string to an
earlier version that compiles on OpenBSD/SPARC64.

I am already signed all my GNU contributor docs.

--[[text/plain; type=patch; name="0001-Reverting-hash_string-function-due-to-problem-on-Ope.patch"
Content-Disposition: attachment; filename="0001-Reverting-hasattach filh_string-function-due-to-problem-on-Ope.patch"][base64]]
RnJvbSBkOTM4NzQ1Y2U1NjA1OGJlMjQwNzhlYzUyZDk3Mzk4Njk3ZGI0Yjc2IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBBaG1lZCBLaGFuemFkYSA8bWVAZW56dS5ydT4KRGF0ZTogTW9u
LCAyNSBKYW4gMjAyMSAyMToxNjo0OCAtMDgwMApTdWJqZWN0OiBbUEFUQ0hdIFJldmVydGluZyBo
YXNoX3N0cmluZyBmdW5jdGlvbiBkdWUgdG8gcHJvYmxlbSBvbgogT3BlbkJTRC9TUEFSQzY0Cgot
LS0KIHNyYy9mbnMuYyB8IDMyICsrKysrKystLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCiAxIGZp
bGUgY2hhbmdlZCwgNyBpbnNlcnRpb25zKCspLCAyNSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQg
YS9zcmMvZm5zLmMgYi9zcmMvZm5zLmMKaW5kZXggN2FiMmU4ZjFhMC4uMDEyN2ZlM2Y0OCAxMDA2
NDQKLS0tIGEvc3JjL2Zucy5jCisrKyBiL3NyYy9mbnMuYwpAQCAtNDU5OSwzNCArNDU5OSwxNiBA
QCAjZGVmaW5lIFNYSEFTSF9NQVhfTEVOICAgNwogRU1BQ1NfVUlOVAogaGFzaF9zdHJpbmcgKGNo
YXIgY29uc3QgKnB0ciwgcHRyZGlmZl90IGxlbikKIHsKLSAgRU1BQ1NfVUlOVCBjb25zdCAqcCAg
ID0gKEVNQUNTX1VJTlQgY29uc3QgKikgcHRyOwotICBFTUFDU19VSU5UIGNvbnN0ICplbmQgPSAo
RU1BQ1NfVUlOVCBjb25zdCAqKSAocHRyICsgbGVuKTsKLSAgRU1BQ1NfVUlOVCBoYXNoID0gbGVu
OwotICAvKiBBdCBtb3N0IDggc3RlcHMuICBXZSBjb3VsZCByZXVzZSBTWEhBU0hfTUFYX0xFTiwg
b2YgY291cnNlLAotICAgKiBidXQgZGl2aWRpbmcgYnkgOCBpcyBjaGVhcGVyLiAgKi8KLSAgcHRy
ZGlmZl90IHN0ZXAgPSAxICsgKChlbmQgLSBwKSA+PiAzKTsKLQotICAvKiBCZXdhcmU6IGBlbmRg
IG1pZ2h0IGJlIHVuYWxpZ25lZCwgc28gYHAgPCBlbmRgIGlzIG5vdCBhbHdheXMgdGhlIHNhbWUK
LSAgICogYXMgYHAgPD0gZW5kIC0gMWAuICAqLwotICB3aGlsZSAocCA8PSBlbmQgLSAxKQorICBj
aGFyIGNvbnN0ICpwID0gcHRyOworICBjaGFyIGNvbnN0ICplbmQgPSBwICsgbGVuOworICB1bnNp
Z25lZCBjaGFyIGM7CisgIEVNQUNTX1VJTlQgaGFzaCA9IDA7CisKKyAgd2hpbGUgKHAgIT0gZW5k
KQogICAgIHsKLSAgICAgIEVNQUNTX1VJTlQgYyA9ICpwOwotICAgICAgcCArPSBzdGVwOworICAg
ICAgYyA9ICpwKys7CiAgICAgICBoYXNoID0gc3hoYXNoX2NvbWJpbmUgKGhhc2gsIGMpOwogICAg
IH0KLSAgaWYgKHAgPCBlbmQpCi0gICAgeyAvKiBBIGZldyBsYXN0IGJ5dGVzIHJlbWFpbiAoc21h
bGxlciB0aGFuIGFuIEVNQUNTX1VJTlQpLiAgKi8KLSAgICAgIC8qIEZJWE1FOiBXZSBjb3VsZCBk
byB0aGlzIHdpdGhvdXQgYSBsb29wLCBidXQgaXQnZCByZXF1aXJlCi0gICAgICAgICBlbmRpYW4t
ZGVwZW5kZW50IGNvZGUgOi0oICAqLwotICAgICAgY2hhciBjb25zdCAqcDEgPSAoY2hhciBjb25z
dCAqKXA7Ci0gICAgICBjaGFyIGNvbnN0ICplbmQxID0gKGNoYXIgY29uc3QgKillbmQ7Ci0gICAg
ICBkbwotICAgICAgICB7Ci0gICAgICAgICAgdW5zaWduZWQgY2hhciBjID0gKnAxKys7Ci0gICAg
ICAgICAgaGFzaCA9IHN4aGFzaF9jb21iaW5lIChoYXNoLCBjKTsKLSAgICAgICAgfQotICAgICAg
d2hpbGUgKHAxIDwgZW5kMSk7Ci0gICAgfQogCiAgIHJldHVybiBoYXNoOwogfQotLSAKMi4yOC4w
Cgo=

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Ahmed Khanzada]

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

Not sure if the patch attached correctly. Trying again.

On Mon, Jan 25, 2021 at 10:37 PM Ahmed Khanzada <me@enzu.ru> wrote:
>
> I'm not an expert on C, OpenBSD, or SPARC64, but I did notice
> emacs-current was not compiling.
>
> During a gmake, bootstrap-emacs would get a SIGBUS and fail. Running
> gdb led me to the hash_string function where it seemed to be failing.
>
> Reverting the hash_string function to a previous version seems to have
> fixed the issue.
>
> If I had to guess, it may have something to do with SPARC64 alignment.
>
> I don't have an updated AMD64 OpenBSD box running at the moment, but if you
> would like me to test there, I could find a way.
>
> I understand that reverting this function might not be the best way
> forward, but wrote a patch just in case that reverts hash_string to an
> earlier version that compiles on OpenBSD/SPARC64.
>
> I am already signed all my GNU contributor docs.
>
> --[[text/plain; type=patch; name="0001-Reverting-hash_string-function-due-to-problem-on-Ope.patch"
> Content-Disposition: attachment; filename="0001-Reverting-hasattach filh_string-function-due-to-problem-on-Ope.patch"][base64]]
> RnJvbSBkOTM4NzQ1Y2U1NjA1OGJlMjQwNzhlYzUyZDk3Mzk4Njk3ZGI0Yjc2IE1vbiBTZXAgMTcg
> MDA6MDA6MDAgMjAwMQpGcm9tOiBBaG1lZCBLaGFuemFkYSA8bWVAZW56dS5ydT4KRGF0ZTogTW9u
> LCAyNSBKYW4gMjAyMSAyMToxNjo0OCAtMDgwMApTdWJqZWN0OiBbUEFUQ0hdIFJldmVydGluZyBo
> YXNoX3N0cmluZyBmdW5jdGlvbiBkdWUgdG8gcHJvYmxlbSBvbgogT3BlbkJTRC9TUEFSQzY0Cgot
> LS0KIHNyYy9mbnMuYyB8IDMyICsrKysrKystLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCiAxIGZp
> bGUgY2hhbmdlZCwgNyBpbnNlcnRpb25zKCspLCAyNSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQg
> YS9zcmMvZm5zLmMgYi9zcmMvZm5zLmMKaW5kZXggN2FiMmU4ZjFhMC4uMDEyN2ZlM2Y0OCAxMDA2
> NDQKLS0tIGEvc3JjL2Zucy5jCisrKyBiL3NyYy9mbnMuYwpAQCAtNDU5OSwzNCArNDU5OSwxNiBA
> QCAjZGVmaW5lIFNYSEFTSF9NQVhfTEVOICAgNwogRU1BQ1NfVUlOVAogaGFzaF9zdHJpbmcgKGNo
> YXIgY29uc3QgKnB0ciwgcHRyZGlmZl90IGxlbikKIHsKLSAgRU1BQ1NfVUlOVCBjb25zdCAqcCAg
> ID0gKEVNQUNTX1VJTlQgY29uc3QgKikgcHRyOwotICBFTUFDU19VSU5UIGNvbnN0ICplbmQgPSAo
> RU1BQ1NfVUlOVCBjb25zdCAqKSAocHRyICsgbGVuKTsKLSAgRU1BQ1NfVUlOVCBoYXNoID0gbGVu
> OwotICAvKiBBdCBtb3N0IDggc3RlcHMuICBXZSBjb3VsZCByZXVzZSBTWEhBU0hfTUFYX0xFTiwg
> b2YgY291cnNlLAotICAgKiBidXQgZGl2aWRpbmcgYnkgOCBpcyBjaGVhcGVyLiAgKi8KLSAgcHRy
> ZGlmZl90IHN0ZXAgPSAxICsgKChlbmQgLSBwKSA+PiAzKTsKLQotICAvKiBCZXdhcmU6IGBlbmRg
> IG1pZ2h0IGJlIHVuYWxpZ25lZCwgc28gYHAgPCBlbmRgIGlzIG5vdCBhbHdheXMgdGhlIHNhbWUK
> LSAgICogYXMgYHAgPD0gZW5kIC0gMWAuICAqLwotICB3aGlsZSAocCA8PSBlbmQgLSAxKQorICBj
> aGFyIGNvbnN0ICpwID0gcHRyOworICBjaGFyIGNvbnN0ICplbmQgPSBwICsgbGVuOworICB1bnNp
> Z25lZCBjaGFyIGM7CisgIEVNQUNTX1VJTlQgaGFzaCA9IDA7CisKKyAgd2hpbGUgKHAgIT0gZW5k
> KQogICAgIHsKLSAgICAgIEVNQUNTX1VJTlQgYyA9ICpwOwotICAgICAgcCArPSBzdGVwOworICAg
> ICAgYyA9ICpwKys7CiAgICAgICBoYXNoID0gc3hoYXNoX2NvbWJpbmUgKGhhc2gsIGMpOwogICAg
> IH0KLSAgaWYgKHAgPCBlbmQpCi0gICAgeyAvKiBBIGZldyBsYXN0IGJ5dGVzIHJlbWFpbiAoc21h
> bGxlciB0aGFuIGFuIEVNQUNTX1VJTlQpLiAgKi8KLSAgICAgIC8qIEZJWE1FOiBXZSBjb3VsZCBk
> byB0aGlzIHdpdGhvdXQgYSBsb29wLCBidXQgaXQnZCByZXF1aXJlCi0gICAgICAgICBlbmRpYW4t
> ZGVwZW5kZW50IGNvZGUgOi0oICAqLwotICAgICAgY2hhciBjb25zdCAqcDEgPSAoY2hhciBjb25z
> dCAqKXA7Ci0gICAgICBjaGFyIGNvbnN0ICplbmQxID0gKGNoYXIgY29uc3QgKillbmQ7Ci0gICAg
> ICBkbwotICAgICAgICB7Ci0gICAgICAgICAgdW5zaWduZWQgY2hhciBjID0gKnAxKys7Ci0gICAg
> ICAgICAgaGFzaCA9IHN4aGFzaF9jb21iaW5lIChoYXNoLCBjKTsKLSAgICAgICAgfQotICAgICAg
> d2hpbGUgKHAxIDwgZW5kMSk7Ci0gICAgfQogCiAgIHJldHVybiBoYXNoOwogfQotLSAKMi4yOC4w
> Cgo=

[-- Attachment #2: 0001-Reverting-hash_string-function-due-to-problem-on-Ope.patch --]
[-- Type: application/octet-stream, Size: 1655 bytes --]

From d938745ce56058be24078ec52d97398697db4b76 Mon Sep 17 00:00:00 2001
From: Ahmed Khanzada <me@enzu.ru>
Date: Mon, 25 Jan 2021 21:16:48 -0800
Subject: [PATCH] Reverting hash_string function due to problem on
 OpenBSD/SPARC64

---
 src/fns.c | 32 +++++++-------------------------
 1 file changed, 7 insertions(+), 25 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 7ab2e8f1a0..0127fe3f48 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4599,34 +4599,16 @@ #define SXHASH_MAX_LEN   7
 EMACS_UINT
 hash_string (char const *ptr, ptrdiff_t len)
 {
-  EMACS_UINT const *p   = (EMACS_UINT const *) ptr;
-  EMACS_UINT const *end = (EMACS_UINT const *) (ptr + len);
-  EMACS_UINT hash = len;
-  /* At most 8 steps.  We could reuse SXHASH_MAX_LEN, of course,
-   * but dividing by 8 is cheaper.  */
-  ptrdiff_t step = 1 + ((end - p) >> 3);
-
-  /* Beware: `end` might be unaligned, so `p < end` is not always the same
-   * as `p <= end - 1`.  */
-  while (p <= end - 1)
+  char const *p = ptr;
+  char const *end = p + len;
+  unsigned char c;
+  EMACS_UINT hash = 0;
+
+  while (p != end)
     {
-      EMACS_UINT c = *p;
-      p += step;
+      c = *p++;
       hash = sxhash_combine (hash, c);
     }
-  if (p < end)
-    { /* A few last bytes remain (smaller than an EMACS_UINT).  */
-      /* FIXME: We could do this without a loop, but it'd require
-         endian-dependent code :-(  */
-      char const *p1 = (char const *)p;
-      char const *end1 = (char const *)end;
-      do
-        {
-          unsigned char c = *p1++;
-          hash = sxhash_combine (hash, c);
-        }
-      while (p1 < end1);
-    }
 
   return hash;
 }
-- 
2.28.0

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Eli Zaretskii]

On January 26, 2021 8:57:18 AM GMT+02:00, Ahmed Khanzada <me@enzu.ru> wrote:
> Not sure if the patch attached correctly. Trying again.


Thanks.  However, could you please show the C-level backtrace from the SIGBUS crash, as displayed by GDB?  I think we'd like to know which string has its data unaligned to cause this.

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Ahmed Khanzada]

Eli Zaretskii <eliz@gnu.org> writes:

> On January 26, 2021 8:57:18 AM GMT+02:00, Ahmed Khanzada <me@enzu.ru> wrote:
>> Not sure if the patch attached correctly. Trying again.
>
>
> Thanks.  However, could you please show the C-level backtrace from the SIGBUS crash, as displayed by GDB?  I think we'd like to know which string has its data unaligned to cause this.

Is the log below the information that you are looking for?

Starting program: /home/enzuru/src/emacs/src/bootstrap-emacs

Breakpoint 1, hash_string (ptr=0x47fa34d596 "DndProtocol", len=11) at
fns.c:4602
4602      EMACS_UINT const *p   = (EMACS_UINT const *) ptr;
(gdb) info args
ptr = 0x47fa34d596 "DndProtocol"
len = 11
(gdb) next
4603      EMACS_UINT const *end = (EMACS_UINT const *) (ptr + len);
(gdb) next
4604      EMACS_UINT hash = len;
(gdb) next
4607      ptrdiff_t step = 1 + ((end - p) >> 3);
(gdb) next
4611      while (p <= end - 1)
(gdb) next
4613          EMACS_UINT c = *p;
(gdb) next

Program received signal SIGBUS, Bus error.
0x000000455fe1dc6c in hash_string (ptr=0x47fa34d596 "DndProtocol",
len=11) at fns.c:4613
4613          EMACS_UINT c = *p;
(gdb) backtrace
#0  0x000000455fe1dc6c in hash_string (ptr=0x47fa34d596 "DndProtocol",
len=11) at fns.c:4613
#1  0x000000455fe1dd48 in sxhash_string (ptr=0x47fa34d596 "DndProtocol",
len=11) at fns.c:4640
#2  0x000000455fe1e36c in sxhash_obj (obj=0x47fa02f0bc, depth=0) at
fns.c:4759
#3  0x000000455fe1e270 in sxhash (obj=0x47fa02f0bc) at fns.c:4741
#4  0x000000455fe1c52c in hashfn_equal (key=0x47fa02f0bc,
h=0x47fa02eff0) at fns.c:4096
#5  0x000000455fe1cf44 in hash_table_rehash (hash=0x47fa02eff5) at
fns.c:4342
#6  0x000000455fdc5264 in hash_table_thaw (hash=0x47fa02eff5) at
pdumper.c:2652
#7  0x000000455fdcd184 in thaw_hash_tables () at pdumper.c:5477
#8  0x000000455fdcccf0 in pdumper_load (dump_filename=0x4832495d00
"/home/enzuru/src/emacs/src/bootstrap-emacs.pdmp") at pdumper.c:5405
#9  0x000000455fcea4ac in load_pdump (argc=1, argv=0xffffffffffff2968)
at emacs.c:859
#10 0x000000455fceabec in main (argc=1, argv=0xffffffffffff2968) at
emacs.c:1067

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Eli Zaretskii]

> From: Ahmed Khanzada <me@enzu.ru>
> Cc: 
> Date: Wed, 27 Jan 2021 20:06:01 -0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks.  However, could you please show the C-level backtrace from the SIGBUS crash, as displayed by GDB?  I think we'd like to know which string has its data unaligned to cause this.
> 
> Is the log below the information that you are looking for?

Yes, thanks.

> 4613          EMACS_UINT c = *p;
> (gdb) next
> 
> Program received signal SIGBUS, Bus error.
> 0x000000455fe1dc6c in hash_string (ptr=0x47fa34d596 "DndProtocol",
> len=11) at fns.c:4613
> 4613          EMACS_UINT c = *p;
> (gdb) backtrace
> #0  0x000000455fe1dc6c in hash_string (ptr=0x47fa34d596 "DndProtocol",
> len=11) at fns.c:4613
> #1  0x000000455fe1dd48 in sxhash_string (ptr=0x47fa34d596 "DndProtocol",
> len=11) at fns.c:4640
> #2  0x000000455fe1e36c in sxhash_obj (obj=0x47fa02f0bc, depth=0) at
> fns.c:4759
> #3  0x000000455fe1e270 in sxhash (obj=0x47fa02f0bc) at fns.c:4741
> #4  0x000000455fe1c52c in hashfn_equal (key=0x47fa02f0bc,
> h=0x47fa02eff0) at fns.c:4096
> #5  0x000000455fe1cf44 in hash_table_rehash (hash=0x47fa02eff5) at
> fns.c:4342
> #6  0x000000455fdc5264 in hash_table_thaw (hash=0x47fa02eff5) at
> pdumper.c:2652
> #7  0x000000455fdcd184 in thaw_hash_tables () at pdumper.c:5477

Stefan, any ideas?

One possibility is to use memcpy instead of dereferencing a pointer.
We could do that either unconditionally or when the original pointer
is not aligned on 4/8-byte boundary.

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Stefan Monnier]

> Starting program: /home/enzuru/src/emacs/src/bootstrap-emacs
>
> Breakpoint 1, hash_string (ptr=0x47fa34d596 "DndProtocol", len=11) at
> fns.c:4602
> 4602      EMACS_UINT const *p   = (EMACS_UINT const *) ptr;
> (gdb) info args
> ptr = 0x47fa34d596 "DndProtocol"
> len = 11
> (gdb) next
> 4603      EMACS_UINT const *end = (EMACS_UINT const *) (ptr + len);
> (gdb) next
> 4604      EMACS_UINT hash = len;
> (gdb) next
> 4607      ptrdiff_t step = 1 + ((end - p) >> 3);
> (gdb) next
> 4611      while (p <= end - 1)
> (gdb) next
> 4613          EMACS_UINT c = *p;
> (gdb) next
>
> Program received signal SIGBUS, Bus error.

Hmm... so it's doing a dereference at address 0x47fa34d596 and getting
a bus error?

I have two questions here:

- I'd guess that the bus error is due to alignment restrictions.
  What hardware is this running on?  Last I checked, the computer
  architecture community had agreed (many years ago already) that
  (except for very small CPUs maybe, those not able to run Emacs) it's
  better to have the hardware support unaligned memory accesses (it took
  more time to get there than the consensus on branch delay slots,
  admittedly), so I'd be curious if there is still moderately recent
  hardware that insists on signaling an error.

- AFAICT from the backtrace, `ptr` points to a plain normal ELisp
  string's content, yet these are supposed to be aligned, so what's
  going on here (this question is not directed at you ;-)


        Stefan

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: eliz@gnu.org,  46111@debbugs.gnu.org
> Date: Thu, 28 Jan 2021 10:09:21 -0500
> 
> Hmm... so it's doing a dereference at address 0x47fa34d596 and getting
> a bus error?

Yes.

> I have two questions here:
> 
> - I'd guess that the bus error is due to alignment restrictions.
>   What hardware is this running on?

See the Subject: it's SPARC64.

> - AFAICT from the backtrace, `ptr` points to a plain normal ELisp
>   string's content, yet these are supposed to be aligned, so what's
>   going on here

I wondered that myself.

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Stefan Monnier]

>> - I'd guess that the bus error is due to alignment restrictions.
>>   What hardware is this running on?
> See the Subject: it's SPARC64.

I mean the actual hardware, not the architecture.

[ I know most RISC processors started with a restriction that they only
  allowed aligned memory accesses, but AFAIK they've changed stance
  since (IIUC the extra hardware can be very little, sometimes even less
  than the hardware that would be needed to implement the ad-hoc
  "support instructions" used to do the unaligned access as a sequence
  of instructions).
  It's one of the RISC simplifications that just didn't pan out.  ]

>> - AFAICT from the backtrace, `ptr` points to a plain normal ELisp
>>   string's content, yet these are supposed to be aligned, so what's
>>   going on here
> I wondered that myself.

And what did you conclude?  ;-)


        Stef

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Stefan Monnier]

>>> - AFAICT from the backtrace, `ptr` points to a plain normal ELisp
>>>   string's content, yet these are supposed to be aligned, so what's
>>>   going on here
>> I wondered that myself.
> And what did you conclude?  ;-)

Hmm... now that I think about it, I think `make_pure_string` may get us
there, because the string's content is allocated from the end without
any alignment!

And the backtrace shows we're hashing the string `DndProtocol` which
comes from `lisp/x-dnd.el` which is indeed preloaded, so I think that's
what's going on.


        Stefan

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Stefan Monnier]

> And the backtrace shows we're hashing the string `DndProtocol` which
> comes from `lisp/x-dnd.el` which is indeed preloaded, so I think that's
> what's going on.

Could you try the patch below?


        Stefan


diff --git a/src/fns.c b/src/fns.c
index 7ab2e8f1a0..0c6bb770ef 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -4610,7 +4610,20 @@ hash_string (char const *ptr, ptrdiff_t len)
    * as `p <= end - 1`.  */
   while (p <= end - 1)
     {
+      /* Here `p` is *almost* always be properly aligned, so we want to
+         optimize for the aligned case, but we still need to support the
+         non-aligned case.  */
+      /* FIXME: Could we just always use `memcpy` and rely on GCC optimizing
+         it to a single word-sized memory access on all-but-sparc64?  */
+#ifdef __sparc64__  /* Arch that still insists on aligned memory accesses. */
+      EMACS_UINT c;
+      if (!((unsigned long)p) % sizeof (c))
+        c = *p;
+      else
+        memcpy (&c, (char const *)p, sizeof (c)); /* `p` is unaligned!  */
+#else
       EMACS_UINT c = *p;
+#fi
       p += step;
       hash = sxhash_combine (hash, c);
     }

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Andreas Schwab]

On Jan 28 2021, Stefan Monnier wrote:

> +#ifdef __sparc64__  /* Arch that still insists on aligned memory accesses. */

$ git grep -E '#define +STRICT_ALIGNMENT +1' -- gcc/config | wc -l
35

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 28 Jan 2021 11:17:31 -0500
> Cc: 46111@debbugs.gnu.org
> 
> +      /* Here `p` is *almost* always be properly aligned, so we want to
> +         optimize for the aligned case, but we still need to support the
> +         non-aligned case.  */
> +      /* FIXME: Could we just always use `memcpy` and rely on GCC optimizing
> +         it to a single word-sized memory access on all-but-sparc64?  */
> +#ifdef __sparc64__  /* Arch that still insists on aligned memory accesses. */
> +      EMACS_UINT c;
> +      if (!((unsigned long)p) % sizeof (c))
> +        c = *p;
> +      else
> +        memcpy (&c, (char const *)p, sizeof (c)); /* `p` is unaligned!  */
> +#else
>        EMACS_UINT c = *p;
> +#fi

AFAIK, memcpy itself optimizes: once it gets to aligned address, it
starts copying words instead of bytes.  So I'm not sure we need either
#ifdef or the run-time condition.  I suggest to time both variants on
x86 architecture, to see whether there's any performance hit due to
use of memcpy, and if not, use memcpy always -- it will let us have
the cake and eat it, too.

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Stefan Monnier]

> AFAIK, memcpy itself optimizes: once it gets to aligned address, it
> starts copying words instead of bytes.

I don't know what this `memcpy` code expands to on sparc64, but since it
turns into a plain `mov` on x86, I simplified the code to always use
`memcpy`.


        Stefan

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Andreas Schwab]

On Jan 28 2021, Stefan Monnier wrote:

>>> - I'd guess that the bus error is due to alignment restrictions.
>>>   What hardware is this running on?
>> See the Subject: it's SPARC64.
>
> I mean the actual hardware, not the architecture.

Strict alignment is a property of the architecture, not the hardware.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Stefan Monnier]

Andreas Schwab [2021-01-28 17:04:19] wrote:

> On Jan 28 2021, Stefan Monnier wrote:
>
>>>> - I'd guess that the bus error is due to alignment restrictions.
>>>>   What hardware is this running on?
>>> See the Subject: it's SPARC64.
>>
>> I mean the actual hardware, not the architecture.
>
> Strict alignment is a property of the architecture, not the hardware.

IIRC some RISC architectures *allowed* errors on unaligned memory
accesses without requiring it, making it a property of the hardware.


        Stefan

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Philipp Stephani]

Am Di., 26. Jan. 2021 um 10:07 Uhr schrieb Ahmed Khanzada <me@enzu.ru>:
>
> I'm not an expert on C, OpenBSD, or SPARC64, but I did notice
> emacs-current was not compiling.
>
> During a gmake, bootstrap-emacs would get a SIGBUS and fail. Running
> gdb led me to the hash_string function where it seemed to be failing.


The culprit might be commit be0f2de179235980b5409d5e77577207b93a4f12.
I don't think that something like
EMACS_UINT const *p   = (EMACS_UINT const *) ptr;
is legal on any platform; it just happens to work on forgiving
platforms such as x86-64.

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Eli Zaretskii]

On January 26, 2021 11:36:25 AM GMT+02:00, Philipp Stephani <p.stephani2@gmail.com> wrote:
> Am Di., 26. Jan. 2021 um 10:07 Uhr schrieb Ahmed Khanzada
> <me@enzu.ru>:
> >
> > I'm not an expert on C, OpenBSD, or SPARC64, but I did notice
> > emacs-current was not compiling.
> >
> > During a gmake, bootstrap-emacs would get a SIGBUS and fail. Running
> > gdb led me to the hash_string function where it seemed to be
> failing.
> 
> 
> The culprit might be commit be0f2de179235980b5409d5e77577207b93a4f12.
> I don't think that something like
> EMACS_UINT const *p   = (EMACS_UINT const *) ptr;
> is legal on any platform; it just happens to work on forgiving
> platforms such as x86-64.

It's definitely legal, as it doesn't violate any laws...

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Andreas Schwab]

On Jan 26 2021, Eli Zaretskii wrote:

> It's definitely legal, as it doesn't violate any laws...

It violates the laws of C.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Eli Zaretskii]

On January 26, 2021 1:17:06 PM GMT+02:00, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Jan 26 2021, Eli Zaretskii wrote:
> 
> > It's definitely legal, as it doesn't violate any laws...
> 
> It violates the laws of C.

We call that "invalid"  not "illegal".

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Andreas Schwab]

On Jan 26 2021, Eli Zaretskii wrote:

> On January 26, 2021 1:17:06 PM GMT+02:00, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> On Jan 26 2021, Eli Zaretskii wrote:
>> 
>> > It's definitely legal, as it doesn't violate any laws...
>> 
>> It violates the laws of C.
>
> We call that "invalid"  not "illegal".

It's still a bug.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: 46111@debbugs.gnu.org, p.stephani2@gmail.com, me@enzu.ru
> Date: Tue, 26 Jan 2021 12:49:44 +0100
> 
> On Jan 26 2021, Eli Zaretskii wrote:
> 
> > On January 26, 2021 1:17:06 PM GMT+02:00, Andreas Schwab <schwab@linux-m68k.org> wrote:
> >> On Jan 26 2021, Eli Zaretskii wrote:
> >> 
> >> > It's definitely legal, as it doesn't violate any laws...
> >> 
> >> It violates the laws of C.
> >
> > We call that "invalid"  not "illegal".
> 
> It's still a bug.

That goes without saying: a crash is always a bug.

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > It violates the laws of C.

C is not a government, and neither is the ISO C Committee.
Breaking the rules of C is not illegal.

Indeed, GNU C goes against the standard in some simple ways
unless you specify options such as --pedantic.

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#46111: Reverting fns.c hash function due to OpenBSD/SPARC64 compile breaking

[Andreas Schwab]

On Jan 27 2021, Richard Stallman wrote:

> Breaking the rules of C is not illegal.

If it breaks, you get to keep both pieces.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."