Closed Bug 1253913 (ECL_USE_FP) Opened 5 years ago Closed 4 years ago

ECL_USE_FP support

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox47 affected)

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

ECL_USE_FP is not supported with the current code and can only used together with NSS_ECC_MORE_THAN_SUITE_B. The code is unmaintained and does not build (e.g. ecp_fpinc.c contains syntax errors).
Alias: ECL_USE_FP
Attached patch ecl-use-fp-removal.patch (obsolete) — Splinter Review
this removes all ecc fp code and the corresponding build flag ECL_USE_FP. The code hasn't been used for a while now and was only introduced for better performance on UltraSPARC III chips.
Assignee: nobody → franziskuskiefer
Attachment #8737781 - Flags: review?(wtc)
Attachment #8737781 - Flags: review?(rrelyea)
Comment on attachment 8737781 [details] [diff] [review]
ecl-use-fp-removal.patch

Review of attachment 8737781 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. IMPORTANT: the changes to ecl.c are wrong. Please fix them
and upload a new patch before you check this in.

After this patch is checked in, we should verify a clean removal
with this MXR query:

http://mxr.mozilla.org/nss/search?string=ECL_USE_FP

::: lib/freebl/ecl/ecl.c
@@ -232,5 @@
> -				ECGroup_consGFp(&irr, &curvea, &curveb, &genx, &geny,
> -								&order, params->cofactor);
> -			if (group == NULL) { res = MP_UNDEF; goto CLEANUP; }
> -			MP_CHECKOK(ec_group_set_nistp192_fp(group));
> -#else

IMPORTANT: this is wrong. This #else block and the #else
block that begins on line 251 should be kept.
Attachment #8737781 - Flags: review?(wtc) → review+
Comment on attachment 8737781 [details] [diff] [review]
ecl-use-fp-removal.patch

Review of attachment 8737781 [details] [diff] [review]:
-----------------------------------------------------------------

This code does remove fp. The r+ is for that. I'd like Julian to chime in that he's fine with this. I think the fact that it doesn't compile is pretty compeling that they likely aren't building with it.

bob
Attachment #8737781 - Flags: review?(rrelyea) → review+
Attachment #8737781 - Flags: review?(julien.pierre)
Comment on attachment 8737781 [details] [diff] [review]
ecl-use-fp-removal.patch

Review of attachment 8737781 [details] [diff] [review]:
-----------------------------------------------------------------

The changes in ecl.c are wrong, as Wan-Teh pointed out. It's not just that the #else block has been deleted, but support for the following curves also seems to have been deleted :

1) ECCurve_SECG_PRIME_160R1
This one appears to have code only if ECL_USE_FP was set . This is strange. Not sure if the existing code was correct, and how this curve could have worked if the ECL_FP code indeed doesn't build.

2) ECCurve_SECG_PRIME_192R1, ECCurve_SECG_PRIME_224R1
These two had code for either ECL_USE_FP or not. The #else block need to be preserved.

I'm in the process of checking our official Sparc builds to see how they were actually done.
FYI, the ECL_USE_FP=1 builds successfully in 32-bit mode on a Linux host.
It only fails when building 64-bit. So, it's possible the code is actually still in use on Sparc 32 bit. While 32-bit Solaris is out of support, there may still be 32-bit apps that are supported, and we still build Solaris 32 bit on Sparc.

As far as the pre-existing error in ecl.c above, the function ecgroup_fromNameAndHex seems to be called only in mpi test programs, but never in an exposed function in freebl or softoken. That explains the pre-existing problem - we are not running the mpi test programs.
I have confirmed that the keygen & cert install for the curve secp160r1 works in our products as built and shipped on x64. But still trying to locate a Solaris host to see exactly what the story is there.
I have confirmed that we are still building this ECL_USE_FP code on both Sparc 32 bit and 64 bit in our official builds. There are 2 variants of freebl libraries for 32 bit, and 2 variants for 64 bit. The variants with the name "fpu" in them have ecl_fpXXX.o object files built.

While the Ultrasparc III/IV CPUs are no longer supported in Solaris 11, it is still supported under Solaris 10 . Thus, these chips still benefit from these FPU optimizations.

Solaris 10 premier support (1st phase) ends in Jan 2018. Extended support ends in Jan 2021. Until those dates pass, we may not want to remove the ECL_USE_FP code.
Comment on attachment 8737781 [details] [diff] [review]
ecl-use-fp-removal.patch

This code is still being used by Oracle.
Attachment #8737781 - Flags: review?(julien.pierre) → review-
Re: ecp_fpinc.c, this file is only being #included from other C files. It is not meant to be built standalone. It does successfully build as part of ecp_fp160.c, ecp_fp192.c, and ecp_fp224.c . I do not believe it contains syntax errors.
(In reply to Julien Pierre from comment #8)
> Re: ecp_fpinc.c, this file is only being #included from other C files. It is
> not meant to be built standalone. It does successfully build as part of
> ecp_fp160.c, ecp_fp192.c, and ecp_fp224.c . I do not believe it contains
> syntax errors.

ecp_fpinc.c does contain a syntax error that are thrown when building nss.

> ecl/ecp_fpinc.c:13:1: error: "/*" within comment [-Werror=comment]
> /* Adds a prefix to a given token to give a unique token name. Prefixes

This is easy to fix (and I think the only error) but I don't think any of the buildbots is using the ECL_USE_FP flag. Otherwise we would have seen the error. It would be great if Oracle would provide patches that makes old code like this actually compile if it's still using it.

While we could make this code work again we should wait for a decision on bug 1253912 as this code can only be used together with NSS_ECC_MORE_THAN_SUITE_B.
(In reply to Julien Pierre from comment #4)
> 
> Review of attachment 8737781 [details] [diff] [review]:
[...]
> 1) ECCurve_SECG_PRIME_160R1
> This one appears to have code only if ECL_USE_FP was set . This is strange.
> Not sure if the existing code was correct, and how this curve could have
> worked if the ECL_FP code indeed doesn't build.

Julien:

I looked into this. This change only removes an optimization
for that curve. That curve is still supported by using the
default implementation. So this change is fine.

Also, the three curves in question have small key sizes (160,
192, and 224) by today's standards. They are unlikely to be
used. So I think it is fine to use a less-optimized
implementation of these three curves, on any CPU.

Franziskus:

This patch actually shows the ECL_USE_FP code does NOT
complicate our implementation. The code in the shared
files (ecl-priv.h and ecl.c) for supporting ECL_USE_FP
is cleanly separated in an "object-oriented" fashion.
Although I still support removing the ECL_USE_FP code,
I just wanted to give an objective assessment of its
"harm".
(In reply to Wan-Teh Chang from comment #10)
> (In reply to Julien Pierre from comment #4)
> > 
> > Review of attachment 8737781 [details] [diff] [review]:
> [...]
> > 1) ECCurve_SECG_PRIME_160R1
> > This one appears to have code only if ECL_USE_FP was set . This is strange.
> > Not sure if the existing code was correct, and how this curve could have
> > worked if the ECL_FP code indeed doesn't build.
> 
> Julien:
> 
> I looked into this. This change only removes an optimization
> for that curve. That curve is still supported by using the
> default implementation. So this change is fine.
> 
> Also, the three curves in question have small key sizes (160,
> 192, and 224) by today's standards. They are unlikely to be
> used. So I think it is fine to use a less-optimized
> implementation of these three curves, on any CPU.
> 

Correct, this does not remove support for any curve. Only optimised code.

> Franziskus:
> 
> This patch actually shows the ECL_USE_FP code does NOT
> complicate our implementation. The code in the shared
> files (ecl-priv.h and ecl.c) for supporting ECL_USE_FP
> is cleanly separated in an "object-oriented" fashion.
> Although I still support removing the ECL_USE_FP code,
> I just wanted to give an objective assessment of its
> "harm".

I agree. This code (and its removal) is relatively non-intrusive. But keeping the code also means that we have to maintain it, which is the reason why I'm strongly in favour of removing it. But depending on the outcome of the NSS_ECC_MORE_THAN_SUITE_B discussion the question of removing ECL_USE_FP might be solved implicitly.
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #9)
> ecp_fpinc.c does contain a syntax error that are thrown when building nss.
> 
> > ecl/ecp_fpinc.c:13:1: error: "/*" within comment [-Werror=comment]
> > /* Adds a prefix to a given token to give a unique token name. Prefixes
> 
> This is easy to fix (and I think the only error) but I don't think any of
> the buildbots is using the ECL_USE_FP flag. Otherwise we would have seen the
> error. It would be great if Oracle would provide patches that makes old code
> like this actually compile if it's still using it.

We don't have any patches applied to make this code build. I believe the difference here is only that you are using a different compiler, which considers this to be an error, rather than a warning.

Here is an excerpt from my build on Linux x86 with the ECL_USE_FP=1

[jrpierre@slc01heg freebl]$which gcc
/usr/bin/gcc
[jrpierre@slc01heg freebl]$gcc -v
Using built-in specs.
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 4.4.7 20120313 (Red Hat 4.4.7-16) (GCC) 
[jrpierre@slc01heg freebl]$pwd
/scratch/jrpierre/nss/321/nss/lib/freebl
[jrpierre@slc01heg freebl]$gmake all ECL_USE_FP=1 > & log2
[jrpierre@slc01heg freebl]$

Output :

gcc -o Linux3.8_x86_cc_glibc_PTH_DBG.OBJ/Linux_SINGLE_SHLIB/ecp_fp160.o -c -g -fPIC -Di386 -DLINUX2_1 -m32 -Wall -DNSS_NO_GCC48 -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -UNDEBUG -DDEBUG_jrpierre -D_REENTRANT -DNSS_ECC_MORE_THAN_SUITE_B -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DFREEBL_NO_DEPEND -DNSS_X86_OR_X64 -DNSS_X86 -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE  -DMP_ASSEMBLY_DIV_2DX1D -DMP_USE_UINT_DIGIT -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN -DECL_USE_FP -DMP_API_COMPATIBLE -I../../../dist/Linux3.8_x86_cc_glibc_PTH_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl  ecl/ecp_fp160.c
In file included from ecl/ecp_fp160.c:11:
ecl/ecp_fpinc.c:13:1: warning: "/*" within comment
gcc -o Linux3.8_x86_cc_glibc_PTH_DBG.OBJ/Linux_SINGLE_SHLIB/ecp_fp192.o -c -g -fPIC -Di386 -DLINUX2_1 -m32 -Wall -DNSS_NO_GCC48 -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -UNDEBUG -DDEBUG_jrpierre -D_REENTRANT -DNSS_ECC_MORE_THAN_SUITE_B -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DFREEBL_NO_DEPEND -DNSS_X86_OR_X64 -DNSS_X86 -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE  -DMP_ASSEMBLY_DIV_2DX1D -DMP_USE_UINT_DIGIT -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN -DECL_USE_FP -DMP_API_COMPATIBLE -I../../../dist/Linux3.8_x86_cc_glibc_PTH_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl  ecl/ecp_fp192.c
In file included from ecl/ecp_fp192.c:11:
ecl/ecp_fpinc.c:13:1: warning: "/*" within comment
gcc -o Linux3.8_x86_cc_glibc_PTH_DBG.OBJ/Linux_SINGLE_SHLIB/ecp_fp224.o -c -g -fPIC -Di386 -DLINUX2_1 -m32 -Wall -DNSS_NO_GCC48 -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -UNDEBUG -DDEBUG_jrpierre -D_REENTRANT -DNSS_ECC_MORE_THAN_SUITE_B -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DFREEBL_NO_DEPEND -DNSS_X86_OR_X64 -DNSS_X86 -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE  -DMP_ASSEMBLY_DIV_2DX1D -DMP_USE_UINT_DIGIT -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN -DECL_USE_FP -DMP_API_COMPATIBLE -I../../../dist/Linux3.8_x86_cc_glibc_PTH_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl  ecl/ecp_fp224.c
In file included from ecl/ecp_fp224.c:11:
ecl/ecp_fpinc.c:13:1: warning: "/*" within comment

Did a new version of the C standard change this nested comment issue to be an actual error ? Or is this only an artifact of the different warning level options and compiler that you are using ?

Of course, we are not actually building the above ECL_USE_FP code on a Linux host - but on a Solaris host, with a different compiler, but obviously it also only considers this to be a warning, rather than an error.
(In reply to Wan-Teh Chang from comment #10)

> I looked into this. This change only removes an optimization
> for that curve. That curve is still supported by using the
> default implementation. So this change is fine.

Thanks - I had missed that default case, and also the other high-level callers of this function.
Attachment #8738535 - Flags: superreview?(julien.pierre)
Attachment #8738535 - Flags: review?(franziskuskiefer)
(In reply to Julien Pierre from comment #12)
> 
> Did a new version of the C standard change this nested comment issue to be
> an actual error ? Or is this only an artifact of the different warning level
> options and compiler that you are using ?

Hi Julien: NSS now treats warnings as errors if Visual C++,
clang, or a recent version of gcc is used. This is controlled
by the NSS_ENABLE_WERROR variable:

http://mxr.mozilla.org/nss/search?string=NSS_ENABLE_WERROR
Comment on attachment 8738535 [details] [diff] [review]
Add missing comment delimiter. Reformat comment.

Review of attachment 8738535 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: lib/freebl/ecl/ecp_fpinc.c
@@ -12,2 @@
>  
>  /* Adds a prefix to a given token to give a unique token name. Prefixes

alternatively we can remove this / (not that it would matter)
Attachment #8738535 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8738535 [details] [diff] [review]
Add missing comment delimiter. Reformat comment.

Review of attachment 8738535 [details] [diff] [review]:
-----------------------------------------------------------------

Patch checked in: https://hg.mozilla.org/projects/nss/rev/771069c9816b

::: lib/freebl/ecl/ecp_fpinc.c
@@ -12,2 @@
>  
>  /* Adds a prefix to a given token to give a unique token name. Prefixes

I also considered this. The subject of this paragraph is
completely unrelated to the subject of the previous
paragraph, so I think it is better for these two paragraphs
to be separate comment blocks.
Attachment #8738535 - Flags: checked-in+
Attachment #8738535 - Flags: superreview?(julien.pierre) → superreview+
Attachment #8737781 - Attachment is obsolete: true
Most of ECECL_USE_FP got removed already. Removing the last bits https://hg.mozilla.org/projects/nss/rev/3dfc16480e837b69682f4d857c5e734f21eff3bb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.31
You need to log in before you can comment on or make changes to this bug.