Closed Bug 1350362 Opened 7 years ago Closed 7 years ago

New NSS Build System in Firefox breaks MinGW Build

Categories

(NSS :: Build, enhancement)

Product:
NSS
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(2 files, 1 obsolete file)

Bug 1350362 Fix NSS Build System for MinGW
59 bytes, text/x-review-board-request
ted
: review+
Details
nss-nss.patch
3.28 KB, patch
Details | Diff | Splinter Review 
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)
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.patch — — Splinter 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
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/169d5dfe505f
Fix NSS Build System for MinGW r=ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/169d5dfe505f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: