329058 - mpmontg.c doesn't compile when MP_CHAR_STORE_SLOW is defined

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

As an experiment, I defined MP_CHAR_STORE_SLOW in a build for Windows.
The result was a failed build.  As I looked into it, I found several
errors, all of which had to be fixed before it would build.  This makes
me think that we are not using the MP_CHAR_STORE_SLOW code on any 
platform!  Can that be right, Bob?

Here are the problems I encountered with that symbol defined.

1. I encoutered an error from this bit of code in mpmontg.c
#if !defined(MPI_IS_BIG_ENDIAN) && !defined(MPI_IS_LITTLE_ENDIAN)
#error "You must define MPI_IS_BIG_ENDIAN or MPI_IS_LITTLE_ENDIAN\n" \
       "  if you define MP_CHAR_STORE_SLOW."

This test is inside an ifdef for MP_CHAR_STORE_SLOW.
This test and message are incorrect.  The code below it NEVER actually
tests for either MPI_IS_BIG_ENDIAN or MPI_IS_LITTLE_ENDIAN.  The code
below it DOES check for IS_LITTLE_ENDIAN.  

According to LXR, there are no places in NSS that actually define 
either of those two strings, MPI_IS_BIG_ENDIAN or MPI_IS_LITTLE_ENDIAN.
mpi's own "target.mk" makefile (used only in standalone builds of mpi)
defines MP_IS_LITTLE_ENDIAN, which doesn't satsify the above test.

2. When MP_CHAR_STORE_SLOW is defined, the code attempts to use an 
undefined symbol, namely, MP_DIGIT_BITS.  MP_DIGIT_BITS is not defined
in mpi's standard header files.  (MP_DIGIT_BIT (no S) is defined).
MP_DIGIT_BITS is defined in an ECC header file, but we don't want the
main mpi sources to depend on the additional (and optional) ecc headers.

Consequently, I think if NSS was defining MP_CHAR_STORE_SLOW for any
platform, the build would fail on that platform.  And so I conclude
that NSS 3.11 is not actually using MP_CHAR_STORE_SLOW at all.  
That result is VERY surprising.

Because NSS is apparently not using MP_CHAR_STORE_SLOW, I don't think
this bug is necessarily P1, and I will mark it "normal" severity.
But I could be persuaded to raise the priority & severity.

I wrote a patch that fixed these problems, so that the code would at 
least compile with MP_CHAR_STORE_SLOW defined, but I have not tested 
the build with that patch for correctness.  

The fix has two main parts in mpmontg.c:
1. change the test from MPI_IS_xxxxxx_ENDIAN to just IS_xxxxxx_ENDIAN
2. change MP_DIGIT_BITS to MP_DIGIT_BIT wherever it appears in that file.

Patch forthcoming.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

marking p2 for now.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 (untested)

This patch contains some changes to some files that are only used as part of
a stand-alone build of MPI, and are not used in NSS builds.  The changes to
those files need not be reviewed for NSS purposes.

In addition to these changes, I wonder if we need to change one or more NSS
makefiles to define MP_CHAR_STORE_SLOW for certain platforms (AMD64).

Comment 3

[Robert Relyea]

Comment on attachment 213715 [details] [diff] [review]
patch v1 (untested)

r+ = relyea

Comment 4

[Robert Relyea]

Comment on attachment 213715 [details] [diff] [review]
patch v1 (untested)

please review, I'd like to see this in 3.11

Comment 5

[Robert Relyea]

I've tested the mpmontg.c code on linux 32 and 64 bit.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)
> I've tested the mpmontg.c code on linux 32 and 64 bit.

Thanks for testing this patch.  Just to satisfy my curiosoty, have you 
verified that MP_CHAR_STORE_SLOW was defined for the 64-bit linux, and 
that it worked and performed as expected? 

Comment 7

[Wan-Teh Chang]

Comment on attachment 213715 [details] [diff] [review]
patch v1 (untested)

r=wtc.  Please make the suggested changes below
when you check this in.

Question: when mpmontg.c is compiled as part of
lib/freebl (as opposed a standalone mpi build),
who defines the IS_LITTLE_ENDIAN or IS_BIG_ENDIAN
macro?  NSPR header files define one of these
macros, but mpmontg.c doesn't include any NSPR
header file.  I don't understand why you want to
check in a patch that only fixes the standalone mpi
build.

In target.mk, please remove the following unused
new code:

>+# define common file name suffixes
>+A=a
>+O=o
>+Fo = "-o "

and in the new ifeq ($(TARGET),WIN32) section:

