324448 - Allow building NSS with MSVC without a standalone assembler

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Benjamin Smedberg]

We'd like to build Mozilla with the MSVC8 "express" free edition, which doesn't come with the standalone assembler. There are two options:

1) (Expedient, but not ideal) : if the standalone assembler is not found by configure, just compile the C version (mpi.c)

2) (The Good and Right fix) Convert mpi_x86.asm to be a .c file with inline assembly (which can be handled by all versions of MSVC).

Comment 1

[Wan-Teh Chang]

I seem to remember that Microsoft said inline assembly is
deprecated.  Unfortunately I don't have the reference for
this statement, nor am I sure that I remember it correctly.

Other than this concern, I am fine with converting mpi_x86.asm
to be a .c file with inline assembly.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We rely on inline assembly in at least xptcall, and I suspect it's widely used by others, so I'm pretty comfortable using that method even if deprecated by MSFT.

Comment 3

[Julien Pierre]

mpi_x86.asm was also used by the OS/2 VACPP build. The OS/2 compilers don't support inline assembly. I don't think the gcc OS/2 build uses this file anymore, though.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We don't have to remove mpi_x86.asm (so as not to break OS2), we can just add mpi_msvc_x86.c or somesuch with the inline assembly.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

IIRC the MS assembler we use is also free.  Assuming (for the sake of this 
discussion) that it is freely and legally available to use, is there any 
objection to using it?

Comment 6

[Wan-Teh Chang]

Nelson: the MS assembler (ml.exe) is hard to find standalone.

MSVC 6.0 does not come with ml.exe, but an easy way to get
it is to install the Processor Pack for MSVC 6.0.  In fact,
this is why the Mozilla build instructions require the
Processor Pack.

MSVC 7.1 (.NET 2003) comes with ml.exe.

Now the Mozilla team needs a solution for MSVC 8 (2005)
Express Edition, which does not come with ml.exe.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The author of the book 
"Windows Assembly Language and Systems Programming
 16- and 32-Bit Low-Level Programming for the PC and Windows"
has made a zip file containing the relevant asermber and linker files 
available on his web site.  His page http://www.goosee.com/goosee/x86/
gives this download link:  http://www.goosee.com/goosee/x86/winmasm.zip
and says: 

"My book mostly gave example code for Windows asm programming using Borland 
TASM. However, all the tools for Microsoft MASM programming are now free, 
and I like MASM better anyhow. So, I've bundled together a complete minimal 
package of tools, including MASM version 6.14, linker, make, resource compiler 
and libraries, plus an example skeleton application. All of the tools and 
libraries were downloaded free from the Internet, and I've included copyright 
notices where appropriate -- please read them and make sure you agree with 
Microsoft's conditions before using the tools."

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

MASM is available "free" in various places, but usually as part of a much larger download (e.g., the Windows SDK, or the DDK).  The zip file on that site includes masm + rc + dependencies, from the Win 98 DDK or possibly the SDK; the EULA's for both are included in the zip file.  I very much doubt that the author there has the legal right to redistribute them in the zip file (in fact, the DDK EULA that's in the zip file expressly prohibits redistribution and has limits on the number of installs etc etc etc).

Even if MASM was available from MS as a tiny single-executable download, it would still be worth doing the inline assembly bits -- this is the only part in the build that requires MASM, and it would be nice to remove that dependency when setting up a build environment.

Comment 9

[Benjamin Smedberg]

The various downloads that offer ml.exe (MASM32 and such) are of dubious legality. ml.exe was offered with the win98 DDK, but that is no longer available from the Microsoft website and the EULA doesn't say that you can redistribute ml.exe

Mozilla could continue to use obviously-legal copies of ml.exe that come with the MSVC6 processor pack (since we own licenses of MSVC6), or with the paid version of MSVC7 or MSVC8, but we would really like to be able to build with the express edition which does not include the standalone assembler.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

The win98 DDK is presently available at
http://download.microsoft.com/download/win98SE/Install/Gold/W98/EN-US/98ddk.exe

It's 26 MB.  It's a self extracting .CAB file.  You can use various cab 
unpacking tools to extract contents without extracting everything, by changing 
the file suffix to .CAB.  This CAB file contains a bunch of .cab files.  
One of them, BINS_DDK.CAB, contains these 3 relevant files:

