1216318 - Get NSS ready for landing on mozilla-central

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

Attachment: Simple fix

The changes to sha512.c are sufficient here.

Kai, with these changes, you can remove the NSS_DISABLE_GTESTS environment variable on the android builders.  When we are ready to start testing there, we can just change the code that way.

There is a copy of stlport in the android NDK that we could use to build a cross-compiled version of the gtests, but I think that I'll defer that for some other day.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Groundwork for android c++ builds

This just disables the c++ builds on android.  I could fix the gtests build by using the copy of stlport that the android NDK includes, but that seems like something for another day.

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8675910 [details] [diff] [review]
Simple fix

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

ekr reviewed it and I checked that the actual code is good.  I removed the other HTONL implementations and ran all the sha tests.

https://hg.mozilla.org/projects/nss/rev/46bd290cf6ca

Comment 3

[Martin Thomson [:mt:]]

Attachment: memcpyftw.patch

I stared at the strict aliasing error here for a while and decided that this would be better.  Worst case: performance for DES degrades on some platforms.  I think that we can tolerate that, if indeed the compiler is poor enough that it can't manage to work out what to do.  Who knows, maybe the old code was shielding us from some potential optimizations.

Comment 4

[Martin Thomson [:mt:]]

Kai, with the memcpy patch, this builds on both debug and optimized builds.  I think that I have the same setup as you, so I'd be surprised if the builders complain.

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 8675911 [details] [diff] [review]
Groundwork for android c++ builds

r=kaie

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 8675949 [details] [diff] [review]
memcpyftw.patch

r=kaie

Comment 7

[Kai Engert [:KaiE:]]

Bob, if you have reason to disagree with the patch in attachment 8675949 [details] [diff] [review], please scream.

Comment 8

[Robert Relyea]

I'm fine with it.

bob

Comment 9

[Kai Engert [:KaiE:]]

The try build is failing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58bc598f63a6

We need to add the __PK11_SetCertificateNickname symbol to the firefox nss.def file, but that's trivial.

