COPY memory alignment SIGSEGV bug

[apotschka]

Minimal example check_copy.c, compiled via

$ cc -g -o check_copy check_copy.c /usr/lib/libopenblas.so.0

Error occurs for offset == 1:

$ valgrind ./check_copy
==16357== Memcheck, a memory error detector
==16357== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16357== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==16357== Command: ./check_copy
==16357==
1.000000 1.000000
0.500000 0.500000
0.333333 0.333333
==16357==
==16357== Process terminating with default action of signal 11 (SIGSEGV)
==16357==  General Protection Fault
==16357==    at 0x64FB980: dcopy_k_HASWELL (in /usr/lib/libopenblasp-r0.2.18.so)
==16357==    by 0x6EEE82F: (below main) (libc-start.c:291)

Changing the increment of offset to offset += 8 makes the code run fine.

System:

$ uname -a
Linux *** 4.4.0-67-generic #88-Ubuntu SMP Wed Mar 8 16:34:45 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
$ cc --version
cc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
$ zcat /usr/share/doc/libopenblas-base/changelog.Debian.gz | head -1
openblas (0.2.18-1ubuntu1) xenial; urgency=medium
$ cat /proc/cpuinfo | head -30
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 63
model name      : Intel(R) Core(TM) i7-5820K CPU @ 3.30GHz
stepping        : 2
microcode       : 0x36
cpu MHz         : 1201.921
cache size      : 15360 KB
physical id     : 0
siblings        : 12
core id         : 0
cpu cores       : 6
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 15
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx est tm2 ssse3 sdbg fma cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm epb tpr_shadow vnmi flexpriority ept vpid fsgsbase tsc_adjust bmi1 avx2 smep bmi2 erms invpcid cqm xsaveopt cqm_llc cqm_occup_llc dtherm ida arat pln pts
bugs            :
bogomips        : 6599.88
clflush size    : 64
cache_alignment : 64
address sizes   : 46 bits physical, 48 bits virtual
power management:

processor       : 1
vendor_id       : GenuineIntel
cpu family      : 6

I know it is not a good idea to have the vectors not aligned at 8 byte address boundaries. However, my expectation would be that the code does not crash in this case.

[martin-frbg]

Segfault happens on Nehalem as well, copy_sse2.S appears to be original code from libgoto. I do not have time today to investigate further - maybe there is something unusual about the testcase if it comes up only now, maybe the assembly "only" needs the proper .align in the proper place ?

[martin-frbg]

For the record, gdb puts the fault at line 216 of copy_sse2.S, which is movaps -16*SIZE(X),%xmm0

[apotschka]

I guess the problem is violation of Intel ABI, pp. 13-14:

Like the Intel386 architecture, the AMD64 architecture in general does not require all data accesses to be properly aligned. Misaligned data accesses are slower than aligned accesses but otherwise behave identically. The only exceptions are that __m128, __m256 and __m512 must always be aligned properly.

@brada4: Is turning off SSE also for properly aligned memory really a good idea in view of performance?

I would be happy with the following resolution:

• Keep OpenBLAS code as is. The fault really indicates a memory misalignment problem in the calling code.
• Document crash messages with misaligned memory so that users can find them via google and fix their code. Maybe this issue is a good place in addition to an entry at the FAQ.

[brada4]

SSE2 is omnipresent on x86_64, and it is emitted at any compiler, and _copy (Or any L1 BLAS) routines are completely memory bound. Even glibc has AVX memory copy routines nowadays, probably SSE assembly is even slower (memory bound... i.e spends 2x more CPU cycles in same time) than memcpy in the border case.
I think no big damage bypassing something that tries corrupting irrelevant memory.

[brada4]

@martin-frbg what makes me wonder that sandybridge is not affected.

[martin-frbg]

Wouldn't the test case above (with offset=8 of course) allow benchmarking of sse vs. compiler-picked implementation rather than handwaving ?

[brada4]

Most kernels assume aligned input anyway. But that does not mean need to crash in unaligned cases.
i.e they start with 2^n-sized data blocks then process tail of array in smaller blocks. To handle unaligned input optimally other tail must process head of input until aligned block start is reached.

[martin-frbg]

Maybe it is just the cblas interface that should ensure proper alignment before calling the kernel function (but that would seem to create the weird situation of using a potentially inefficient copy implementation just to drive the hand-optimized one) ? At least playing with the .align statements already present in copy_sse2.S does not appear to help, while replacing the two affected movaps instructions at label L15 with movups does. Still no benchmarking to see which version wins.

[martin-frbg]

My preliminary tests suggest that replacing the movaps instructions of copy_sse2.S with movups has almost negligible impact on Nehalem at least, while the plain C implementation from arm/copy.c is significantly slower. (320 vs 500 seconds for the aligned version of check_copy.c with nm=300 and an outer loop that does 10 million invocations of the loop on "offset").

[martin-frbg]

From various sources, it appears that movups is identical to movaps in performance when accessing aligned memory on Intel targets since Nehalem. For AMD processors - which I do not have available - I only find this feature expressly mentioned in their Software Optimization Guide for the "Family 16h" processors, but not for 15h (Bulldozer et al.)
As (I think) was already mentioned above, "fixing" just the xCOPY functions would probably only delay the crash with unaligned data until another optimized function is reached, and general perfomance will probably be poorer than with properly aligned data. I am not aware of a more benign way to signal to a user that they had better align their data however...

[apotschka]

Why not check memory alignment on each call and fail with a meaningful error message on misalignment instead of a SIGSEGV? Checking alignment costs just an OR operation of the address with a mask of 0x3 (8 bytes), 0x7 (16 bytes), 0xF (32 bytes), etc. and a jump to the error handler if the result is nonzero. Branch prediction (no jump) should work efficiently in this case.

[martin-frbg]

Have to defer to @xianyi and other core developers on this, but to me this sounds expensive even if it is "just an OR operation" on every call.
Perhaps providing a set of utility functions to make it easier for a user to check their input and/or documenting the requirement of proper alignment would help already ?
copy_sse2.S appears to be unchanged from libGoto2 so the issue does not appear to arise often (though of course nobody knows how many people may have hit that segmentation fault and switched to a different blas implementation immediately).
As an immediate fix for just this specific issue, copy_sse2.S could be cloned to a new file that uses movups throughout and is referenced from just KERNEL.NEHALEM, KERNEL.SANDYBRIDGE and KERNEL.HASWELL so as not to compromise the performance on older or AMD cpus. Or would the performance difference be tiny even there - I see movups being used (via #define movapd movups even) in trsm_kernel_LN_4x4_barcelona.S and others already - so that it could just be replaced unconditionally ?