-  BIN_LINK.EXE         ==> link.exe
-  BIN_WIN98_ML.EXE     ==> ml.exe
-  BIN_WIN98_ML.ERR     ==> ml.err

Extract them and rename them, removing the prefixes.  Voila.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Do you object to this because you think that we should do nothing that can't be worked around with arbitrary build-system contortions, or because you think that it would actually harm NSS to provide an inline-assembly implementation of these elements?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hello?  I'm ready at this point to just check in the nss makefile change to disable the use of the assembly versions of those functions on the trunk.

Comment 13

[Wan-Teh Chang]

I guess Nelson's objection may be because he doens't want
us (the NSS developers) to maintain yet another version of
the same x86 assembly code.  Right now we already have three
versions of that code:
  mpi_i86pc.s	 
  mpi_x86.asm	 
  mpi_x86.s

In any case, without a proposed patch, this bug can't make
any progress.

Comment 14

[Benjamin Smedberg]

I think the plan, if there is one, is to replace mpi_x86.asm with mpi_x86.c using bare functions and inline assembly.

Comment 15

[Wan-Teh Chang]

mpi_x86.asm is also used by OS/2 (with the VACPP compiler).  Since
one of the NSS developers, Julien Pierre, develops on OS/2 and
prefers VACPP to GCC, we may need to keep mpi_x86.asm around.  But
now that I realize mpi_x86.asm will only be used by a handful of
people (probably just Julien), I think the maintenance burden of
mpi_x86.c is only negligibly higher than what we have now.

I also found that we aren't using the x86 assembly code in the
OS/2 GCC build and the Windows MinGW/GCC build.  Does anyone
know if they can take the mpi_x86.s file (for the gas assembler)?
Or would we need to use GCC inline assembly?

Comment 16

[Julien Pierre]

I don't mind if we do away with the mpi_x86.asm . Re: mpi_x86.s, I think there were issues with some assembler directives that weren't accepted by all versions of gas on all operating systems. See bug 241081 .

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

We're adding new assembly code all the time.  
Bob added some just last week.
Perhaps we need a tool/script that can convert a .asm 
or .s into a .c .  Such a contribution would be welcome.

Comment 18

[Wan-Teh Chang]

The cc list shows that there is a lot of interest
in this bug.  I'd appreciate it if one of you could
submit a patch for review.

Comment 19

[Benjamin Smedberg]

Attachment: Switch from .asm to inline __asm

Comment 20

[Benjamin Smedberg]

I merely copied the function bodies from the .asm and wrapped them in naked function decls, no code changes at all.

Comment 21

[Julien Pierre]

Comment on attachment 213495 [details] [diff] [review]
Switch from .asm to inline __asm

Your patch removes mpi_x86.asm . It should also remove all uses of it throughout freebl/Makefile . You did not fix the XP_OS2_VACPP section of the file which uses it. You need to remove the ASFILES statement also, as well as the MP_ASSEMBLY_* from DEFINES in that section.

Comment 22

[Benjamin Smedberg]

Attachment: Switch from .asm to inline __asm, rev. 1.1

Comment 23

[Wan-Teh Chang]

Attachment: Benjamin's patch, rev. 1.2

I made the following two changes to Benjamin's
patch rev. 1.1.

1. White space and comment changes to mpi_x86.c.

2. I changed s_mpv_div_2dx1d to return int instead
of void.  That function is declared to return
mp_err, which is typedef'ed to be int.  I use int
here to avoid including a header file.

3. Benjamin removed the XP_OS2_VACPP section altogether.
I only made the changes Julien described in comment 21.

Comment 24

[Benjamin Smedberg]

It doesn't matter what the declared return type is, since it's (naked), which means that the assembly code manages the return type.

Comment 25

[Wan-Teh Chang]

Attachment: Diffs between mpi_x86.asm and mpi_x86.c

Julien, Nelson, this diff file should help you verify
that the assembly codes in mpi_x86.asm and mpi_x86.c
are the same.

Comment 26

[Julien Pierre]

a) re comment #24, it would still be nice to have the return type listed right there above the assembly code, to see if they are consistent, even though the compiler can't check and error.

