Get NSS ready for landing on mozilla-central

RESOLVED FIXED in 3.21

Status

NSS
Libraries
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
Created attachment 8675910 [details] [diff] [review]
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.
Attachment #8675910 - Flags: review?(kaie)
(Assignee)

Comment 1

3 years ago
Created attachment 8675911 [details] [diff] [review]
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.
Attachment #8675911 - Flags: review?(kaie)
(Assignee)

Updated

3 years ago
Assignee: nobody → martin.thomson
(Assignee)

Comment 2

3 years ago
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
Attachment #8675910 - Flags: review?(kaie)
Attachment #8675910 - Flags: review+
Attachment #8675910 - Flags: checked-in+
(Assignee)

Comment 3

3 years ago
Created attachment 8675949 [details] [diff] [review]
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.
Attachment #8675949 - Flags: review?(kaie)
(Assignee)

Comment 4

3 years ago
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

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

r=kaie
Attachment #8675911 - Flags: review?(kaie) → review+

Comment 6

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

r=kaie
Attachment #8675949 - Flags: review?(kaie) → review+

Comment 7

3 years ago
Bob, if you have reason to disagree with the patch in attachment 8675949 [details] [diff] [review], please scream.
Flags: needinfo?(rrelyea)

Comment 8

3 years ago
I'm fine with it.

bob
Flags: needinfo?(rrelyea)

Comment 9

3 years ago
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?
(Assignee)

Comment 10

3 years ago
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
Flags: needinfo?(kaie)
(Assignee)

Comment 11

3 years ago
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
(Assignee)

Comment 12

3 years ago
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
(Assignee)

Comment 13

3 years ago
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.
(Assignee)

Comment 14

3 years ago
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
(Assignee)

Comment 16

3 years ago
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
(Assignee)

Comment 17

3 years ago
Created attachment 8677862 [details] [diff] [review]
firefox-changes.patch
Attachment #8677862 - Flags: review?(kaie)
(Assignee)

Comment 18

3 years ago
Created attachment 8677865 [details] [diff] [review]
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.
Attachment #8677865 - Flags: review?(kaie)
(Assignee)

Comment 19

3 years ago
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>
(Assignee)

Updated

3 years ago
OS: Android → All
Hardware: ARM → All
Summary: Get NSS ready for android → Get NSS ready for landing on mozilla-central

Comment 20

3 years ago
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.
(Assignee)

Comment 21

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

Comment 22

3 years ago
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.
Flags: needinfo?(martin.thomson)

Updated

3 years ago
Flags: needinfo?(kaie)

Comment 23

3 years ago
(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.
Flags: needinfo?(martin.thomson)

Comment 24

3 years ago
(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.

Updated

3 years ago
Attachment #8677862 - Flags: review?(kaie) → review+

Updated

3 years ago
Attachment #8677865 - Flags: review?(kaie) → review+
(Assignee)

Updated

3 years ago
Attachment #8677862 - Flags: review?(ted)
(Assignee)

Comment 26

3 years ago
Created attachment 8678278 [details]
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 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!
Attachment #8678278 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8677862 - Flags: review?(ted) → checked-in+
(Assignee)

Comment 29

3 years ago
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
Attachment #8678278 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Blocks: 1211568
(Assignee)

Comment 30

3 years ago
Resolving this now.  Note that I haven't checked in the memcpy patch, I'll do that separately.
No longer blocks: 1211568
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INCOMPLETE

Updated

3 years ago
Resolution: INCOMPLETE → FIXED

Comment 31

3 years ago
(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.

Updated

3 years ago
Blocks: 1218254
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.
(Assignee)

Comment 34

3 years ago
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.

Updated

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

What was the intent?
(Assignee)

Comment 36

3 years ago
(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.
ifndef WARNINGS_AS_ERRORS, then.
You need to log in before you can comment on or make changes to this bug.