>+A=lib
>+O=obj
>+CC=cl
>+EXE=.exe
>+Fo = -Fo

Please remove the following commented-out code:

>+# cl -FoWIN954.0_DBG.OBJD/WIN95_SINGLE_SHLIB/mpi.obj -c 
>+
>+#ml.exe -FoWIN954.0_DBG.OBJD/WIN95_SINGLE_SHLIB/mpi_x86.obj -Cp -Sn -Zi -coff
>+#-I../../../../dist/WIN954.0_DBG.OBJD/include  -I../../../../dist/public/nss
>+#-I../../../../dist/private/nss -Impi -Iecl -c mpi/mpi_x86.asm

Comment 8

[Wan-Teh Chang]

Comment on attachment 213715 [details] [diff] [review]
patch v1 (untested)

If the goal is to fix the standalone mpi build,
it may be better to change MPI_IS_BIG_ENDIAN,
MPI_IS_LITTLE_ENDIAN, and IS_LITTLE_ENDIAN to
MP_IS_BIG_ENDIAN, MP_IS_LITTLE_ENDIAN, and
MP_IS_LITTLE_ENDIAN respectively in mpmontg.c,
and not change -DMP_IS_LITTLE_ENDIAN to
-DIS_LITTLE_ENDIAN in target.mk.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

My intent in creating this patch was NOT merely (or even primarily) to fix 
the stand-alone builds.  It was primarily to make MP_CHAR_STORE_SLOW work 
(that is, compile cleanly) when built in NSS, and to make Bob's consistency checks (must define little or big) actually check for the presence of the 
feature test macro upon which the code actually depends, rather than test
for a macro that is irrelevant.  

I did this under the (mistaken?) belief that Bob had planned and intended 
that NSS would actually use MP_CHAR_STORE_SLOW on at least one platform. But 
now I am not sure that Bob actually intends for NSS to use MP_CHAR_STORE_SLOW 
on any platform in NSS.  (Bob, can you confirm your intent?)

Wan-Teh, I think your observation that the file never includes any header 
that might define IS_LITTLE_ENDIAN is a crucial, and amounts to r- for this
patch.  It implies that, even now, when built as part of NSS, 
MP_CHAR_STORE_SLOW is not being defined on any platform (or else the 
consistency check for little or big would fail).  In turn, that implies
that this patch is incomplete, because it fails to have NSS actually use
MP_CHAR_STORE_SLOW.   

If Bob's intent is to NOT use MP_CHAR_STORE_SLOW on any platform, then I
question the point of retaining that code.  But I doubt that is Bob's 
intent.  I'd sooner guess that the omission of MP_CHAR_STORE_SLOW is an
oversight, since that feature seems crucial for AMD64 (if nothing else).

Comment 10

[Wan-Teh Chang]

Comment on attachment 213715 [details] [diff] [review]
patch v1 (untested)

Nelson,

Bob can answer the questions authoritatively, but I've studied
the code well enough that I can reconstruct Bob's intent.

In mpmontg.c, we should make the following changes:

  MPI_IS_BIG_ENDIAN     =>  MP_IS_BIG_ENDIAN
  MPI_IS_LITTLE_ENDIAN  =>  MP_IS_LITTLE_ENDIAN
  IS_LITTLE_ENDIAN      =>  MP_IS_LITTLE_ENDIAN

The reason is that all of mpi's configuration macros have the
MP_ prefix, and target.mk has two instances of
-DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN (in the Linux x86
and Solaris AMD64 sections) for the standalone mpi build.

Then, we should add -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN
to the corresponding Linux x86 and Solaris AMD64 sections in
lib/freebl/Makefile, if testing shows that this is a win.

Comment 11

[Robert Relyea]

Yes, though I was comparing debug versus debug builds. yesterday I tried to build optimized and found another build issue. With that fixed, thing run really well.

bob

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

So, Bob, in light of the other build issue you found, please create a new 
patch that uses MP_CHAR_STORE_SLOW and builds and works best if your tests.

Comment 13

[Robert Relyea]

The intent was definately to use the code in NSS. The issue about endianness was discussed long and hard and the compromise was that any platform that defined MP_CHAR_STORE_SLOW would also define the endianness.

I really don't care what the defines say. I had conditional includes of nspr to get the endianness, but reviewers requested I remove it;).

bob

Comment 14

[Robert Relyea]

Attachment: "Sample patch". This patch contains other changes to the Linux build I'm playing with.

I'll attach a patch suitable for review in  a moment.

Comment 15

[Robert Relyea]

Note, there are bugs in the Makefile above, This is the current Makefile I'm playing with, and includes attempts at building intel and amd specific versions of mpi (with currently bad choices of names).

Comment 16

[Robert Relyea]

Comment on attachment 214567 [details] [diff] [review]
"Sample patch". This patch contains other changes to the Linux build I'm playing with.

Ok, this patch is completely useless...

Comment 17

[Robert Relyea]

Attachment: CHECKED IN: Patch, including Nelson's changes, wt'c update, and a Makefile changes to turn it on in Linux

Comment 18

[Robert Relyea]

Comment on attachment 214605 [details] [diff] [review]
CHECKED IN: Patch, including Nelson's changes, wt'c update, and a Makefile changes to turn it on in Linux

ok, this patch looks right.

bob

Comment 19

[Wan-Teh Chang]

Comment on attachment 214605 [details] [diff] [review]
CHECKED IN: Patch, including Nelson's changes, wt'c update, and a Makefile changes to turn it on in Linux

r=wtc.  Please remember to make the changes I described
in comment 7 to freebl/mpi/target.mk.

It would be nice to keep the definition of
-DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN in synch
between freebl/Makefile and freebl/mpi/target.mk.  In
freebl/Makefile, these macros are defined in the Linux
x86_64 and Linux x86 sections, while in freebl/mpi/target.mk,
these macros are defined in the Linux x86 and Solaris AMD64
sections.

Comment 20

[Wan-Teh Chang]

Comment on attachment 214605 [details] [diff] [review]
CHECKED IN: Patch, including Nelson's changes, wt'c update, and a Makefile changes to turn it on in Linux

Re: keeping freebl/Makefile and freebl/mpi/target.mk in sync

I just wanted to add that in freebl/mpi/target.mk,
-DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN are also defined
in the new WIN32 section.

Comment 21

[Robert Relyea]

Checking in target.mk;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/target.mk,v  <--  target.mk
new revision: 1.5; previous revision: 1.4
done
Checking in mpmontg.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpmontg.c,v  <--  mpmontg.c
new revision: 1.18; previous revision: 1.17
done
Checking in tests/mptest-7.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/tests/mptest-7.c,v  <--  mptest-7.c
new revision: 1.5; previous revision: 1.4
done
Checking in tests/mptest-8.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/tests/mptest-8.c,v  <--  mptest-8.c
new revision: 1.5; previous revision: 1.4
done
Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.75; previous revision: 1.74
done


Checked in on tip, awaiting 2nd for 3.11

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 214605 [details] [diff] [review]
CHECKED IN: Patch, including Nelson's changes, wt'c update, and a Makefile changes to turn it on in Linux

Bob, one question about this patch.

>Index: Makefile
>===================================================================

> ifeq ($(CPU_ARCH),x86)
>     ASFILES  = mpi_x86.s
>     DEFINES += -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE 
>     DEFINES += -DMP_ASSEMBLY_DIV_2DX1D
>+    DEFINES += -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN

Is this a win on x86 (32), on either Intel or AMD? 

>     # The floating point ECC code doesn't work on Linux x86 (bug 311432).
>     #ECL_USE_FP = 1
> endif

Comment 23

[Robert Relyea]

Very solid 1/2 % for RSA on intel:). I don't know about amd. 
Nothing measurable for ECC, but ECC is not as multiply intensive as RSA.

Comment 24

[Robert Relyea]

NSS 3.11 patch checked in:
Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.70.2.3; previous revision: 1.70.2.2
done
Checking in mpi/mpmontg.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpmontg.c,v  <--  mpmontg.c
new revision: 1.17.2.1; previous revision: 1.17
done
Checking in mpi/target.mk;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/target.mk,v  <--  target.mk
new revision: 1.4.2.1; previous revision: 1.4
done
Checking in mpi/tests/mptest-7.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/tests/mptest-7.c,v  <--  mptest-7.c
new revision: 1.4.30.1; previous revision: 1.4
done
Checking in mpi/tests/mptest-8.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/tests/mptest-8.c,v  <--  mptest-8.c
new revision: 1.4.30.1; previous revision: 1.4
done
jordan.sfbay.redhat.com(778)

Comment 25

[Robert Relyea]

Nelson, I'm passing this back to you. There are other platforms that my need MP_CHARE_STORE_SLOW turned on (I'm thinking solaris x86 or solaris amd 64). I don't want to turn it on without a test box.

I expect you will hand it off to someone else who has access to those boxes, or just close it.

bob

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Marking fixed.  Thanks, Bob.
If this issue arises on other CPUs, we can open new bugs for those CPUs.