b) re comment #23.2 , is there anything wrong with including the header file and returning mp_err ? If you declare the function int, and the mp_err type changes, then we won't know about the inconsistency between the callers of the function and the declaration in the C file (which is expected to match the asm code, but not guaranteed - see a ...) 

c) re comment #23.3, actually Ben's change *might* be better. I think the MP_USE_UINT_DIGIT and DMP_NO_MP_WORD were defined to limit the integer size to match the assembly code in mpi_x86 . But when using the C implementation, it might be best to leave them off altogether. This would need to be tested by someone who has time. It doesn't have to be tested under OS/2 necessarily, comparing rsaperf numbers with gcc 3.2.x with and without those macros on any x86 machine should suffice.

Comment 27

[Wan-Teh Chang]

Julien, I don't have time to do that performance test.
I hope you or Nelson can predict which is better based
on your understanding of MPI.  I will update my patch
accordingly.

The reason I didn't include "mpi.h" is that the only
thing we need from it is a single typedef:

typedef int               mp_err;

To get the declarations of these functions, I'd need
to include "mpi-priv.h".  But "mpi-priv.h" declare
those functions with function parameters, whereas
mpi_x86.c uses empty parameter lists.  This results in
compiler warnings:

cl -FoWIN954.0_DBG.OBJD/WIN95_SINGLE_SHLIB/mpi_x86.obj -c -Od -Z7 -MDd -W3 -nologo -DXP_PC -DSHLIB_SUFFIX=\"dll\" -DSHLIB_PREFIX=\"\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DNSS_ENABLE_ECC -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -D_DEBUG -UNDEBUG -DDEBUG_wtc -DWIN32 -D_WINDOWS -D_X86_ -DWIN95 -DNSS_ENABLE_ECC -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE  -DMP_ASSEMBLY_DIV_2DX1D -DMP_USE_UINT_DIGIT -DMP_NO_MP_WORD -DMP_API_COMPATIBLE -I../../../../dist/WIN954.0_DBG.OBJD/include  -I../../../../dist/public/nss -I../../../../dist/private/nss -Impi -Iecl  C:/nss-ecc-3.11-branch/mozilla/security/nss/lib/freebl/mpi/mpi_x86.c
mpi_x86.c
C:/nss-ecc-3.11-branch\mozilla\security\nss\lib\freebl\mpi\mpi_x86.c(68) : warning C4026: function declared with formal parameter list
C:/nss-ecc-3.11-branch\mozilla\security\nss\lib\freebl\mpi\mpi_x86.c(131) : warning C4026: function declared with formal parameter list
C:/nss-ecc-3.11-branch\mozilla\security\nss\lib\freebl\mpi\mpi_x86.c(197) : warning C4026: function declared with formal parameter list
C:/nss-ecc-3.11-branch\mozilla\security\nss\lib\freebl\mpi\mpi_x86.c(270) : warning C4026: function declared with formal parameter list
C:/nss-ecc-3.11-branch\mozilla\security\nss\lib\freebl\mpi\mpi_x86.c(349) : warning C4026: function declared with formal parameter list

It's better to think of mpi_x86.c as an assembly language file.

Comment 28

[Wan-Teh Chang]

Attachment: Benjamin's patch, rev. 1.3

I made the changes Julien suggested.

1. I removed the XP_OS2_VACPP section completely from
nss/lib/freebl/Makefile, as Benjamin originally did.
I now agree this is better because there is no
XP_OS2_EMX section in that makefile.

2. mpi_x86.c now includes the mpi-priv.h header and
defines the functions with formal parameter lists.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

My concerns with this patch are not the same as Julien's.
I will test the latest patch this weekend.  
I was/am delighted to see how easily the .asm file was 
converted to inline assembler.  
If it passes my various tests, I will give it r+.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213798 [details] [diff] [review]
Benjamin's patch, rev. 1.3

sr+=nelson for the trunk.
I'm satisfied that the output of the .c is effectively identical to the output of the .asm file.

Comment 31

[Wan-Teh Chang]

Nelson, is it okay to check in this patch on the NSS_3_11_BRANCH?
Otherwise, the Mozilla folks won't get this change until they
upgrade to NSS 3.12.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Checked in Benjamin's patch, rev 1.3 on trunk.

