That's unfortunate. It is somewhat surprising that C memmove breaks word boundary, even the thing we move is a multiple of word size and well aligned...

I'm not really involved with what goes into glibc for each ISA and distro. Sometimes for a distro they want to chose a version that will work on multiple ISAs, and different ISAs might have different alignment requirements so that all affects its implementation. However the main point is that libc's memmove has no guarantee of atomicity of pointer copies so when it is implemented that is not taken into consideration. Having said that, I looked at the Go language definition and I don't see where it mentions that pointers are always aligned either. So couldn't there be a pointer within a struct that is unaligned because of the size of the previous structure members? I'm looking at the Go Programming Language document, at the very end where it says "size and alignment guarantees".

We don't provide any alignment guarantees on pointers to the user.
We do require it internally, though. Pointers must be naturally aligned. Struct layout will insert padding if the field before a pointer does not end at pointer alignment. (And a struct containing a pointer must be pointer-aligned as well.)

It is deeply baked in the runtime that pointers are aligned. If it is not, everything will go wrong. The compiler ensures pointers are aligned.

@ianlancetaylor Would it be possible to add (power) asm implementations of memove and memclr to gccgo that are similar to those implementations in golang?

Sure, it's possible. We have one piece of assembler code already in runtime/go-context.S. Of course it will be necessary to change the frontend to call those functions where appropriate, which may mean losing some compiler optimizations.

That said, I haven't looked into this issue, and I'm somewhat flabbergasted that the C library memmove function doesn't do the right thing. As far as I can see the natural way to write memmove on PPC64 would copy a pointer at a time. Any idea why that doesn't happen?

I'm somewhat flabbergasted that the C library memmove function doesn't do the right thing. As far as I can see the natural way to write memmove on PPC64 would copy a pointer at a time.

This is my impression as well. Kind of surprising.

Of course it will be necessary to change the frontend to call those functions where appropriate, which may mean losing some compiler optimizations.

Maybe it is possible to play some symbol shadowing trick? In the frontend we still lower to __builtin_memmove and when the backend decides to generate an actual memmove call, our symbol shadows the libc symbol? Not sure if it would work. Also not sure about all build modes... (Kind of ugly anyway.)

That said, I haven't looked into this issue, and I'm somewhat flabbergasted that the C library memmove function doesn't do the right thing. As far as I can see the natural way to write memmove on PPC64 would copy a pointer at a time. Any idea why that doesn't happen?

There is no requirement that the memmove in libc ensure pointer atomicity, so they wouldn't have tested for it. On previous ISAs the alignment requirements varied, so that moving 8-bytes required certain alignment either for performance or correctness. In the memmove for the distro they wanted an implementation that would work for several instruction sets. In some cases doing 2 4-byte moves was as good as an 8-byte move if they didn't have to check for alignment.

OK, thanks. It's fine with me to add assembler code for the PPC memmove, as long as on other platforms we continue to call __builtin_memmove. This might involve a PPC-specific declaration of runtime.memmove. It looks like the compiler only calls __builtin_memmove for cases that do not involve pointers, so this might not be too hard.

We are also seeing this failure in issue13160.go intermittently, which I believe is the same issue:

panic: bad pointer read 0x27c038!

goroutine 176 [running]:
main.main..func2
        /home/boger/gccgo.git/gcc/gcc/testsuite/go.test/test/fixedbugs/issue13160.go:61
created by main.main
        /home/boger/gccgo.git/gcc/gcc/testsuite/go.test/test/fixedbugs/issue13160.go:56 +0x394
panic: bad pointer read 0xc000000000!

@ianlancetaylor What is your recommendation where the ppc64 asm implementation for memmove belongs?

I think libgo/runtime is probably the right place.

Sir, I have a issue like this... I wonder how long does it take to be implemented?

I'm not going to commit to a date on this at this point. What gcc release are you using and do you need it for big or little endian?

I am using gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 and little endian.

