972450 - Define -DMP_USE_UINT_DIGIT in lib/freebl/Makefile for Linux x86

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

This problem was reported by Stephan Bergmann of Red Hat on the
[e-mail] mailing list:

  Stumbled over this when trying to build under linux32 the
  nss 1.15.3 that we bundle in LibreOffice, but seems the
  relevant files are still the same on trunk
  hg.mozilla.org/projects/nss:

  lib/freebl/Makefile for OS_TARGET=Linux, CPU_ARCH=x86 sets
  ASFILES=mpi_x86.s but doesn't include -DMP_USE_UINT_DIGIT
  in DEFINES. That causes lib/freebl/mpi/mpi.h to pick up the

    #elif !defined(MP_USE_UINT_DIGIT) && defined(MP_ULONG_LONG_MAX)

  block that does

    typedef unsigned long long mp_digit;

  so that the calls to s_mpv_div_2dx1d in lib/freebl/mpi/mpi.c
  pass 64-bit Nhi, Nlo, divisor arguments, but the implementation
  of s_mpv_div_2dx1d in lib/freebl/mpi/mpi_x86.s apparently
  expects mp_digit to be unsinged long and takes 32-bit arguments,
  garbling the subsequent qp and rp pointer arguments.

  The below patch solved it for me, unless I'm missing the obvious
  and walked down the wrong path anyway (not knowing much about
  the code in question, just seeing it fail in my LibreOffice build).

  Stephan


--- lib/freebl/Makefile
+++ lib/freebl/Makefile
@@ -195,6 +195,7 @@
 ifeq ($(CPU_ARCH),x86)
     ASFILES  = mpi_x86.s
     DEFINES += -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE
+    DEFINES += -DMP_USE_UINT_DIGIT
     DEFINES += -DMP_ASSEMBLY_DIV_2DX1D
     DEFINES += -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN
     # The floating point ECC code doesn't work on Linux x86 (bug 311432).

Comment 1

[Wan-Teh Chang]

Attachment: Patch by Stephan Bergmann

I also define -DMP_NO_MP_WORD to match Windows x86. My understanding
is that MP_NO_MP_WORD is a performance-turning parameter. I remember
I experimented with it two or three years ago, but I forgot whether
it improved or hurt performance. For now let's be consistent on all
x86 platforms.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/60412aa39b8d

Comment 2

[Wan-Teh Chang]

Attachment: mpi.h should test for the C99 ULLONG_MAX macro first. Update comment.

ULLONG_MAX is the standard macro in C99 for the maximum
value of unsigned long long, so we should test for it
first. I also update the comment to say "C99" instead of
"GCC" because ULLONG_MAX is also defined by Visual C++
and Apple's compiler now.

Comment 3

[Wan-Teh Chang]

Attachment: Don't define -DMP_NO_MP_WORD for Linux x86

I found that we are not defining -DMP_NO_MP_WORD for Mac OS X x86.
So I suspect that when I enabled x86 assembly code for Mac OS X,
I did some performance measurements but did not find -DMP_NO_MP_WORD
to improve performance. So I'm taking a more conservative approach
and remove -DMP_NO_MP_WORD for Linux x86 (unless someone shows it
improves performance).

Patch checked in: https://hg.mozilla.org/projects/nss/rev/fa9a3a970910

Comment 4

[Wan-Teh Chang]

Attachment: Don't compile NSS with any *_SOURCE macros defined

While working on bug 966596, I figured out why <limits.h> doesn't
define LLONG_MAX when NSS is compiled. The reason is that we are
compiling NSS with -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE.

These -Dmacro flags were added because we used to compile NSS
the -ansi flag, so these *_SOURCE macros were needed to allow
NSS to use certain system calls.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 8376641 [details] [diff] [review]
mpi.h should test for the C99 ULLONG_MAX macro first. Update comment.

r=kaie

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 8379329 [details] [diff] [review]
Don't compile NSS with any *_SOURCE macros defined

r=kaie

Comment 7

[Wan-Teh Chang]

Comment on attachment 8376641 [details] [diff] [review]
mpi.h should test for the C99 ULLONG_MAX macro first. Update comment.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/444aacba97e4

Comment 8

[Wan-Teh Chang]

Comment on attachment 8379329 [details] [diff] [review]
Don't compile NSS with any *_SOURCE macros defined

Patch checked in: https://hg.mozilla.org/projects/nss/rev/26692da45004