Checking in Makefile;        new revision: 1.73; previous revision: 1.72
Checking in mpi/mpi_x86.c;   initial revision: 1.1
Removing    mpi/mpi_x86.asm; new revision: delete; previous revision: 1.2

I'm OK with us making this change on the 3.11 branch, provided that we can
articulate a consistent policy with which this makes sense.

We (NSS team) have recently reaffirmed the desire to avoid unnecessary 
changes to the "stable" 3.11 branch.  We have allowed the NSS trunk and 
NSS 3.11 branch to diverge out of that desire.  See (e.g.) bug 327529 and
bug 326315 comment 4 .  It seems inconsistent with that desire/policy to 
make this change on the 3.11 branch.  I think we need a clear and consistent
set of rules governing the changes that are acceptable for that branch.

IINM, the mozilla client products pull NSS sources from their own branches 
(right?).  Why is the presence of this patch on the 3.11 stable branch a 
prerequisite to this change going into mozilla client products' branches?
Isn't the reason/justification for these various branches precisely to 
decouple the demands of one branch from the needs of another?

If none of the NSS team objects to making this change on the 3.11 branch, 
and if we can articulate a policy in which this change is acceptable on the 
branch (even though none of the products released from that branch needs 
this change) and yet the changes for bug 327529 and bug 326315 are not 
acceptable, then I'm OK with making this change on the 3.11 branch.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

The checkin of "Benjamin's patch, rev 1.3" broke the build on Linux.
with source files named mpi_x86.c and mpi_x86.s, gmake always chose
the .c file, even on Linux.  

So, to workaround this problem, I renamed mpi_x86.c to mpi_x86_asm.c
Hopefully this will fix the builds.  If folks don't like that name,
we can fix it (again) tomorrow.

Checking in Makefile;          new revision: 1.74;   previous revision: 1.73
Removing mpi/mpi_x86.c;        new revision: delete; previous revision: 1.1
Checking in mpi/mpi_x86_asm.c; initial revision: 1.1

Comment 34

[Wan-Teh Chang]

The file name mpi_x86_asm.c is fine; its similarity
to the file name mpi_x86.asm suggests a relationship
between them.  mpi_x86_msvc.c is also a good name.

I wrote a draft of NSS Patch Release Policy at
http://www.mozilla.org/projects/security/pki/nss/devel/patch-release.html
last August.  This bug falls under the category
of build system changes.  Whether this bug is
critical or not is in the eyes of the beholder.
Vladimir considers this bug a blocker.  I consider
this bug an inconvenience to people setting up their
their Mozilla build environment on Windows, and a
major time sink for the Mozilla folks to sort out
the legal issue of redistributing MASM.

Our goal is to move the Mozilla clients to our static
release tags eventually.  Until then, I'm doing my
best to keep the Mozilla branches as close to NSS
release branches as I can.

Comment 35

[Benjamin Smedberg]

So, where does that leave us WRT getting the trunk mozilla build to use a NSS with this patch? (also considering bug 301249).

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Glen, Wan-Teh, If we check this in on the 3.11 branch, do any
FIPS algorithm tests need to be redone?

Comment 37

[Wan-Teh Chang]

Yes, the FIPS RNG algorithm test needs to be redone
on Windows.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

If you're willing to redo that test on windows,
I'm willing to make this change on the 3.11 branch.

Comment 39

[Wan-Teh Chang]

Attachment: Patch for NSS_3_11_BRANCH

I checked in Benjamin's patch on the NSS_3_11_BRANCH,
with the new file named mpi_x86_asm.c.

Checking in Makefile;        new revision: 1.70.2.5; previous revision: 1.70.2.4
Checking in mpi/mpi_x86_asm.c;   new revision: 1.2.2.3
Removing    mpi/mpi_x86.asm; new revision: delete; previous revision: 1.2

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

A little late, but MASM 8 is now available as a free download; info at http://blogs.msdn.com/vcblog/archive/2006/06/08/622485.aspx

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

That blog, in turn, points to this download page:
http://www.microsoft.com/downloads/details.aspx?FamilyId=7A1C9DA0-0510-44A2-B042-7EF370530C64&displaylang=en

Thanks for the info, Vlad!