1290334 - Various issues building on 64-bits Windows

Status: RESOLVED FIXED

(NSS :: Build, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Support building under MINGW64

Comment 2

[Mike Hommey [:glandium]]

Attachment: part 2 - Force-set CPU_ARCH to x386 or x86_64 depending on USE_64

Let's put things as they are: the NSS build system is a mess. It is not
consistent across platforms, and sometimes relies on user input,
sometimes not.

On Windows x86/x86_64, the USE_64 variable essentially chooses between
actual x86 and x86_64. Except there are CPU_ARCH based tests in
cross-platform Makefiles, and CPU_ARCH doesn't necessarily match what
USE_64 says is wanted (not even mentioning what the compiler actually
will generate). For instance, building a 32-bits NSS under MINGW64 will
say CPU_ARCH is x86_64.

This change makes CPU_ARCH match the reality imposed by USE_64 on
Windows.

Comment 3

[Mike Hommey [:glandium]]

Attachment: part 3 - Only build SSE2 chacha20 implementation with GCC/Clang

The SSE2 code in chacha20_vec.c relies on syntax that only builds with
GCC/Clang and not e.g. MSVC. When building with MSVC and CPU_ARCH is
x86_64, the generic non-SSE2 chacha20.c needs to be used.

Only build chacha20_vec.c when the compiler is GCC or Clang.

Comment 4

[Mike Hommey [:glandium]]

Attachment: part 4 - Use $(CURDIR) for PWD when it's a Windows path

Comment 5

[Mike Hommey [:glandium]]

Attachment: part 5 - Use single quotes for core_abspath

Double quotes allow all sorts of escaping, which are not necessarily
wanted. Moreover, it forces make to run a subshell instead of executing
the command directly on its own (because of the possibility of there
being something escaped between the double quotes), and that causes
problems when running under MINGW make with a MSYS shell.

When using single quotes, those problems go away, because make runs the
subprocess without an intermediate shell, and avoids a painful
MINGW->MSYS->MINGW transition.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Force the LINK environment variable to never be set

Comment 7

[Mike Hommey [:glandium]]

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=2480e99d41133910ea4c18007b4a3e9ea9769d9b

Comment 8

[Mike Hommey [:glandium]]

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

Comment 9

[Wan-Teh Chang]

Comment on attachment 8775948 [details] [diff] [review]
Support building under MINGW64

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

Mike: thank you for the patch. I have some questions.

::: coreconf/arch.mk
@@ +210,3 @@
>  # the uname.exe in the MSYS toolkit.
>  #
> +ifneq (,$(filter MINGW%_NT,$(or $(findstring MINGW32_NT,$(OS_ARCH)),$(findstring MINGW64_NT,$(OS_ARCH)))))

This expression seems more complicated than necessary. To match the
comment on line 209:

    # If uname -s returns "MINGW*_NT-*", we assume that we are using
    # the uname.exe in the MSYS toolkit.

it seems that we just need to say:

    ifneq (,$(filter MINGW%_NT,$(OS_ARCH)))

If we only want to allow MINGW32_NT and MINGW64_NT, it seems that we
can just say:

    ifneq (,$(filter MINGW32_NT MINGW64_NT,$(OS_ARCH)))

@@ +225,1 @@
>  	endif

Nit: it should be possible to avoid the nested if statements.
Maybe we should just match the pattern i%86?

Comment 10

[Wan-Teh Chang]

Comment on attachment 8775949 [details] [diff] [review]
part 2 - Force-set CPU_ARCH to x386 or x86_64 depending on USE_64

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

r=wtc.

This change should be good. It is similar to what we do in
coreconf/Darwin.mk:

ifeq (,$(filter-out i%86,$(CPU_ARCH)))
ifdef USE_64
CC              += -arch x86_64
CCC             += -arch x86_64
override CPU_ARCH       = x86_64
else
OS_REL_CFLAGS   = -Di386
CC              += -arch i386
CCC             += -arch i386
override CPU_ARCH       = x86
endif
else
...


and in coreconf/Linux.mk:

ifeq ($(OS_TEST),x86_64)
ifeq ($(USE_64),1)
        CPU_ARCH        = x86_64
        ARCHFLAG        = -m64
else
ifeq ($(USE_X32),1)
        CPU_ARCH        = x86_64
        ARCHFLAG        = -mx32
        64BIT_TAG       = _x32
else
        OS_REL_CFLAGS   = -Di386
        CPU_ARCH        = x86
        ARCHFLAG        = -m32
endif
endif

BUT there are subtle differences. In this patch we change CPU_ARCH
for both x386 and x86_64. In coreconf/Darwin.mk and coreconf/Linux.mk,
we change CPU_ARCH for only one of them (i%86 or x86_64, respectively).

Comment 11

[Wan-Teh Chang]

Comment on attachment 8775950 [details] [diff] [review]
part 3 - Only build SSE2 chacha20 implementation with GCC/Clang

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

Mike: the code you modified was added in Bug 1280596.
Tim, Kai: please review this patch.

Note: I am not sure if CC_IS_GCC should be used outside
coreconf/Werror.mk, but it is already being used in
lib/freebl/Makefile to handle
poly1305-donna-x64-sse2-incremental-source.c, so this is
probably OK.

Comment 12

[Wan-Teh Chang]

Comment on attachment 8775953 [details] [diff] [review]
part 4 - Use $(CURDIR) for PWD when it's a Windows path

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

r=wtc.

::: coreconf/rules.mk
@@ +364,4 @@
>  # Windows
>  ifeq (,$(filter-out _WIN%,$(NS_USE_GCC)_$(OS_TARGET)))
>  NEED_ABSOLUTE_PATH := 1
> +# CURDIR is always an absolute path. If it doesn't start with a /, it's a

Is CURDIR a built-in variable of GNU Make?

@@ +365,5 @@
>  ifeq (,$(filter-out _WIN%,$(NS_USE_GCC)_$(OS_TARGET)))
>  NEED_ABSOLUTE_PATH := 1
> +# CURDIR is always an absolute path. If it doesn't start with a /, it's a
> +# Windows path meaning we're running under MINGW make (as opposed to MSYS
> +# make), or pymake. In both cases, it's preferable to use a Windows path,

I didn't know MINGW make and MSYS make are different.
I thought MINGW is just a compiler, and MSYS is a
suite of Unix commands.

Comment 13

[Wan-Teh Chang]

Comment on attachment 8775954 [details] [diff] [review]
part 5 - Use single quotes for core_abspath

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

r=wtc. I seem to remember I wrote this line. I didn't
consider the differences between double quotes and
single quotes, and I also don't know about the use
of a subshell by make for double quotes. Thanks!

Comment 14

[Wan-Teh Chang]

Comment on attachment 8775955 [details] [diff] [review]
Force the LINK environment variable to never be set

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

Mike: do you think a better fix is to rename our "LINK" makefile variable
to "LD"?  Then we will override LD instead of LINK from the command line,
and will avoid the conflict with MSVC's LINK environment variable.

::: coreconf/WIN32.mk
@@ +24,5 @@
>  	CC           = cl
>  	CCC          = cl
>  	LINK         = link
> +	# MSVC's link.exe allows to pass extra objects to link through the LINK
> +	# environment variable. When running under a shell that has the GNU

Is this the feature?

https://msdn.microsoft.com/en-us/library/6y6t9esh.aspx

Would be good to reference this MSDN page.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Wan-Teh Chang from comment #12)
> Is CURDIR a built-in variable of GNU Make?

Yes

> I didn't know MINGW make and MSYS make are different.
> I thought MINGW is just a compiler, and MSYS is a
> suite of Unix commands.

MINGW also provides many of the unix commands as "native" win32 executables. MINGW make. For instance, is faster than MSYS make because it can use CreateProcess instead of fork()+exec() (fork being slow). It also works with windows paths (c:\foo or c:/foo), instead of using msys paths (/c/foo).(In reply to Wan-Teh Chang from comment #9)

> Comment on attachment 8775948 [details] [diff] [review]
> ::: coreconf/arch.mk
> @@ +210,3 @@
> >  # the uname.exe in the MSYS toolkit.
> >  #
> > +ifneq (,$(filter MINGW%_NT,$(or $(findstring MINGW32_NT,$(OS_ARCH)),$(findstring MINGW64_NT,$(OS_ARCH)))))
> 
> This expression seems more complicated than necessary. To match the
> comment on line 209:
> 
>     # If uname -s returns "MINGW*_NT-*", we assume that we are using
>     # the uname.exe in the MSYS toolkit.
> 
> it seems that we just need to say:
> 
>     ifneq (,$(filter MINGW%_NT,$(OS_ARCH)))

It's MINW*_NT-*, not MINGW*_NT, so that wouldn't work, and you can't have two % in the filter pattern

> If we only want to allow MINGW32_NT and MINGW64_NT, it seems that we
> can just say:
> 
>     ifneq (,$(filter MINGW32_NT MINGW64_NT,$(OS_ARCH)))

... but this would work with an additional % (filter MINGW32_NT% MINGW64_NT%)

(In reply to Wan-Teh Chang from comment #14)
> Comment on attachment 8775955 [details] [diff] [review]
> Force the LINK environment variable to never be set
> 
> Review of attachment 8775955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mike: do you think a better fix is to rename our "LINK" makefile variable
> to "LD"?  Then we will override LD instead of LINK from the command line,
> and will avoid the conflict with MSVC's LINK environment variable.

I'll try that.

> ::: coreconf/WIN32.mk
> @@ +24,5 @@
> >  	CC           = cl
> >  	CCC          = cl
> >  	LINK         = link
> > +	# MSVC's link.exe allows to pass extra objects to link through the LINK
> > +	# environment variable. When running under a shell that has the GNU
> 
> Is this the feature?
> 
> https://msdn.microsoft.com/en-us/library/6y6t9esh.aspx

Yes... and it's actually worse than I thought, since it can pass flags too.

Comment 16

[Mike Hommey [:glandium]]

Attachment: part 1 - Support building under MINGW64

Comment 17

[Mike Hommey [:glandium]]

Attachment: part 6 - Use a LD variable instead of LINK

Comment 18

[Wan-Teh Chang]

Comment on attachment 8776736 [details] [diff] [review]
part 6 - Use a LD variable instead of LINK

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

r=wtc. Thanks!

Comment 19

[Wan-Teh Chang]

Comment on attachment 8776733 [details] [diff] [review]
part 1 - Support building under MINGW64

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

r=wtc. Thanks. I suggest a small change.

::: coreconf/arch.mk
@@ +210,3 @@
>  # the uname.exe in the MSYS toolkit.
>  #
> +ifneq (,$(filter MINGW32_NT% MINGW64_NT%,$(OS_ARCH)))

Nit: I suggest adding a dash '-' to the patterns:

ifneq (,$(filter MINGW32_NT-% MINGW64_NT-%,$(OS_ARCH)))

Comment 20

[Mike Hommey [:glandium]]

https://hg.mozilla.org/projects/nss/rev/bdaef552e501
https://hg.mozilla.org/projects/nss/rev/c27a0cc78376
https://hg.mozilla.org/projects/nss/rev/e9ff703f2282
https://hg.mozilla.org/projects/nss/rev/60c3c9f9a5c0
https://hg.mozilla.org/projects/nss/rev/22bd9fec8978
https://hg.mozilla.org/projects/nss/rev/3f203849a5db

Comment 21

[Gregory Szorc [:gps]]

We need the fixed in this bug in mozilla-central to unblock announcing our new msys2-based Windows development environment (to replace MozillaBuild). What's the ETA on these commits making it to mozilla-central? If it's possible to cherry pick these commits without doing a full NSS sync, that would be acceptable.

Comment 22

[Martin Thomson [:mt:]]

We should land all of NSS for this.  If this is urgent, we could push a 3.27 beta into m-c fairly quickly, but I think that we want to have a little more time to get some security fixes marshalled.

Comment 23

[Kai Engert (:KaiE:)]

Yes, please avoid cherry picking NSS. We usually land a new version of NSS for each Firefox branch. NSS 3.27 is currently under development, and we usually land beta tags into m-c for early testing.

I've filed bug 1296266 to track that.