I don't have a reduced case for this at the moment. We are trying to track down the cause of several failed self tests. But I hope someone can take a quick look and tweak an attribute somewhere. I'm working on GCC112, which is powerpc64le-unknown-linux-gnu. I built Clang from the 7.0 release tarballs. During a run of UBsan I caught several findings like shown below. All the findings pointed to altivec.h:16363. /home/noloader/llvm/lib/clang/7.0.0/include/altivec.h:16363:10: runtime error: load of misaligned address 0x3fffd3453848 for type '__vector unsigned char' (vector of 16 'unsigned char' values), which requires 16 byte alignment 0x3fffd3453848: note: pointer points here 00 00 00 00 a0 10 97 5f 2f e6 c9 b9 69 36 ab 5d 96 9e 6c cf fc c7 e9 ac c0 09 cd f3 aa cf 66 7c ^ altivec.h:16363 is shown below. Notice the vec_xl: 16361 static inline __ATTRS_o_ai vector unsigned char 16362 vec_xl(signed long long __offset, unsigned char *__ptr) { 16363 return *(vector unsigned char *)(__ptr + __offset); 16364 } As far as I know there is no alignment requirement for the instruction.
Actually, UBSan is right. That function does violate the alignment requirement for the type. We should have unaligned types for casts such as this. I'll have a look to see how prevalent it is.
Created attachment 21135 [details] Fix for alignment in altivec.h
The attached patch should fix the problem. Please try it out and let me know if your UBSan run is clean now. If it is, I'll put this up on Phabricator and we'll get it committed after review.
Nice. UBScan is excellent.
(In reply to Nemanja Ivanovic from comment #2) > Created attachment 21135 [details] > Fix for alignment in altivec.h Thanks Nemanja. I also caught several findings on vec_xst. If interested, here are the POWER7 built-in loads and stores we support in our software. I'm thinking all of them may be candidates if they follow a similar pattern. (I'm not sure of the intersection with LLVM/Clang): * vec_xlw4 * vec_xld2 * vec_xl * vec_vsx_ld vec_xlw4, vec_xld2, vec_xstw4, and vec_xstd2 are from XLC 12 compilers, so the IBM folks may be interested in them since switching to the Clang front-end at 13.1. * vec_xstw4 * vec_xstd2 * vec_xst * vec_vsx_st
Forgot to mention... I think some of the vector splats may also be exposed to the issue.
Hi Jeffrey, can you try the patch posted in https://reviews.llvm.org/D54787 and let me know if the UBSan problems go away? If there are others as well, we can deal with them in a subsequent patch.
(In reply to Nemanja Ivanovic from comment #7) > Hi Jeffrey, > > can you try the patch posted in https://reviews.llvm.org/D54787 > and let me know if the UBSan problems go away? > > If there are others as well, we can deal with them in a subsequent patch. Ack, will do. I need to find the procedure to build LLVM from SVN sources with patches. (My scripts only build release tarballs). In the meantime, here's a complete dump of UBsan findings. Sorry I did not provide it sooner. I thought this was going to be a trivial fix for a passing observation. altivec.h:16363 is the vec_xl load. altivec.h:16507 is the vec_xst store. We don't use [a lot of] vec_splats because it stalls the pipe and hurts performance. I would not be surprised if vec_splats has the same issue. ios_base.h:76 is a old GCC bug. See https://lists.llvm.org/pipermail/cfe-dev/2015-January/040945.html. ===== $ cat -n /home/noloader/llvm/lib/clang/7.0.0/include/altivec.h ... 16361 static inline __ATTRS_o_ai vector unsigned char 16362 vec_xl(signed long long __offset, unsigned char *__ptr) { 16363 return *(vector unsigned char *)(__ptr + __offset); 16364 } ... 16504 static inline __ATTRS_o_ai void vec_xst(vector unsigned char __vec, 16505 signed long long __offset, 16506 unsigned char *__ptr) { 16507 *(vector unsigned char *)(__ptr + __offset) = __vec; 16508 } ... ===== /home/noloader/llvm/lib/clang/7.0.0/include/altivec.h:16363:10: runtime error: load of misaligned address 0x3fffd91091a8 for type '__vector unsigned char' (vector of 16 'unsigned char' values), which requires 16 byte alignment /usr/lib/gcc/ppc64le-redhat-linux/4.8.5/../../../../include/c++/4.8.5/bits/ios_base.h:96:24: runtime error: load of value 4294967221, which is not a valid value for type 'std::_Ios_Fmtflags' /usr/lib/gcc/ppc64le-redhat-linux/4.8.5/../../../../include/c++/4.8.5/bits/ios_base.h:76:67: runtime error: load of value 4294967221, which is not a valid value for type 'std::_Ios_Fmtflags' /home/noloader/llvm/lib/clang/7.0.0/include/altivec.h:16363:10: runtime error: load of misaligned address 0x000010dd4407 for type '__vector unsigned char' (vector of 16 'unsigned char' values), which requires 16 byte alignment /home/noloader/llvm/lib/clang/7.0.0/include/altivec.h:16507:3: runtime error: store to misaligned address 0x01002dd66018 for type '__vector unsigned char' (vector of 16 'unsigned char' values), which requires 16 byte alignment
(In reply to Nemanja Ivanovic from comment #7) > Hi Jeffrey, > > can you try the patch posted in https://reviews.llvm.org/D54787 > and let me know if the UBSan problems go away? Thanks again Nemanja. I am embarrassed to ask this, but how do I clone from LLVM testing branch? I'm having trouble locating the procedures. Sorry to ask. Google is turning up a lot of noise. Searching for "LLVM clone testing repo" and "LLVM clone review repo" is returning hits for LLVM Testing Infrastructure Guide and how to write regression tests for the main SVN repo. ===== In the meantime, I can give you a six step procedure to duplicate my results. The only tool required is GNU Make 3.80 or above. GNU make avoids dependencies and replaces Autotools and CMake. 1. ssh gcc112.fsffrance.org 2. build Clang, install at $HOME/llvm 3. git clone https://github.com/weidai11/cryptopp 4. cd cryptopp 5. CXX=$HOME/llvm/bin/clang++ make ubsan 6. ./cryptest.exe v We intentionally make things that simple. We don't like bug reports and mailing list messages. We don't require any build tools like Autotools or CMake. We've actually had folks argue against makefile targets for asan, ubsan, valgrind, etc. But it makes things dead simple for users, and that's what we want.
(In reply to Jeffrey Walton from comment #9) > (In reply to Nemanja Ivanovic from comment #7) > > Hi Jeffrey, > > > > can you try the patch posted in https://reviews.llvm.org/D54787 > > and let me know if the UBSan problems go away? > > Thanks again Nemanja. > > I am embarrassed to ask this, but how do I clone from LLVM testing branch? > I'm having trouble locating the procedures. Hi Jeffrey, I was actually thinking if you could apply that patch directly to whatever source tree of clang you have. It should apply cleanly to $CLANG_SRC/lib/Headers/altivec.h and then simply rebuilding should copy it to the correct location. You can then run the UBSan tests. If this doesn't work for some reason, I'll follow the directions you provided but it might be until some time next week before I get a chance to do that.
(In reply to Nemanja Ivanovic from comment #7) > > can you try the patch posted in https://reviews.llvm.org/D54787 > and let me know if the UBSan problems go away? > > If there are others as well, we can deal with them in a subsequent patch. I was able to drop the new files on top of the files from the LLVM 7.0 source files. I am happy to report they tested as expected. That is, there were no spurious runtime errors reported. (The _Ios_Fmtflags one is still present, but it is a valid finding). And more importantly (for us), we had no self-test failures in AES, GCM, SHA256 or BLAKE2. So your patch fixed us, too. We can now claim support of LLVM Clang on PowerPC. Given your patch fixed loads, stores and encryption algorithms I think it is a very important patch. LLVM might consider releasing a 7.1 soon. Also see https://github.com/weidai11/cryptopp/issues/742#issuecomment-441389956 .
Thanks again Nemanja. I'm catching LLVM test failures with the test files, but I believe it is because of the way I patched or a missing %clang_cc1 . Keep in mind I'm working from LLVM 7.0 release tarballs. My build script is available at https://github.com/noloader/build-llvm . Does this look correct to you? gcc112$ cd ~/llvm_source/ gcc112$ find . -name '*test_CodeGen_builtins-ppc-vsx.c*' ./llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c ./llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c.patched gcc112$ find . -name '*test_CodeGen*' ./llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c ./llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c ./llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c.patched At the moment I stopped downloading test_CodeGen_builtins-ppc-altivec.c and test_CodeGen_builtins-ppc-vsx.c because they look incompatible with LLVM 7.0. (I'm happy to stop here because all of our self tests pass). ========== ... Scanning dependencies of target check-llvm [100%] Running the LLVM regression tests llvm-lit: /home/noloader/llvm_source/llvm/utils/lit/lit/main.py:505: note: raised the process limit from 4096 to 51200 FAIL: LLVM :: CodeGen/test_CodeGen_builtins-ppc-vsx.c (15703 of 26918) ******************** TEST 'LLVM :: CodeGen/test_CodeGen_builtins-ppc-vsx.c' FAILED ******************** Script: -- : 'RUN: at line 2'; %clang_cc1 -target-feature +altivec -target-feature +vsx -triple powerpc64-unknown-unknown -emit-llvm /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c -o - | /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c : 'RUN: at line 3'; %clang_cc1 -target-feature +altivec -target-feature +vsx -triple powerpc64le-unknown-unknown -emit-llvm /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c -o - | /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c -check-prefix=CHECK-LE -- Exit Code: 2 Command Output (stderr): -- /home/noloader/llvm_build/test/CodeGen/Output/test_CodeGen_builtins-ppc-vsx.c.script: line 1: %clang_cc1: command not found FileCheck error: '-' is empty. FileCheck command line: /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-vsx.c -- ******************** FAIL: LLVM :: CodeGen/test_CodeGen_builtins-ppc-altivec.c (15734 of 26918) ******************** TEST 'LLVM :: CodeGen/test_CodeGen_builtins-ppc-altivec.c' FAILED ******************** Script: -- : 'RUN: at line 2'; %clang_cc1 -target-feature +altivec -triple powerpc-unknown-unknown -emit-llvm /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c -o - | /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c : 'RUN: at line 4'; %clang_cc1 -target-feature +altivec -triple powerpc64-unknown-unknown -emit-llvm /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c -o - | /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c : 'RUN: at line 6'; %clang_cc1 -target-feature +altivec -triple powerpc64le-unknown-unknown -emit-llvm /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c -o - | /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c -check-prefix=CHECK-LE : 'RUN: at line 8'; not %clang_cc1 -triple powerpc64le-unknown-unknown -emit-llvm /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c -ferror-limit 0 -DNO_ALTIVEC -o - 2>&1 | /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c -check-prefix=CHECK-NOALTIVEC -- Exit Code: 2 Command Output (stderr): -- /home/noloader/llvm_build/test/CodeGen/Output/test_CodeGen_builtins-ppc-altivec.c.script: line 1: %clang_cc1: command not found FileCheck error: '-' is empty. FileCheck command line: /home/noloader/llvm_build/bin/FileCheck /home/noloader/llvm_source/llvm/test/CodeGen/test_CodeGen_builtins-ppc-altivec.c -- ******************** Testing Time: 59.02s ******************** Failing Tests (2): LLVM :: CodeGen/test_CodeGen_builtins-ppc-altivec.c LLVM :: CodeGen/test_CodeGen_builtins-ppc-vsx.c Expected Passes : 10777 Expected Failures : 44 Unsupported Tests : 16095 Unexpected Failures: 2 gmake[3]: *** [test/CMakeFiles/check-llvm] Error 1 gmake[2]: *** [test/CMakeFiles/check-llvm.dir/all] Error 2 gmake[1]: *** [test/CMakeFiles/check.dir/rule] Error 2 gmake: *** [check] Error 2 Failed to test LLVM sources
The failures appear to be because the test cases patched into the wrong directory. Namely they patched into $LLVM_SRC rather than $LLVM_SRC/tools/clang.
Fix committed in https://reviews.llvm.org/rC347556 Is it too late to merge this into the 7.0.1 release?
Sorry, that was dumb of me. I marked it as a blocker for release 7.0.1, yet resolved it as fixed. Reopening this to be considered for merging.
Hal, what do you think about merging this?
(In reply to Tom Stellard from comment #16) > Hal, what do you think about merging this? To double check, should all of these be aligned(1), or should they have the alignment of the scalar type?
(In reply to Hal Finkel from comment #17) > (In reply to Tom Stellard from comment #16) > > Hal, what do you think about merging this? > > To double check, should all of these be aligned(1), or should they have the > alignment of the scalar type? To be honest, I initially meant to align them as the scalar type, but decided to just make them unaligned for the following reasons: - In practice, we will only do something different between align(16) and align(N) where N < 16 (i.e. VMX vs. VSX loads - or VMX loads with adjustment on non-VSX targets) - I don't know if this is legal or not, but I envisioned a possible use case where someone might pass a pointer to an element of a packed structure (which might have an alignment less than the natural alignment of the scalar type)
(In reply to Nemanja Ivanovic from comment #18) > (In reply to Hal Finkel from comment #17) > > (In reply to Tom Stellard from comment #16) > > > Hal, what do you think about merging this? > > > > To double check, should all of these be aligned(1), or should they have the > > alignment of the scalar type? > > To be honest, I initially meant to align them as the scalar type, but > decided to just make them unaligned for the following reasons: > - In practice, we will only do something different between align(16) and > align(N) where N < 16 (i.e. VMX vs. VSX loads - or VMX loads with adjustment > on non-VSX targets) > - I don't know if this is legal or not, but I envisioned a possible use case > where someone might pass a pointer to an element of a packed structure > (which might have an alignment less than the natural alignment of the scalar > type) Okay. I'd like to match what GCC does in this case (in part, specifically, so that UBSan will catch problems that will show up when using GCC as well as with Clang). Can you please check?
> Okay. I'd like to match what GCC does in this case (in part, specifically, > so that UBSan will catch problems that will show up when using GCC as well > as with Clang). Can you please check? GCC aligns to 1 byte as well. Sample code (the file with a main just calls test with an unaligned pointer): #include <altivec.h> #include <stdio.h> #define PRINT_VEC(X) printf("{ %u, %u, %u, %u }\n", X[0], X[1], X[2], X[3]); void print2(vector unsigned int a); void test(unsigned int *A) { vector unsigned int AV = vec_xl(0, A); print2(AV); #if TRIP_UBSAN printf("*A = %u\n", *A); #endif printf("A = 0x%p\n", A); PRINT_VEC(AV); } $ gcc a.c b.c -O2 -fsanitize=undefined $ ./a.out 2 16 { 33554432, 268435456, 33554432, 268435456 } A = 0x0x3fffde4c48a1 { 33554432, 268435456, 33554432, 268435456 } $ gcc a.c b.c -O2 -fsanitize=undefined -DTRIP_UBSAN $ ./a.out 2 16 { 33554432, 268435456, 33554432, 268435456 } b.c:9:3: runtime error: load of misaligned address 0x3ffff11e1751 for type 'unsigned int', which requires 4 byte alignment 0x3ffff11e1751: note: pointer points here 00 00 00 00 00 00 00 02 00 00 00 10 00 00 00 02 00 00 00 10 00 00 00 02 00 00 00 10 00 00 00 02 ^ *A = 33554432 A = 0x0x3ffff11e1751 { 33554432, 268435456, 33554432, 268435456 }
(In reply to Hal Finkel from comment #19) > (In reply to Nemanja Ivanovic from comment #18) > > ... > Okay. I'd like to match what GCC does in this case (in part, specifically, > so that UBSan will catch problems that will show up when using GCC as well > as with Clang). Can you please check? Hal, sorry to step in. We are also having trouble with GCC 8. Previous versions were OK. I would caution about following GCC too closely. Also see https://github.com/weidai11/cryptopp/issues/749. My apologies again. I'd hate to see you take well working code and unintentionally transform it into something less.
(In reply to Jeffrey Walton from comment #21) > (In reply to Hal Finkel from comment #19) > > (In reply to Nemanja Ivanovic from comment #18) > > > ... > > Okay. I'd like to match what GCC does in this case (in part, specifically, > > so that UBSan will catch problems that will show up when using GCC as well > > as with Clang). Can you please check? > > Hal, sorry to step in. > > We are also having trouble with GCC 8. Previous versions were OK. I would > caution about following GCC too closely. Also see > https://github.com/weidai11/cryptopp/issues/749. > > My apologies again. I'd hate to see you take well working code and > unintentionally transform it into something less. Okay. We need to check with the GCC team on what they're planning to do in this regard. If GCC 8 has already shipped with stricter alignment requirements, and they're not planning to change that, then I don't want to loosen ours too much so that UBSan will actually pick up problems that will manifest when using newer versions of GCC.
Ack. And in case it helps establish provenance for LLVM and Clang behavior... Looking at the 64-Bit ELF V2 ABI Specification [1], p. 122, I believe this is the controlling paragraph: Pointers to vector types are defined like pointers of other C/C++ types. Pointers to objects may be defined to have const and volatile properties. While the preferred alignment for vector data types is a multiple of 16 bytes, pointers may point to vector objects at an arbitrary alignment. From a little earlier in the page, this is also relevant: This [the ELF V2 ABI] is primarily applicable to the vector facility of the POWER ISA, also known as Power SIMD, consisting of the VMX (or Altivec) and VSX instructions. This set of instructions operates on groups of 2, 4, 8, or 16 vector elements at a time in 128-bit registers. I think use of "or Altivec" is a bit misleading when paired with VSX above because VSX is roughly POWER7 and the ISA allows unaligned loads and stores. (The provenance of the "VSX portion" of the statement is in the Power ISA 2.06 document). Even though Altivec allows the pointer to point to any memory address, the effective address must be 16-byte aligned. So it is eliding a lot of information about Altivec. If the programmer uses a misaligned effective memory address on Altivec (non-VSX machines) then they will suffer the consequences. Based on some OpenPower recommendations I tried to do this a long time ago on my old PowerMac G5 running MacPorts with GCC 5 or 6: uint32_t d[4] = ...; vector unsigned int x = *(vector unsigned int*)d; It did not work as expected, and it did not end well. [1] https://openpowerfoundation.org/?resource_lib=64-bit-elf-v2-abi-specification-power-architecture
Hal, I believe Jeffrey's comment regarding newer versions of GCC was meant as a general cautionary statement regarding following GCC precedent. The issue he mentioned does not pertain to the alignment we are discussing here but to GCC using signed integers instead of unsigned under the covers somehow. I've done a further test with a fairly recent GCC8 for this particular issue and the behaviour hasn't changed. gcc (GCC) 8.1.1 20180702 [gcc-8-branch revision 262315] Exhibits the exact same behaviour regarding alignment for vec_xl and friends.
Regarding comment #23, note that some of the text you reference has been redacted. See the 1.4 Errata at the same website. The vec_xl and vec_xst interfaces are the only supported mechanisms for accessing unaligned data as vectors. Programmers are cautioned not to cast an unaligned pointer to a pointer-to-vector, as this constitutes undefined behavior under the C standard.
(In reply to Nemanja Ivanovic from comment #24) > Hal, I believe Jeffrey's comment regarding newer versions of GCC was meant > as a general cautionary statement regarding following GCC precedent. The > issue he mentioned does not pertain to the alignment we are discussing here > but to GCC using signed integers instead of unsigned under the covers > somehow. > > I've done a further test with a fairly recent GCC8 for this particular issue > and the behaviour hasn't changed. > > gcc (GCC) 8.1.1 20180702 [gcc-8-branch revision 262315] > > Exhibits the exact same behaviour regarding alignment for vec_xl and friends. In that case, yes, please pull in the change. Thanks!
Merged: r347935