Closed Bug 1290334 Opened 4 years ago Closed 4 years ago

Various issues building on 64-bits Windows

Categories

(NSS :: Build, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(6 files, 2 obsolete files)

No description provided.
Attached patch Support building under MINGW64 (obsolete) — Splinter Review
Attachment #8775948 - Flags: review?(wtc)
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.
Attachment #8775949 - Flags: review?(wtc)
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.
Attachment #8775950 - Flags: review?(wtc)
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.
Attachment #8775954 - Flags: review?(wtc)
Attachment #8775955 - Flags: review?(wtc)
Blocks: 1290616
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 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).
Attachment #8775949 - Flags: review?(wtc) → review+
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.
Attachment #8775950 - Flags: review?(wtc)
Attachment #8775950 - Flags: review?(ttaubert)
Attachment #8775950 - Flags: review?(kaie)
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.
Attachment #8775953 - Flags: review?(wtc) → review+
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!
Attachment #8775954 - Flags: review?(wtc) → review+
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.
(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.
Attachment #8775948 - Attachment is obsolete: true
Attachment #8775948 - Flags: review?(wtc)
Attachment #8775955 - Attachment is obsolete: true
Attachment #8775955 - Flags: review?(wtc)
Attachment #8776736 - Flags: review?(wtc)
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!
Attachment #8776736 - Flags: review?(wtc) → review+
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)))
Attachment #8776733 - Flags: review?(wtc) → review+
Attachment #8775950 - Flags: review?(kaie) → review+
Attachment #8775950 - Flags: review?(ttaubert)
Target Milestone: --- → 3.27
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.
Flags: needinfo?(kaie)
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.
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.
Flags: needinfo?(kaie)
You need to log in before you can comment on or make changes to this bug.