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

RESOLVED FIXED in 3.16

Status

P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: wtc, Assigned: wtc)

Tracking

trunk
3.16
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
This problem was reported by Stephan Bergmann of Red Hat on the
dev-security@lists.mozilla.org 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).
(Assignee)

Comment 1

5 years ago
Created attachment 8376631 [details] [diff] [review]
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
Attachment #8376631 - Flags: review+
Attachment #8376631 - Flags: checked-in+
(Assignee)

Comment 2

5 years ago
Created attachment 8376641 [details] [diff] [review]
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.
Attachment #8376641 - Flags: review?(kaie)
(Assignee)

Comment 3

5 years ago
Created attachment 8377859 [details] [diff] [review]
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
Attachment #8377859 - Flags: checked-in+
(Assignee)

Comment 4

5 years ago
Created attachment 8379329 [details] [diff] [review]
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.
Attachment #8379329 - Flags: review?(kaie)

Comment 5

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

r=kaie
Attachment #8376641 - Flags: review?(kaie) → review+

Comment 6

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

r=kaie
Attachment #8379329 - Flags: review?(kaie) → review+
(Assignee)

Comment 7

5 years ago
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
Attachment #8376641 - Flags: checked-in+
(Assignee)

Comment 8

5 years ago
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
Attachment #8379329 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.