Hi, Kurt, > On Wed, Feb 13, 2013 at 12:19:19PM +0100, Andy Polyakov via RT wrote: >> >>> Probably this "strict aliasing" 64-bit optimization bug for >>> "crypto/bn/bn_nist.c" > What bug are you talking about? There doesn't seem to be a strict > aliasing warning in that file, and they use a union to get around > the problem.
Sorry, I mean about compilers bug of "-fstrict-aliasing" optimization. > I can at least reproduce the problem with clang 3.0. > It works without problems with gcc 4.7. gcc-mp-4.7 & gcc-mp-4.8 not affected. But Apple Xcode compiler and MacPorts clang compilers affected (based on llvm version 3.0-3.2). Number MacPorts user report about this problem, see https://trac.macports.org/ticket/38015 > PS: I think at least this patch makes sense, but doesn't change anything: > - const unsigned int *bp=(const unsigned int *)buf; > + const unsigned int *bp=(const unsigned int *)buf.ui; This patch inefficient. Workaround: leom:openssl-SNAP-20130212.test-x86_64 leo$ diff -u ../openssl-SNAP-20130212/crypto/bn/bn_nist.c crypto/bn/bn_nist.c --- ../openssl-SNAP-20130212/crypto/bn/bn_nist.c 2013-01-11 18:13:43.000000000 +0400 +++ crypto/bn/bn_nist.c 2013-02-14 03:06:43.000000000 +0400 @@ -424,7 +424,7 @@ #if defined(NIST_INT64) { NIST_INT64 acc; /* accumulator */ - unsigned int *rp=(unsigned int *)r_d; + volatile unsigned int *rp=(unsigned int *)r_d; const unsigned int *bp=(const unsigned int *)buf.ui; acc = rp[0]; acc += bp[3*2-6]; @@ -556,7 +556,7 @@ #if defined(NIST_INT64) && BN_BITS2!=64 { NIST_INT64 acc; /* accumulator */ - unsigned int *rp=(unsigned int *)r_d; + volatile unsigned int *rp=(unsigned int *)r_d; const unsigned int *bp=(const unsigned int *)buf.ui; acc = rp[0]; acc -= bp[7-7]; @@ -704,7 +704,7 @@ #if defined(NIST_INT64) { NIST_INT64 acc; /* accumulator */ - unsigned int *rp=(unsigned int *)r_d; + volatile unsigned int *rp=(unsigned int *)r_d; const unsigned int *bp=(const unsigned int *)buf.ui; acc = rp[0]; acc += bp[8-8]; @@ -909,7 +909,7 @@ #if defined(NIST_INT64) { NIST_INT64 acc; /* accumulator */ - unsigned int *rp=(unsigned int *)r_d; + volatile unsigned int *rp=(unsigned int *)r_d; const unsigned int *bp=(const unsigned int *)buf.ui; acc = rp[0]; acc += bp[12-12]; -- Sorry for my bests English. Serguei E. Leontiev w:+7(495)939-2382 USSR,Moscow,Universitetskij 13 Sternberg Astronom. w:+7(495)780-4820 USSR,Moscow,127018,Sushchevskij val 16-5 Institute, MSU h:+7(495)318-1146 USSR,Moscow,113303,Kakhovka 6-40 m:+7(916)686-1081 SMS: <http://www.mts.ru/sms> <http://lnfm1.sai.msu.ru/~leo> 13.02.2013, в 23:23, Kurt Roeckx <k...@roeckx.be> написал(а): > On Wed, Feb 13, 2013 at 12:19:19PM +0100, Andy Polyakov via RT wrote: >> >>> Probably this "strict aliasing" 64-bit optimization bug for >>> "crypto/bn/bn_nist.c" > > What bug are you talking about? There doesn't seem to be a strict > aliasing warning in that file, and they use a union to get around > the problem. > >>> Mac OSX compiler fail test/ectest: cc [Apple LLVM version 4.2 >>> (clang-425.0.24) (based on LLVM 3.2svn)] gcc-mp-4.3 gcc-mp-4.4 gcc-mp-4.5 >>> gcc-mp-4.6 clang-mp-3.0 clang-mp-3.1 clang-mp-3.2 >>> >>> Mac OSX compiler test/ectest OK: gcc-apple-4.2 gcc-mp-4.7 gcc-mp-4.8 >>> [gcc-mp-4.8 (MacPorts gcc48 4.8-20130203_0+universal) 4.8.0 20130203 >>> (experimental)] clang-mp-2.9 clang-mp-3.3 [clang version 3.3 (trunk 173279)] > > I can at least reproduce the problem with clang 3.0. > > It works without problems with gcc 4.7. > >> Could you test following *instead*? In every #if defined(NIST_INT64) >> section you'll see a number of references to bp[something]. Can you >> replace them with buf.ui[samething] and run the test? Currently bp is >> constified buf.ui and it might give overeager compiler idea to reorder >> references to buf in #if defined(NIST_IN64) section and [inlined] >> nist_cp_bn_0 and cause the mayhem. > > That doesn't change anything. I still get: > testing internal curves: ........... > EC_GROUP_check() failed with curve secp384r1 > . > EC_GROUP_check() failed with curve prime192v1 > > EC_GROUP_check() failed with curve prime192v2 > > EC_GROUP_check() failed with curve prime192v3 > ... > EC_GROUP_check() failed with curve prime256v1 > ............................................... failed > > ectest.c:1268: ABORT > > > Kurt > > > PS: I think at least this patch makes sense, but doesn't change anything: > --- a/crypto/bn/bn_nist.c > +++ b/crypto/bn/bn_nist.c > @@ -530,7 +530,7 @@ int BN_nist_mod_224(BIGNUM *r, const BIGNUM *a, const > BIGNUM *field, > { > NIST_INT64 acc; /* accumulator */ > unsigned int *rp=(unsigned int *)r_d; > - const unsigned int *bp=(const unsigned int *)buf; > + const unsigned int *bp=(const unsigned int *)buf.ui; > > acc = rp[0]; acc -= bp[7-7]; > acc -= bp[11-7]; rp[0] = (unsigned int)acc; acc >>= 32; > ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