Our own Android NDK build passed, but the Firefox Android builders show errors, e.g.:

 11:06:05     INFO -  alg1485.c:162:57: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
 11:06:05     INFO -       return (n2k->kind != SEC_OID_UNKNOWN) ? n2k->maxLen : -1;
 11:06:05     INFO -                                                           ^
 11:06:05     INFO -  alg1485.c: In function 'AppendAVA':
 11:06:05     INFO -  alg1485.c:963:40: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
 11:06:05     INFO -       valueLen = (useHex ? avaValue->len :


Is this caused by different compiler flags? I think it would be good to have our own Android builder use the same compiler flags than the Firefox Android builds, allowing us to catch the compiler errors with NSS buildbot.

Or is it a matter of different compilers showing different errors?

Comment 10

[Martin Thomson [:mt:]]

This is related to our use of -Werror.  As you might recall, we discussed landing a different set of flags for Android for this reason.  I don't know why -Wsign-compare is enabled some places and not others, but maybe the easiest thing to do is land some exclusions.

I think that we ultimately want to modify the arguments that the Firefox build uses here.  But that looks difficult:
https://dxr.mozilla.org/mozilla-central/source/config/external/nss/Makefile.in

My ideal solution is to just unconditionally add -Wno-error=sign-compare and then ultimately fix bug 1212199 and remove it.

I tried adding -Wno-error=sign-compare and that didn't work on clang builds, though it might work if we didn't also add -Wsign-compare (which Firefox seems not be adding for builds other than Android/B2G).  You also need to add -Wno-error=type-limits as well.

Perhaps you can just land:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1211725&attachment=8671824

Comment 11

[Martin Thomson [:mt:]]

OK, here's one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e79fd1b56ddd

I added 3.21 with the following patch:

diff --git a/config/external/nss/nss.def b/config/external/nss/nss.def
--- a/config/external/nss/nss.def
+++ b/config/external/nss/nss.def
@@ -412,16 +412,17 @@ PK11_PubEncryptPKCS1
 PK11_PubUnwrapSymKey
 PK11_PubWrapSymKey
 PK11_RandomUpdate
 PK11_ReadRawAttribute
 PK11_ReferenceSlot
 PK11_ResetToken
 PK11SDR_Decrypt
 PK11SDR_Encrypt
+PK11_SetCertificateNickname
 PK11_SetPasswordFunc
 PK11_SetSlotPWValues
 PK11_Sign
 PK11_SignatureLen
 PK11_UnwrapPrivKey
 PK11_UnwrapSymKey
 PK11_UpdateSlotAttribute
 PK11_UserDisableSlot
diff --git a/security/nss/TAG-INFO b/security/nss/TAG-INFO
--- a/security/nss/TAG-INFO
+++ b/security/nss/TAG-INFO
@@ -1,1 +1,1 @@
-NSS_3_20_1_RTM
+NSS_3_21_TEST
diff --git a/coreconf/Linux.mk b/coreconf/Linux.mk
--- a/coreconf/Linux.mk
+++ b/coreconf/Linux.mk
@@ -163,16 +163,17 @@ NSS_HAS_GCC48 := $(shell \
   [ `$(CC) -dumpversion | cut -f 1 -d . -` -eq 4 -a \
     `$(CC) -dumpversion | cut -f 2 -d . -` -ge 8 -o \
     `$(CC) -dumpversion | cut -f 1 -d . -` -ge 5 ] && \
   echo true || echo false)
 export NSS_HAS_GCC48
 endif
 ifeq (true,$(NSS_HAS_GCC48))
 OS_CFLAGS += -Werror
+OS_CFLAGS += -Wno-error=sign-compare -Wno-error=type-limits
 else
 # Old versions of gcc (< 4.8) don't support #pragma diagnostic in functions.
 # Use this to disable use of that #pragma and the warnings it suppresses.
 OS_CFLAGS += -DNSS_NO_GCC48
 $(warning Unable to find gcc >= 4.8 disabling -Werror)
 endif
 
 ifdef USE_PTHREADS

Comment 12

[Martin Thomson [:mt:]]

OK, several errors there.  I needed to add __PK11_SetCertificateNickname, the underscores being important.  I also missed -Wno-error=empty-body.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f987a300aa

Comment 13

[Martin Thomson [:mt:]]

Gah, almost there.

Clang chokes on the extra arguments that the Firefox build passes in because it doesn't understand -Wno-error.  It looks like I will have to disable -Werror on clang.

Also, Android lollipop complains about our use of sprintf (instead of snprintf), which is very annoying because it doesn't appear to be tied to a particular flag and so I don't know how to disable it.

Comment 14

[Martin Thomson [:mt:]]

OK, more changes needed.

We need to disable -Werror on android to deal with the lollipop issue.  Drop the above change for coreconf/Linux.mk.

I have a hack for the clang thing too.

(Fun thing that I discovered, we have three instances of the -Qunused-arguments options when compiling Firefox using clang.)

diff --git a/config/external/nss/Makefile.in b/config/external/nss/Makefile.in
--- a/config/external/nss/Makefile.in
+++ b/config/external/nss/Makefile.in
@@ -208,17 +208,17 @@ DEFAULT_GMAKE_FLAGS += \
 
 # Android has pthreads integrated into -lc, so OS_PTHREAD is set to nothing
 ifeq ($(OS_TARGET), Android)
 DEFAULT_GMAKE_FLAGS += \
 	OS_RELEASE='2.6' \
 	OS_PTHREAD= \
 	$(NULL)
 
-DEFAULT_GMAKE_FLAGS += ARCHFLAG='$(CFLAGS) -DCHECK_FORK_GETPID $(addprefix -DANDROID_VERSION=,$(ANDROID_VERSION)) -include $(topsrcdir)/security/manager/android_stub.h'
+DEFAULT_GMAKE_FLAGS += ARCHFLAG='$(filter-out -W%,$(CFLAGS)) -DCHECK_FORK_GETPID $(addprefix -DANDROID_VERSION=,$(ANDROID_VERSION)) -include $(topsrcdir)/security/manager/android_stub.h'
 endif
 endif
 
 ifdef WRAP_LDFLAGS
 NSS_EXTRA_LDFLAGS += $(WRAP_LDFLAGS)
 endif
 
 ifdef MOZ_GLUE_WRAP_LDFLAGS
@@ -236,17 +236,17 @@ DEFAULT_GMAKE_FLAGS += FREEBL_NO_DEPEND=
 ifeq ($(OS_TARGET),Linux)
 DEFAULT_GMAKE_FLAGS += FREEBL_LOWHASH=1
 endif
 
 ifdef MOZ_NO_WLZDEFS
 DEFAULT_GMAKE_FLAGS += ZDEFS_FLAG=
 endif
 ifdef MOZ_CFLAGS_NSS
-DEFAULT_GMAKE_FLAGS += XCFLAGS='$(CFLAGS)'
+DEFAULT_GMAKE_FLAGS += XCFLAGS='$(filter-out -W%,$(CFLAGS))'
 DEFAULT_GMAKE_FLAGS += DARWIN_DYLIB_VERSIONS='-compatibility_version 1 -current_version 1 $(LDFLAGS)'
 endif
 ifeq (1_1,$(CLANG_CL)_$(MOZ_ASAN))
 XLDFLAGS := $(OS_LDFLAGS)
 DEFAULT_GMAKE_FLAGS += XLDFLAGS='$(XLDFLAGS)'
 endif
 
 DEFAULT_GMAKE_FLAGS += NSS_NO_PKCS11_BYPASS=1
diff --git a/coreconf/Linux.mk b/coreconf/Linux.mk
--- a/coreconf/Linux.mk
+++ b/coreconf/Linux.mk
@@ -162,17 +162,22 @@ ifndef NSS_HAS_GCC48
 NSS_HAS_GCC48 := $(shell \
   [ `$(CC) -dumpversion | cut -f 1 -d . -` -eq 4 -a \
     `$(CC) -dumpversion | cut -f 2 -d . -` -ge 8 -o \
     `$(CC) -dumpversion | cut -f 1 -d . -` -ge 5 ] && \
   echo true || echo false)
 export NSS_HAS_GCC48
 endif
 ifeq (true,$(NSS_HAS_GCC48))
+# Android lollipop generates the following warning:
+# error: call to 'sprintf' declared with attribute warning: sprintf is often misused; please use snprintf [-Werror]
+# So, just suppress -Werror entirely
+ifneq ($(OS_TARGET),Android)
 OS_CFLAGS += -Werror
+endif
 else
 # Old versions of gcc (< 4.8) don't support #pragma diagnostic in functions.
 # Use this to disable use of that #pragma and the warnings it suppresses.
 OS_CFLAGS += -DNSS_NO_GCC48
 $(warning Unable to find gcc >= 4.8 disabling -Werror)
 endif
 
 ifdef USE_PTHREADS

Comment 15

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2ea40e6169e

Comment 16

[Martin Thomson [:mt:]]

OK, this was a lot harder than I had expected.  The Firefox build system is much, much worse than I had even imagined.  I'll attach the patch that looks like it is working here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e953f49ed722

Comment 17

[Martin Thomson [:mt:]]

Attachment: firefox-changes.patch

Comment 18

[Martin Thomson [:mt:]]

Attachment: nss-changes.diff

Sadly, this required a rework of the -Werror stuff.

This disabled -Werror on Android due to all the trouble that it caused.  I also had to hack the warning options to get clang to compile cleanly.

Comment 19

[Martin Thomson [:mt:]]

Summary for Kai:

Look at the two new patches, they cause everything to compile correctly.  One is for nss, the other for gecko.  I didn't include the TAG-INFO change: the script should handle that.

Changes are:
  Firefox:
    - Add the missing function to nss.def
    - Filter out -Wxxx options from CFLAGS
  NSS:
    - Disable -Werror on Android entirely
    - Change the way that -Wno-xxx options are set because we use warnings that some versions of clang don't understand and -Werror causes it to error out on -Wno-<unsupported warning>

Comment 20

[:Cykesiopka]

Comment on attachment 8677862 [details] [diff] [review]
firefox-changes.patch

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

::: config/external/nss/nss.def
@@ +681,5 @@
>  VFY_VerifyData
>  VFY_VerifyDataWithAlgorithmID
>  VFY_VerifyDigestDirect
>  _SGN_VerifyPKCS1DigestInfo
> +__PK11_SetCertificateNickname

Drive by nit: It would be nice if you could sort this alphabetically, since most of the file is sorted as such.

Comment 21

[Martin Thomson [:mt:]]

Cykesiopka, unfortunately, this is alphabetical.  Or would you prefer I ignore the underscores?

Comment 22

[Kai Engert [:KaiE:]]

disable_warning=$(shell $(CC) -x c -E -Werror -W$(1) /dev/null >/dev/null 2>&1 && echo -Wno-$(1))

If I understand correctly, this calls
  clang -x c -E -Werror -Wsomething
without further parameters.

In my testing, I tried both a parameter value that it understands -Weverything, and one that is invalid -Wbla. Both times I get a failure results on the shell, echo $? prints 1.

If it really works, can you please explain what I'm missing? Thanks.

Comment 23

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #22)
> If it really works, can you please explain what I'm missing? Thanks.

never mind, I missed the /dev/null parameter, thanks to Martin for explaining on IRC.

Comment 24

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #21)
> Cykesiopka, unfortunately, this is alphabetical.  Or would you prefer I
> ignore the underscores?

Given there's already a symbol with leading underscore added at the end, which didn't ignore underscore for the order, it seems to ok to add the new one at the end, too.

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 8677865 [details] [diff] [review]
nss-changes.diff

https://hg.mozilla.org/projects/nss/rev/feae8cff7fed
https://hg.mozilla.org/projects/nss/rev/02eb5f0c5d80

Comment 26

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1216318 - Disable warnings as errors in NSS by default, r?ted

Bug 1216318 - Disable warnings as errors in NSS by default, r?ted

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 8678278 [details]
MozReview Request: Bug 1216318 - Disable warnings as errors in NSS by default, r?ted

https://reviewboard.mozilla.org/r/23155/#review20643

Looks good, thanks!

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23856d260d53

Comment 29

[Martin Thomson [:mt:]]

Comment on attachment 8678278 [details]
MozReview Request: Bug 1216318 - Disable warnings as errors in NSS by default, r?ted

https://hg.mozilla.org/integration/mozilla-inbound/rev/23856d260d53

Comment 30

[Martin Thomson [:mt:]]

Resolving this now.  Note that I haven't checked in the memcpy patch, I'll do that separately.

Comment 31

[:Cykesiopka]

(In reply to Martin Thomson [:mt:] from comment #21)
> Cykesiopka, unfortunately, this is alphabetical.  Or would you prefer I
> ignore the underscores?

Yes, but I guess it doesn't matter now.

Thanks anyways.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23856d260d53

Comment 33

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/23155/#review20847

::: config/external/nss/Makefile.in:270
(Diff revision 1)
> +ifeq (1,$(ALLOW_COMPILER_WARNINGS))

If the intent here is to have NSS use -Werror following --enable-warnings-as-errors from Gecko's configure, it's the wrong test.
If the intent here is to follow the value set in the corresponding moz.build, it's the wrong test as well.

In practice, -Werror is never forcefully disabled by Gecko's build system.

While here, I'm mention that the $(shell) business in Linux.mk and Darwin.mk are incurring significant overhead on Gecko no-op builds.

Comment 34

[Martin Thomson [:mt:]]

What would the right test be?  I was only following guidance here and am happy to do whatever work is needed to make this better.

See bug 1211568 for the performance issues.  We might be able to tweak the gecko build to avoid any overheads based on that.

Comment 35

[Mike Hommey [:glandium]]

(In reply to Martin Thomson [:mt:] from comment #34)
> What would the right test be?

What was the intent?

Comment 36

[Martin Thomson [:mt:]]

(In reply to Mike Hommey [:glandium] from comment #35)
> (In reply to Martin Thomson [:mt:] from comment #34)
> > What would the right test be?
> 
> What was the intent?

The intent is to enable the -Werror=xxx stuff that NSS has if someone has --enable-warnings-as-errors and not otherwise, but to also continue to respect the override in moz.build.  I think that is what we ended up on in IRC.

If you prefer something else, I'm open to it.  Basically, I only need -Werror for when I'm operating on NSS standalone and don't mind which way this goes when it is part of Firefox proper.  Of course, I have a general preference for more -Werror over less.

Comment 37

[Mike Hommey [:glandium]]

ifndef WARNINGS_AS_ERRORS, then.