Closed Bug 1350362 Opened 8 years ago Closed 8 years ago

New NSS Build System in Firefox breaks MinGW Build

Categories

(NSS :: Build, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Tracking Status
firefox55 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(2 files, 1 obsolete file)

The NSS Build System has broken lots of things in the NSS Build for MinGW. Jacek has a partial patch here: https://pastebin.mozilla.org/8983055
That pastebin seems to have expired. When I wrote the GYP files I didn't do any testing with mingw, so I'm not at all surprised . The one thing that might be annoying is that GYP itself doesn't do a very good job of supporting things like mingw, so we may have to hack around that to get it to work.
Here's an updated link - I'm working on ironing these things out to mergable patches. https://pastebin.mozilla.org/8983309
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review134800 ::: security/generate_mapfile.py:26 (Diff revision 1) > > with open(input, 'rb') as f: > for line in f: > line = line.rstrip() > # Remove all lines containing ';-' > - if ';-' in line: > + #if ';-' in line: I don't really understand why you need these changes. ::: security/nss/coreconf/config.gypi:217 (Diff revision 1) > ], > }], > # Shared library specific settings. > [ '_type=="shared_library"', { > 'conditions': [ > - [ 'cc_use_gnu_ld==1', { > + [ 'cc_is_mingw==1 or cc_use_gnu_ld==1', { Seems like you should just be setting the existing `cc_use_gnu_ld` here, since that's actually why you need this, right?
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review134980 ::: security/generate_mapfile.py:26 (Diff revision 1) > > with open(input, 'rb') as f: > for line in f: > line = line.rstrip() > # Remove all lines containing ';-' > - if ';-' in line: > + #if ';-' in line: Sorry, this was a WIP patch. The changes to generate_mapfile.py are all bad and need to be examined; and I will look at your other comment closely also.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review135020 ::: security/nss/coreconf/config.gypi:217 (Diff revision 1) > ], > }], > # Shared library specific settings. > [ '_type=="shared_library"', { > 'conditions': [ > - [ 'cc_use_gnu_ld==1', { > + [ 'cc_is_mingw==1 or cc_use_gnu_ld==1', { 6 of one, half dozen of the other. If I set cc_use_gnu_ld I can avoid the two cc_is_mingw==1 you see here; but I would need to add a cc_is_mingw!=1 in two other locations to avoid running code I don't want to run.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review134800 > I don't really understand why you need these changes. The MinGW build expects a very small, minimal file format for the def. ; isn't a comment in it so we need to rip all those out.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review137430 ::: security/generate_mapfile.py:25 (Diff revision 3) > import buildconfig > > > def main(output, input): > is_darwin = buildconfig.substs['OS_ARCH'] == 'Darwin' > + is_mingw_crosscompile = "WINNT" == buildconfig.substs['OS_ARCH'] and "Linux" == buildconfig.substs['HOST_OS_ARCH'] This could be cleaner and more proper. What you really want to know is if ld is used in Windows (as in Windows target) build. A condition checking cc_use_gnu_ld instead of HOST_OS_ARCH would be better. Also it's not really specific to cross compilation, so I'd remove it from variable name. ::: security/moz.build:115 (Diff revision 3) > # We could probably do so, but not without a lot of > # careful consideration. > 'NO_PGO': True, > } > if CONFIG['OS_TARGET'] == 'WINNT': > - if CONFIG['CPU_ARCH'] == 'x86': > + if CONFIG['CPU_ARCH'] == 'x86' and not gyp_vars['cc_is_mingw']: I think this option is specific to MSVC, so proper check would be CONFIG['_MSC_VER']. ::: security/nss/coreconf/config.gypi:91 (Diff revision 3) > 'dll_suffix': '<(dll_suffix)', > 'freebl_name': '<(freebl_name)', > 'cc_is_clang%': '<(cc_is_clang)', > 'cc_use_gnu_ld%': '<(cc_use_gnu_ld)', > # Some defaults > + 'cc_is_mingw%': 0, I'm not sure that's the best way to handle. AFAICS cc_use_gnu_ld is set to 0 for mingw builds. It should be set, which solves most cc_is_mingw use-cases. Do we have a simple way in gyp files to test if msvc is used? If not, it may be cleaner to introduce something like cc_is_msvc. ::: security/nss/coreconf/config.gypi:176 (Diff revision 3) > [ 'cc_use_gnu_ld==1', { > 'ldflags': [ > '-Wl,--version-script,<(INTERMEDIATE_DIR)/out.>(mapfile)', > ], > }], > - [ 'OS=="win"', { > + [ 'cc_is_mingw!=1 and OS=="win"', { I think it should use cc_use_gnu_ld instead. ::: security/nss/coreconf/config.gypi:218 (Diff revision 3) > ], > }], > # Shared library specific settings. > [ '_type=="shared_library"', { > 'conditions': [ > - [ 'cc_use_gnu_ld==1', { > + [ 'cc_is_mingw==1 or cc_use_gnu_ld==1', { This should not be needed once cc_use_gnu_ld is fixed. ::: security/nss/coreconf/config.gypi:535 (Diff revision 3) > }], > ], > }, > }, > 'conditions': [ > - [ 'cc_use_gnu_ld==1', { > + [ 'cc_is_mingw==1 or cc_use_gnu_ld==1', { Same here. ::: security/nss/lib/freebl/freebl.gyp:205 (Diff revision 3) > +# 'NSS_BEVAND_ARCFOUR', > +# 'MPI_AMD64', > +# 'MP_ASSEMBLY_MULTIPLY', > +# 'NSS_USE_COMBA', > +# 'USE_HW_AES', > +# 'INTEL_GCM', As far as I remember, mingw could use assembly implementations that are covered by those macros and I think makefile-based build system supported that. But I'm fine with keeping it simple.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review137430 > As far as I remember, mingw could use assembly implementations that are covered by those macros and I think makefile-based build system supported that. But I'm fine with keeping it simple. I'd strongly suggest to consider doing this. Without it you'll get pretty slow code.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review141208 ::: security/moz.build:115 (Diff revision 3) > # We could probably do so, but not without a lot of > # careful consideration. > 'NO_PGO': True, > } > if CONFIG['OS_TARGET'] == 'WINNT': > - if CONFIG['CPU_ARCH'] == 'x86': > + if CONFIG['CPU_ARCH'] == 'x86' and not gyp_vars['cc_is_mingw']: Actually, I can build successfully without this. So I'm just not going to touch this. ::: security/nss/lib/freebl/freebl.gyp:205 (Diff revision 3) > +# 'NSS_BEVAND_ARCFOUR', > +# 'MPI_AMD64', > +# 'MP_ASSEMBLY_MULTIPLY', > +# 'NSS_USE_COMBA', > +# 'USE_HW_AES', > +# 'INTEL_GCM', I just realized I don't think this matters, since we do a x86 build. I uncommented the flags and everything worked fine.
Flagging Ted for review as I think this might be ready.
Flags: needinfo?(ted)
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review141656 ::: security/generate_mapfile.py:16 (Diff revision 5) > # > # The NSS build system processes them using a series of sed replacements, > # but the Mozilla build system is already running a Python script to generate > # the file so it's simpler to just do the replacement in Python. > +# > +# One difference between the NSS build system and Mozilla's is that So this doesn't work for a standalone NSS build? What do we need to make that work? ::: security/generate_mapfile.py:40 (Diff revision 5) > continue > # Remove the string ' DATA '. > line = line.replace(' DATA ', '') > # Remove the string ';+' > + if not is_mingw: > - line = line.replace(';+', '') > + line = line.replace(';+', '') nit: tab ::: security/nss/lib/freebl/freebl.gyp:196 (Diff revision 5) > + 'MP_IS_LITTLE_ENDIAN', > + 'NSS_BEVAND_ARCFOUR', > + 'MPI_AMD64', > + 'MP_ASSEMBLY_MULTIPLY', > + 'NSS_USE_COMBA', > + 'USE_HW_AES', Is there a reason this is missing INTEL_GCM? Without it we don't use the fast assembly for AES-GCM.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review141658 Oh, and this will have to be two separate patches eventually. One against NSS for the NSS changes and one against gecko for those changes.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review141656 > nit: tab This file uses spaces actually...
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review141656 > So this doesn't work for a standalone NSS build? What do we need to make that work? After talked with Franziskus, we're not going to spend the time to get NSS completely mingw-crosscompile compatible at this time. > This file uses spaces actually... I double checked and those are spaces.
Attached patch Patch for NSS (obsolete) — Splinter Review
Okay I split the patch in two - one through ReviewBoard for -central and one in a splinter for NSS.
Comment on attachment 8859822 [details] Bug 1350362 Fix NSS Build System for MinGW https://reviewboard.mozilla.org/r/131810/#review142006 ::: security/generate_mapfile.py:30 (Diff revision 7) > + is_mingw = "WINNT" == buildconfig.substs['OS_ARCH'] and buildconfig.substs['GCC_USE_GNU_LD'] > > with open(input, 'rb') as f: > for line in f: > line = line.rstrip() > # Remove all lines containing ';-' Can you update the comment to match? ::: security/moz.build:96 (Diff revision 7) > # in the environment, which isn't true here. I don't know that > # setting that would be harmful, but we already have this information > # anyway. > if CONFIG['CLANG_CXX'] or CONFIG['CLANG_CL']: > gyp_vars['cc_is_clang'] = 1 > + if CONFIG['GNU_CC']: We actually have a more precise variable you can use: `GCC_USE_GNU_LD` https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/build/autoconf/toolchain.m4#46 ::: security/moz.build:119 (Diff revision 7) > if CONFIG['OS_TARGET'] == 'WINNT': > if CONFIG['CPU_ARCH'] == 'x86': > # This should really be the default. > sandbox_vars['ASFLAGS'] = ['-safeseh'] > + if CONFIG['MOZ_FOLD_LIBS_FLAGS']: > + sandbox_vars['CFLAGS'] = [CONFIG['MOZ_FOLD_LIBS_FLAGS']] Since `MOZ_FOLD_LIBS_FLAGS` isn't actually used anywhere else, how about you change it to an `AC_SUBST_LIST` in old-configure.in, and then it'll already be a list here. https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/old-configure.in#5367
Attachment #8859822 - Flags: review?(ted) → review+
Flags: needinfo?(ted)
Keywords: checkin-needed
Comment on attachment 8866951 [details] [diff] [review] Patch for NSS Review of attachment 8866951 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/freebl/freebl_base.gypi @@ +114,5 @@ > 'mpi/mp_comba_amd64_masm.asm', > 'intel-aes-x64-masm.asm', > 'intel-gcm-x64-masm.asm', > ], > + },], nit: remove ,
Attachment #8866951 - Flags: review+
Attached patch nss-nss.patchSplinter Review
Changes },], to }],
Attachment #8866951 - Attachment is obsolete: true
This will land in NSS 3.31. But I'll land a new snapshot of NSS next week so you'll be able to use it.
Target Milestone: --- → 3.31
Now that the NSS part has landed, the other part needs to be checked in.
Keywords: checkin-needed
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: