1350362 - New NSS Build System in Firefox breaks MinGW Build

Status: RESOLVED FIXED

(NSS :: Build, enhancement)

Description

[Tom Ritter [:tjr]]

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

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Tom Ritter [:tjr]]

Here's an updated link - I'm working on ironing these things out to mergable patches. https://pastebin.mozilla.org/8983309

Comment 3

[Tom Ritter [:tjr]]

Attachment: Bug 1350362 Fix NSS Build System for MinGW

Review commit: https://reviewboard.mozilla.org/r/131810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131810/

Comment 4

[(not currently active) Ted Mielczarek]

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 5

[Tom Ritter [:tjr]]

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 6

[Tom Ritter [:tjr]]

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 7

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/1-2/

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/2-3/

Comment 9

[Tom Ritter [:tjr]]

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 10

[Jacek Caban]

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 11

[Franziskus Kiefer [:franziskus]]

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 12

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/3-4/

Comment 13

[Tom Ritter [:tjr]]

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.

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/4-5/

Comment 15

[Tom Ritter [:tjr]]

Flagging Ted for review as I think this might be ready.

Comment 16

[Franziskus Kiefer [:franziskus]]

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 17

[Franziskus Kiefer [:franziskus]]

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 18

[(not currently active) Ted Mielczarek]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

https://reviewboard.mozilla.org/r/131810/#review141674

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

https://reviewboard.mozilla.org/r/131810/#review141738

::: 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(';+', '')

This file uses spaces actually...

Comment 20

[Tom Ritter [:tjr]]

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 21

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/5-6/

Comment 22

[Tom Ritter [:tjr]]

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.

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/6-7/

Comment 24

[Tom Ritter [:tjr]]

Attachment: Patch for NSS

Okay I split the patch in two - one through ReviewBoard for -central and one in a splinter for NSS.

Comment 25

[(not currently active) Ted Mielczarek]

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

Comment 26

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/7-8/

Comment 27

[Tom Ritter [:tjr]]

Comment on attachment 8859822 [details]
Bug 1350362 Fix NSS Build System for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131810/diff/8-9/

Comment 28

[Franziskus Kiefer [:franziskus]]

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 ,

Comment 29

[Tom Ritter [:tjr]]

Attachment: nss-nss.patch

Changes },], to }],

Comment 30

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/66268ec0eda0956937d66270999ca6e0fc424862
Bug 1350362 - Support building NSS in Firefox with MinGW on Linux, r=franziskus

Comment 31

[Franziskus Kiefer [:franziskus]]

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.

Comment 32

[Tom Ritter [:tjr]]

Now that the NSS part has landed, the other part needs to be checked in.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/169d5dfe505f
Fix NSS Build System for MinGW r=ted

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/169d5dfe505f