947653 - Enable ECC by default and add an option NSS_DISABLE_ECC to disable it

Status: RESOLVED FIXED

(NSS :: Build, enhancement, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Now that Red Hat is shipping ECC-enabled NSS, all the major NSS contributors have NSS enabled by default, so let's make ECC the default, so that contributors to NSS are hacking on a configuration more similar to the configurations the core NSS team are using every day.

In particular, currently if you forget to specify NSS_ENABLE_ECC=1 when running the test suite, then we won't run any of the tests that require ECC. This means we can easily overlook failures that only occur in ECC functionality by simply forgetting NSS_ENABLE_ECC=1, which I've done a few times now. ECC on by default makes this more bulletproof.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Update build system to enable ECC by default (use NSS_DISABLE_ECC instead of NSS_ENABLE_ECC)

This first patch causes us to enable ECC (including the #define of NSS_ENABLE_ECC) by default, unless the environment variable NSS_DISABLE_ECC is set.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Replace the NSS_ENABLE_ECC macro with a NSS_DISABLE_ECC macro in C code

This patch is basically:

   s/#ifdef NSS_ENABLE_ECC/#ifndef NSS_DISABLE_ECC/g
   s|/* NSS_ENABLE_ECC */|/* NSS_DISABLE_ECC*|g

Additionally, it changes the build system to #define NSS_DISABLE_ECC when the NSS_DISABLE_ECC environment variable is defined.

Comment 3

[Wan-Teh Chang]

Comment on attachment 8344243 [details] [diff] [review]
Part 1: Update build system to enable ECC by default (use NSS_DISABLE_ECC instead of NSS_ENABLE_ECC)

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

I skimmed through this patch. It seems correct. I'll let Bob do the review.

::: automation/buildbot-slave/bbenv-example.sh
@@ +22,1 @@
>  export NSS_ECC_MORE_THAN_SUITE_B=1

Maybe NSS_ECC_MORE_THAN_SUITE_B=1 should also be removed.

::: coverage/cov.sh
@@ +35,1 @@
>  export NSS_ECC_MORE_THAN_SUITE_B=1

Maybe we should also remove NSS_ECC_MORE_THAN_SUITE_B.

Comment 4

[Wan-Teh Chang]

Comment on attachment 8344244 [details] [diff] [review]
Part 2: Replace the NSS_ENABLE_ECC macro with a NSS_DISABLE_ECC macro in C code

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

r=wtc.

Given the size of this patch and the build option change, I think we
should fix this bug in NSS 3.16.

::: lib/softoken/pkcs11c.c
@@ +4903,5 @@
>  	    param = SEC_ASN1EncodeItem(NULL, NULL, &(lk->u.dsa.params),
>  				       nsslowkey_PQGParamsTemplate);
>  	    algorithm = SEC_OID_ANSIX9_DSA_SIGNATURE;
>  	    break;
> +#ifndef NSS_DISABLE_ECC	    

Nit: remove the spaces at the end of line.

Comment 5

[Wan-Teh Chang]

Comment on attachment 8344243 [details] [diff] [review]
Part 1: Update build system to enable ECC by default (use NSS_DISABLE_ECC instead of NSS_ENABLE_ECC)

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

This patch is missing the change to coreconf/config.mk, which your
other patch depends on:

ifndef NSS_DISABLE_ECC
DEFINES += -DNSS_ENABLE_ECC
endif

Comment 6

[Robert Relyea]

Comment on attachment 8344244 [details] [diff] [review]
Part 2: Replace the NSS_ENABLE_ECC macro with a NSS_DISABLE_ECC macro in C code

r+

At some point we can probably remove the ifndef DISABLE_ECC from nss proper, just leave the ones in softoken. Elio, it might be good to reach out to debian and see if that would cause headaches for them. Also, Elio, I thing we have a redhat upstream patch for removing ECC ifdefs from certutil and have certutil detect ECC capability on the fly.

None of these suggestions need hold-up this patch.

bob

Comment 7

[Robert Relyea]

> Maybe NSS_ECC_MORE_THAN_SUITE_B=1 should also be removed.

Not being up on the test/build system, I'm not sure which way to go on this. If you only build one flavor, then we should build the flavor without NSS_ECC_MORE_THAN_SUITE_B set, since that is the most common deployment of NSS these days. If we build multiple flavors, at least one should have NSS_ECC_MORE_THAN_SUITE_B set.


bob

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8344243 [details] [diff] [review]
Part 1: Update build system to enable ECC by default (use NSS_DISABLE_ECC instead of NSS_ENABLE_ECC)

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

::: automation/buildbot-slave/bbenv-example.sh
@@ +22,1 @@
>  export NSS_ECC_MORE_THAN_SUITE_B=1

Good idea. I did remove it.

::: coverage/cov.sh
@@ +35,1 @@
>  export NSS_ECC_MORE_THAN_SUITE_B=1

I did not remove this one because this seems like a coverage test that is specific to Sun/Oracle and I'm guessing they want it to be tested with NSS_ECC_MORe_THAN_SUITE_B.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #5)
> This patch is missing the change to coreconf/config.mk, which your
> other patch depends on:
> 
> ifndef NSS_DISABLE_ECC
> DEFINES += -DNSS_ENABLE_ECC
> endif

The change to coreconf/config.mk is in the other patch:

-ifndef NSS_DISABLE_ECC
-DEFINES += -DNSS_ENABLE_ECC
+ifdef NSS_DISABLE_ECC
+DEFINES += -DNSS_DISABLE_ECC
 endif

I agree the way that the patches is split up in this respect is confusing. I will combine the two patches together when I check them in.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Update build system to enable ECC by default (use NSS_DISABLE_ECC instead of NSS_ENABLE_ECC) [rebased to current tip]

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Replace the NSS_ENABLE_ECC macro with a NSS_DISABLE_ECC macro in C code [rebased to tip, wtc's nit addressed]

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: replace-NSS_ENABLE_ECC-with-NSS-DISABLE_ECC-code.patch [v3, really rebased to tip now]

Forgot to "hg qrefresh" when I attached part 2.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8383419 [details] [diff] [review]
Part 1: Update build system to enable ECC by default (use NSS_DISABLE_ECC instead of NSS_ENABLE_ECC) [rebased to current tip]

Elio, in email Bob suggested that I have you do this review instead of him. This is something we already agreed to do in the NSS teleconference a few weeks ago and in the discussions above.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for the quick review. I folded both patches together and committed them together:

http://hg.mozilla.org/projects/nss/rev/d4365c889186

I did the following sanity checks before checking the patch in:

1. I verified that no more instances of "NSS_ENABLE_ECC" appear in the source code.

2. I verified the test suite passes, with no ECC tests run, with:

       NSS_DISABLE_ECC=1 make clean all
       cd tests
       NSS_DISABLE_ECC=1 ./all.sh

3. I verified that the SSL tests pass, with the ECC tests run, with:
       make clean all
       cd tests
       NSS_TESTS=ssl ./all.sh

(In reply to Robert Relyea from comment #6)
> At some point we can probably remove the ifndef DISABLE_ECC from nss proper,
> just leave the ones in softoken.

I filed bug 978637 to remove the compile-time checks from libssl (and libnss too), and bug 978638 for removing the compile-time checks from the tools.

> Also, Elio, I thing
> we have a redhat upstream patch for removing ECC ifdefs from certutil and
> have certutil detect ECC capability on the fly.

Please attach the patch to bug 978638.