@ianlancetaylor I don't want to break older BE ISAs (< power7) but if I port over the Golang memmove to ppc64asm I don't know if it will work there and I have no way to test those. I assume it would be OK to use libc memmove for < power7 and the new asm for >= power7 so the old ones continue to work the same? Also I suspect the memmove atomicity isn't broken for < power7 but again can't test that.

It's OK with me to only use the new assembler code on newer processors.

We could also just write the code in C. Or switch between C and assembler based on the processor.

If this was written in C then that should work on all processors and no need to switch back and forth.

I would be fine with a C implementation but it might not be the fastest possible.

@ianlancetaylor Sir, are you going to fix this bug(memmove in glibc)?

@tkdlqm2 I'm leaving this for @laboger .

@Helflym Is this an issue on AIX for gccgo?

@ianlancetaylor I haven't had to update Makefiles or configure files in gcc for a while, so maybe I'm doing something wrong. If I regenerate the configure file in libgo from configure.ac using autoconf it doesn't match what's in master.

diff --git a/libgo/configure b/libgo/configure
index 70f64c974fd..b23f145480f 100755
--- a/libgo/configure
+++ b/libgo/configure
@@ -818,6 +818,7 @@ infodir
 docdir
 oldincludedir
 includedir
+runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -900,6 +901,7 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -1152,6 +1154,15 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
+  -runstatedir | --runstatedir | --runstatedi | --runstated \
+  | --runstate | --runstat | --runsta | --runst | --runs \
+  | --run | --ru | --r)
+    ac_prev=runstatedir ;;
+  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
+  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
+  | --run=* | --ru=* | --r=*)
+    runstatedir=$ac_optarg ;;
+
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1289,7 +1300,7 @@ fi
 for ac_var in  exec_prefix prefix bindir sbindir libexecdir datarootdir \
                datadir sysconfdir sharedstatedir localstatedir includedir \
                oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-               libdir localedir mandir
+               libdir localedir mandir runstatedir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1442,6 +1453,7 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
+  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
@@ -11544,7 +11556,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11557 "configure"
+#line 11559 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -11650,7 +11662,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11663 "configure"
+#line 11665 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H

If I add a new file go-memmove.c and only use it for PPC64 then it looks like I have to update configure.ac, regenerate configure, and update Makefile.in.

@laboger Honestly no idea. I don't think so. As long as you stay with C code, it should work. I'll have to check if the test is failing in first instance too. I haven't run gccgo testsuite for a while.

@Helflym I was mainly wondering if the fix should be done for AIX too. I think the answer is yes.

@laboger I think too. If it doesn't work, I'll revert for AIX. But I see no disadvantage to have a internal memmove instead of calling a libc version. Especially on AIX where libcalls are expensives.

@laboger You have to use an unpatched version of autoconf 2.69. Unfortunately distros often patch autoconf, so you can't use a distro autoconf, you have to build it yourself. That said, I can fix this up for you.

@ianlancetaylor I'm working on a fix for memmove which I believe works but now the test fails because of memclrNoHeapPointers which uses memset. Based on the function name I had assumed that it didn't require 8-byte atomicity, but the test expects it.

Where are these requirements documented?

Unfortunately I doubt that they are documented anywhere.

memclrNoHeapPointers does need pointer atomicity, because it is the underlying implementation of memclrHasPointers (which is write barrier + memclrNoHeapPointers).

We probably want to document it somewhere.

Change https://golang.org/cl/287692 mentions this issue: runtime: document pointer write atomicity for memclrNoHeapPointers

I think the solution for this is to have Power asm versions of both memmove and memclr. We can't have a C memclr because the libc memset doesn't ensure 8 byte atomicity and the compiler will in some cases determine that C code could be done by the memset builtin and use that. If memset must be asm then might as well do memmove also.

Changed my mind again -- better to use C.

Change https://golang.org/cl/294931 mentions this issue: libgo: ensure memmove, memset 8 byte atomicity on ppc64x