303508 - Add freebl shared libs that do only 64-bit integer math

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Today, NSS uses floating point code to do bignum arithmetic on Sparc family
64-bit CPUs.  The FPU code is faster than integer code on some of them,
and not on others.  So we also need a version of freebl shared libs that 
do not use the FPU, but use 64-bit integer code instead.  

Freebl already has all the code to do this.  It's just a matter of building
a shared lib with the right preprocessor symbols defined.  

Today, we do not build a separate freebl shared lib for the v9 64-bit ISA.
We'll have to change that to build two freebl shared libs for v9, (one 
for FPU code, one for integer code).  We'll also have to add another freebl
shared lib for the 32-bit v8+ ISA, so we'll have two for that ISA, one
with FPU code and one with integer code.  

The existing freebl shared libs are named:
   freebl_pure32.so        v8  (32 bit integer only)
   freebl_hybrid.so        v8+ (FPU)
I propose to add these:
   freebl_v8+INT.so        v8+ (64-bit integer code)
   freebl_v9FPU.so         v9  (FPU code)
   freebl_v9INT.so         v9  (64-bit integer only)

Comment 1

[Wan-Teh Chang]

"v8plus" seems better than "v8+". For example, the
compiler flag is -xarch=v8plus and the $ISALIST
linker keyword is sparcv8plus.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Yeah, I got rid of the "+" and changed my mind about the naming conventions.
I'll leave _hybrid and _pure32 alone.  I'm currently thinking about
_v8int64, _v9int64 and _v9fpu32 (since all the FPU code uses 32-bit ints)

Comment 3

[Julien Pierre]

Nelson,

I don't think we need the distinction of "fpu32", since the FPU code can
currently only operate on 32-bit ints on all Sparc chips . The maximum size of
the bignums is determined by the instruction set and whether fp or int math is
chosen, so in a sense this is redundant information . And it will certainly be a
source of confusion when asking our customers which library they were running if
there are two numbers in the name of the library.

I think the names of the additional libraries you originally proposed in this
bug are fine if you just replace "+" with "plus" . I don't like the capitalized
INT and FPU also, but it's OK to leave it.

I would prefer if we also renamed "freebl_pure32" to "freebl_v8int" and
"freebl_hybrid" to "freebl_v8plusfp[u]" out of consistency, but leaving them
alone is fine too .

Comment 4

[Wan-Teh Chang]

I agree we should rename the two existing freebl .so's
to be consistent with the three new ones.  "pure32" and
"hybrid" have been a source of questions.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

OK, so the current proposal is   (all prefixed with freebl )

_v8int       (formerly _pure32)
_v8plusint   (new)
_v8plusfpu   (formerly _hybrid)
_v9int       (new)
_v9fpu       (new)

Now I have one other observation.
Formerly, we used the _pure32 and _hybrid names on both sparc and HP PARisc.
Shall we rename the HPUX libs too?  Or leave them alone?
I don't think the v8 and v9 names are appropriate for HPUX :)
I'd kinda like having a uniform set of names across platforms.
Do we need yet another proposal?  :)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I've been trying to implement this, and one thing has become clear to me.
We want names to be as common to platforms as they can be.  
IOW, the build that builds 64-bit integer code using the platform's 
64-bit ABI, wants to have a common name whether it's on HPUX or Sun or
whathaveyou.  Similarly, the 32-bit FPU code should have the same name
on any platforms on which it is built.  Actually, this applies to both
the names of the built shared libs, and to the names of the directories
in which they are built.  That way, the Makefile can have one set of 
lines for the "32-bit-ABI-with-64-bit-integer-instructions" for all
platforms that support it, and doesn't need separate Makefile code for
that case on each platform.  Otherwise, the makefiles just become too 
big and unmaintainable, IMO.

So, how about this:

freebl_ABI32_INT32  (formerly was pure32)
freebl_ABI32_INT64
freebl_ABI32_FPU    (formerly was hybrid on Sun and HPUX)
freebl_ABI64_INT
freebl_ABI64_FPU

We can make names upper or lower case, and include underscores or not.
I don't care too much about that.  I just want common names for all 
platforms.

Comment 7

[Wan-Teh Chang]

I agree it's good to have cross-platform names.

I like all lowercase library names better.

I have one suggestion -- omit ABI32 and ABI64.
The reason is that the 32-bit and 64-bit libraries
will be installed in different directories, so they
may have the same file names.  For example, the
32-bit and 64-bit versions of the nss3 shared library
are both called libnss3.so.  So I suggest the following:

32-bit ABI shared libraries:
freebl_int32  (formerly was pure32)
freebl_int64
freebl_fpu    (formerly was hybrid on Sun and HPUX)

64-bit ABI shared libraries:
freebl_int
freebl_fpu

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I want all the freebl shared libs on a platform to have unique names.
I want to be able to tell the ABI and bignum implementation by looking at
(or hearing) just the file name.  People talk about these shared libs by only
their files names.  I don't want all future convertsations about these shared 
libs to be mired in ambiguity.  

Lower case is fine.  If brevity is desired, then we could use
freebl_32int    (formerly was pure32)
freebl_32int64
freebl_32fpu    (formerly was hybrid on Sun and HPUX)
freebl_64int
freebl_64fpu

Comment 9

[Wan-Teh Chang]

OK.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for hacks branch

under test now.  This patch is not a candidate for trunk checkin, but it
might be useful for one of the other NSS core team members to review it, 
so we can avoid more wasted patches later.

Comment 11

[Saul Edwards]

Attachment: Add integer library builds for 64-bit sparc; split DEFINES and arch flags into separate sections

Most of this work contributed by Neil Williams, Nelson Bolyard, and Julien
Pierre.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 193768 [details] [diff] [review]
Add integer library builds for 64-bit sparc; split DEFINES and arch flags into separate sections

r=nelson@bolyard provided you make the following changes.
I don't need to see another patch for these changes.
I'm asking Wan-Teh for second review

>+ifeq (OS2,$(OS_TARGET))
>+LN_S     = cp
>+endif

should indent that LN_S assignment to match the other indentation

>+
> ifeq (,$(filter-out WINNT WIN95,$(OS_TARGET)))  #omits WIN16 and WINCE
> ifdef NS_USE_GCC
> # Ideally, we want to use assembler
> #     ASFILES  = mpi_x86.s
> #     DEFINES += -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE \
> #                -DMP_ASSEMBLY_DIV_2DX1D
> # but we haven't figured out how to make it work, so we are not
> # using assembler right now.
>     ASFILES  =
>     DEFINES += -DMP_NO_MP_WORD -DMP_USE_UINT_DIGIT
> else
>+    LN_S     = cp
>     ASFILES  = mpi_x86.asm

LN_S needs to be defined for the GCC case too.
I suggest you move that line up to just before the preceeding
ifdef NS_USE_GCC

> ifeq ($(OS_TARGET), HP-UX)
> ifneq ($(OS_TEST), ia64)
>+# PA-RISC
>+ifndef USE_64
>+FREEBL_BUILD_SINGLE_SHLIB=
>+HAVE_ABI32_INT32=1
>+HAVE_ABI32_FPU=1
>+endif

Indent those assignements.

> MKSHLIB += -R '$$ORIGIN'
>+ifdef NS_USE_GCC
>+  ifdef GCC_USE_GNU_LD

This appears to introduce a new makefile variable: GCC_USE_GNU_LD
If I understand it, this doesn't tell GCC to use GNU ld, but 
rather tells the make file that gcc on this box has been configured
to use GNU ld.	In that case, I think the name GCC_USES_GNU_LD 
would be more appropriate.  Is it used in any other Makefile?

> 	ifdef USE_64
>+	    #ARCHFLAG=-m64 set by coreconf/sunOS5.mk
>+	endif

I'm glad for that comment.  It assures me (a reviewer) that this 
case wasn't merely overlooked, but that the default is correct.
It also provides an indication of what the default must be.

>+	ifdef USE_ABI32_INT64
>+	    # this builds for Sparc v8+a ABI32_FPU architecture, 64-bit registers, 
>+	    # 32-bit ABI, it uses 64-bit words, integer arithmetic, no FPU
>+	    SOL_CFLAGS += -fast -xO5 -xrestrict=%all -xchip=ultra

Probably should have a comment (perhaps just a checkin log comment)
that we plan to replace all the -fast options soon, as soon as we've
figured out the right replacement for each architecture.

>+ifndef LN_S
>+LN_S = ln -s
>+endif

indentation

Comment 13

[Wan-Teh Chang]

Nelson: GCC_USE_GNU_LD is an existing make variable.
It means that gcc has been configured to use GNU ld.
Feel free to rename the make variable as GCC_USES_GNU_LD.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Thanks for that info, Wan-Teh.  If GCC_USE_GNU_LD is already documented 
(e.g. in www.mozilla.org) or used in mozilla browser makefiles, then 
we should leave it alone.  Do you know if it is documented or used outside
of NSS sources?

Comment 15

[Wan-Teh Chang]

Comment on attachment 193768 [details] [diff] [review]
Add integer library builds for 64-bit sparc; split DEFINES and arch flags into separate sections

Sorry, Saul.  Although I didn't test this patch, I
saw strong evidences in my code review that it is
incomplete.

Nit: Please follow the indentation and coding style of
the file you modify, even if the style is bad.	Variable
definitions should be
    FOO = 1
    BAR =
instead of
    FOO=1
    BAR=
except when they are on the make command line, in which
case space is not allowed.

Now the problems of substance.

1. You replaced FREEBL_EXTENDED_BUILD by FREEBL_BUILD_SINGLE_SHLIB
(with the opposite sense) and FREEBL_RECURSIVE_BUILD by
FREEBL_CHILD_BUILD.  lib/freebl/config.mk also uses
FREEBL_EXTENDED_BUILD and FREEBL_RECURSIVE_BUILD, but your
patch doesn't change lib/freebl/config.mk.

2. You added a new variable LN_S to replace the original
hardcoded "ln -s".  The idea is good, but the execution
wasn't complete because we also use "ln -s" to link a
directory ($(CDDIR)/mpi).  On OS/2 and Windows, LN_S is
"cp", which can't copy a directory.

I suggest going back to the hardcoded "ln -s".	If you
still want to use a variable, consider first setting LN_S to
the default (ln -s) and then overriding it (cp) for OS/2 and
Windows, just like how you define FREEBL_BUILD_SINGLE_SHLIB.

3. You moved the following block of code:

>+ifdef NS_USE_GCC
>+  ifdef GCC_USE_GNU_LD
>+    MKSHLIB += -Wl,-Bsymbolic,-z,defs,-z,now,-z,text,--version-script
>+  else
>+    MKSHLIB += -Wl,-B,symbolic,-z,defs,-z,now,-z,text
>+  endif # GCC_USE_GNU_LD
>+else
>+  MKSHLIB += -B symbolic -z defs -z now -z text
>+endif # NS_USE_GCC

but while doing this, you dropped ",mapfile.Solaris" after
--version-script, and dropped ",-M,mapfile.Solaris" and
"-M mapfile.Solaris" for the other two cases.

Nit: you indent this block of code by two spaces.
Please change that to be consistent with the indentation of
the other code.

4. For SunOS  sparc, in NS_USE_GCC case, you have

> 	ifdef USE_64
>+	    #ARCHFLAG=-m64 set by coreconf/sunOS5.mk
>+	endif

But in the Sun WorkShop case, you have

>+	ifdef USE_ABI64_INT
...
>+	endif
>+	ifdef USE_ABI64_FPU
...
>+	endif

So the NS_USE_GCC case is incomplete.

5. In the FREEBL_BUILD_SINGLE_SHLIB case, we have

>+################### Single ABI stuff #########################
>+SINGLE_SHLIB_DIR = $(OBJDIR)/$(OS_TARGET)_SINGLE_SHLIB
>+SINGLE_SHLIB_FILES = $(addprefix $(SINGLE_SHLIB_DIR)/, $(FILES2LN))
>+ALL_TRASH += $(SINGLE_SHLIB_DIR) 
>+
>+$(SINGLE_SHLIB_DIR):
>+	-mkdir $(SINGLE_SHLIB_DIR)
>+
>+$(SINGLE_SHLIB_FILES) : $(SINGLE_SHLIB_DIR)/% : % 
>+	$(LN_S) $(CDDIR)/$* $(SINGLE_SHLIB_DIR)
>+
>+libs:: $(SINGLE_SHLIB_DIR) $(SINGLE_SHLIB_FILES)
>+	cd $(SINGLE_SHLIB_DIR) && $(MAKE) FREEBL_CHILD_BUILD=1 \
>+ FREEBL_PARENT=../.. CORE_DEPTH=../../$(CORE_DEPTH) libs
>+######################## common stuff #########################

This case has two differences from the other cases.  First,
it does not create a symbolic link to $(CDDIR)/mpi.  Second,
it uses "../.." instead of $(CDDIR) in the values of FREEBL_PARENT
and CORE_DEPTH.  Why the differences?

The missing symbolic link to $(CDDIR)/mpi should break
FREEBL_BUILD_SINGLE_SHLIB builds.

6. You broke the release_md rule.  The old rule is:

>-release_md::
>-	$(MAKE) FREEBL_RECURSIVE_BUILD=1 USE_HYBRID=1 $@
>-	cd $(PURE32DIR) && $(MAKE) FREEBL_RECURSIVE_BUILD=1 USE_PURE_32=1 FREEBL_PARENT=$(CDDIR) CORE_DEPTH=$(CDDIR)/$(CORE_DEPTH) $@

Note that it invokes the release_md target ($@) in the two
recursive builds.

The new rule is a no-op:

>+release_md:: $(libs)
>+

7. This item is a commentary, not really a suggested change.

One reason is makefile is so complicated is that the parent
build and the child builds use the same makefile.

I believe the makefile can be simplified by using a separate
makefile for each child build.	In addition, we can create
subdirectories under the lib/freebl directory, one for each
child build, and put the child build makefiles in the subdirectories.
Those makefiles can use GNU make's vpath or VPATH to point to
the source files in other directories.

We have to create subdirectories for the child builds anyway.
My suggestion above is to create the subdirectories in the
source tree, as opposed to under $(OBJDIR).

Comment 16

[Wan-Teh Chang]

Nelson: GCC_USE_GNU_LD is used in Mozilla, NSPR, and NSS.
It is not a standard autoconf or make variable though.
It was I who added the variable.  (I was fed up by all
the bug reports of broken Solaris GCC builds and spent
some time figuring out how to tell GCC with Solaris ld
and GCC with GNU ld apart.)

This is one of the things a non-native speaker has
difficulty with :-)  I don't mind changing it to make
it more correct.

Comment 17

[Julien Pierre]

Wan-Teh,

Re: your comment #15, issue 2 :

I believe we are no longer attempting to link any directories in this patch.
Look at the assignment for FILES2LN to verify that . This is why the build works
on Windows and OS/2 (I tested the later on the perf hacks branch successfully,
though not with this exact patch) .

issue 3 :
this is resolved in the (missing) config.mk patch by using the standard MAPFILE
coreconf environment variable ;) Hopefully Saul will attach the missing
config.mk patch soon.

issue 5 :

The SINGLE_SHLIB build isn't broken, again because of changes in config.mk .
We should use the same ../.. path for all ABI builds though, not just for the
SINGLE_SHLIB case .

The ../.. path is used to fixed the Windows cygwin case where $(CDDIR) is a
cygwin path which can't be passed to any native tools . I only did it for
SINGLE_SHLIB because Windows only has SINGLE_SHLIB, but it wouldn't hurt the
other ABI builds .

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

I already addressed all Wan-Teh's Makefile issues on the branch.
Saul is working today on putting together a complete patch to bring the rest
of the branch changes to the trunk.

Comment 19

[Saul Edwards]

Attachment: Add changes from freebl, coreconf, and shlibsign; address numerous review comments

This patch applies cleanly to the trunk.  It builds and passes QA on Solaris
and Linux AMD64 and Solaris Sparc 32- and 64-bit builds.

We elected not to reorganize the gmake infrastructure of freebl (Makefile,
config.mk, manifest.mn, etc) any further at this time.	However, we should
probably split the files as Wan-Teh suggested in the near future.

Sorry for the incomplete patch in the previous review requests, guys.

Comment 20

[Saul Edwards]

Attachment: new freebl.def accompanies above patch

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194403 [details] [diff] [review]
Add changes from freebl, coreconf, and shlibsign; address numerous review comments

Wan-Teh or Bob, please review this patch together with the separately attached
new .def file.

I'm confident that this patch (together with the separately attached .def file)
is complete, because I have applied it to a trunk tree and built it.  I believe
it cleans up the issues previously seen in the Makefile.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194404 [details]
new freebl.def accompanies above patch

This is essentially the file formerly known as solaris.mapfile.  
I suggest changing the copyright date in the license header to show the years
2000-2005.

Comment 23

[Wan-Teh Chang]

Comment on attachment 194404 [details]
new freebl.def accompanies above patch

We should use a version like NSSprivate_3.11 to suggest
that this interface should only be used internally
by NSS itself.	See the naming convention described
in the Solaris Linker and Libraries Guide at
http://docs.sun.com/app/docs/doc/817-1984/6mhm7pl2t?a=view.

NSS's naming convention does not need the underscore
between freebl and 3.  So the library name should
be "freebl3".

Comment 24

[Julien Pierre]

Wan-teh,

The _3 comes from the LIBRARY_VERSION and was used in the past because some
freebl libraries had digits at the end (eg. libfreebl_pure32) and this would
have resulted in names ending with strange numbers .
One library still ends with digits - freebl_32int64 - so we can't do away with
the underscore completely. Maybe we can make that LIBRARY_NAME to 
freebl_32int64_ instead, and change LIBRARY_VERSION to just 3 .

Comment 25

[Saul Edwards]

Just FYI: When applied to the trunk builds, attachments 194403 and 194404 pass
QA for the following platforms:

Solaris Sparc 9 and 10, 32/64 bit
Solaris x86 9 and 10, 32/64 bit
Linux RHEL 3 and RHEL 4, 32/64 bit
AIX 5.1, 32/64 bit
HPUX 11.11, 32/64 bit
Windows

Comment 26

[Wan-Teh Chang]

I was referring to the freebl DLL produced on
FREEBL_BUILD_SINGLE_SHLIB platforms.  The name
of this DLL should be libfreebl3.so/freebl3.dll;
there is no need for the underscore.  I understand
why you need the underscore for the names of the
(multiple) freebl shared libraries on other platforms.

Comment 27

[Wan-Teh Chang]

Comment on attachment 194403 [details] [diff] [review]
Add changes from freebl, coreconf, and shlibsign; address numerous review comments

I'm sorry that my review comments are very long, but I did
a careful review of this patch and found many problems. Most
of them are minor, but some are serious.

1. coreconf/HP-UXB.11.mk

The changes to this file are basically okay.  I suggest
two changes.  1) use = instead of += in all the HPUX_OS_CFLAGS
assignments.  2) rename HPUX_OS_CFLAGS as ARCHFLAG, which
is the name of the equivalent variable for Solaris.  Note:
if you rename HPUX_OS_CFLAGS, make sure you also rename it
in lib/freebl/Makefile.

2. coreconf/SunOS5.mk

The changes to this file are good.  I suggest that we remove
this comment:

> # We always use Sun's assembler and linker, which use Sun's naming convention.

It is out of date.  First, we support the use of GNU linker
(when GCC is used) now.  Second, the assembler flag it refers
to (-Wa,-xarch=v8plus) is moved to lib/freebl/Makefile by this
patch.

3. coreconf/UNIX.mk

The change to this file is wrong.  It doesn't work (I tested
it with GNU make 3.79.1 and 3.80).  It is a Bourne shell feature.

>-	DEFINES    += -DDEBUG -UNDEBUG -DDEBUG_$(shell whoami)
>+	DEFINES    += -DDEBUG -UNDEBUG -DDEBUG_$(USER:-$(shell whoami))

Did you test this change? $(USER:-$(shell whoami)) has an empty value.

4. nss/cmd/shlibsign/Makefile

The changes to this file are basically okay.  I suggest three changes.

Please remove the line
    CHECKFILES = softokn3.chk
from cmd/shlibsign/manifest.mn

>+CHECKLIBS += $(wildcard $(DIST)/lib/$(DLL_PREFIX)freebl*.$(DLL_SUFFIX))

I suggest you add "3" after the "*" to make the pattern more restrictive.

>+CHECKLOC = $(subst .$(DLL_SUFFIX),.chk,$(CHECKLIBS))

Usually we do the suffix replacement using this shorthand form:
  CHECKLOC = $(CHECKLIBS:.$(DLL_SUFFIX)=.chk)

This form is more robust because it only does the substitution
when .$(DLL_SUFFIX) is found at the end of a word.

> %.chk: %.$(DLL_SUFFIX) 
> ifeq ($(OS_TARGET), OS2)
> 	@cmd.exe /c sign.cmd $(DIST) $(OBJDIR) $(OS_TARGET) $(NSPR_LIB_DIR) $<
> else
>-	@sh ./sign.sh $(DIST) $(OBJDIR) $(OS_TARGET) $(NSPR_LIB_DIR) $<
>+	sh ./sign.sh $(DIST) $(OBJDIR) $(OS_TARGET) $(NSPR_LIB_DIR) $<
> endif

You should remove the @ character from the OS2 case, too.

5. nss/lib/freebl/Makefile

>+# default for all platforms
>+# unset this on those that have multiple libraries
>+FREEBL_BUILD_SINGLE_SHLIB = 1

Add "freebl" between "multiple" and "libraries".

> MKSHLIB += +k +vshlibunsats -u FREEBL_GetVector +e FREEBL_GetVector

Remove "-u FREEBL_GetVector +e FREEBL_GetVector".  It is replaced by
the use of the mapfile.

> # build for DA1.1 (HP PA 1.1) pure 32 bit model

Might want to change "pure 32 bit" to "ABI32_INT32" because you
changed "hybrid" to "ABI32_FPU" in a comment below.

>+# We always use Sun's assembler and linker, which use Sun's naming convention.

Remove "and linker" and change "use" to "uses".  We support GNU
linker (when GCC is used) now.

>+################### Single ABI stuff #########################

Should "ABI" be changed to "shared library"?

>+# multiple ABIs

Should "ABIs" be changed to "shared libraries"?

In this part of the makefile, you can eliminate two levels of
ifdefs by taking advantage of the fact that some of these variables
are mutually exclusive.

  ifdef FREEBL_BUILD_SINGLE_SHLIB
  ...
  else
  # multiple ABIs
  ifndef USE_64
  ifdef HAVE_ABI32_FPU
  ...
  endif
  ifdef HAVE_ABI32_INT32
  ...
  endif
  ifdef HAVE_ABI32_INT64
  ...
  endif
  else # above is 32-bit builds, below is 64-bit builds
  ifdef HAVE_ABI64_FPU
  ...
  endif
  ifdef HAVE_ABI64_INT
  ...
  endif
  endif # 32b or 64b recursive builds
  endif # FREEBL_BUILD_SINGLE_SHLIB

can be changed to

  ifdef FREEBL_BUILD_SINGLE_SHLIB
  ...
  endif
  # multiple ABIs
  ifdef HAVE_ABI32_FPU
  ...
  endif
  ifdef HAVE_ABI32_INT32
  ...
  endif
  ifdef HAVE_ABI32_INT64
  ...
  endif
  # above is 32-bit builds, below is 64-bit builds
  ifdef HAVE_ABI64_FPU
  ...
  endif
  ifdef HAVE_ABI64_INT
  ...
  endif

By the way, the above clearly shows the first variable's name
(FREEBL_BUILD_XXX) looks very different from the other related
variables' names (HAVE_XXX).

6. nss/lib/freebl/arcfour.c

I don't understand the changes from NSS_USE_HYBRID to IS_64 and
SOLARIS to __sparc:

>-#if defined(NSS_USE_HYBRID) && !defined(SOLARIS) && !defined(NSS_USE_64) 
>+#if defined(IS_64) && !defined(__sparc) && !defined(NSS_USE_64)

Also, the other ifdef you changed is:

>-#if (defined(NSS_USE_HYBRID) && !defined(SOLARIS)) || defined(NSS_USE_64)
>+#if (defined(IS_64) && !defined(__sparc)) || defined(NSS_USE_64)

Why is it !defined(NSS_USE_64) above and defined(NSS_USE_64) here?

7. nss/lib/freebl/config.mk

>+# this is not a recursive child make.  We make a .a static lib.

Remove ".a".

>-# This is a recursive build.  
>+# This is a recursive build.  We're the child make. We build the shared lib.

Combine the first two sentences into "This is a recursive child make."

>-TARGETS	     = $(SHARED_LIBRARY)

Restore this line

>+# don't want the 32 in the shared library name
>+SHARED_LIBRARY = $(OBJDIR)/$(DLL_PREFIX)$(LIBRARY_NAME)$(LIBRARY_VERSION).$(DLL_SUFFIX)
>+IMPORT_LIBRARY = $(OBJDIR)/$(IMPORT_LIB_PREFIX)$(LIBRARY_NAME)$(LIBRARY_VERSION)$(IMPORT_LIB_SUFFIX)

Change the IMPORT_LIBRARY line to
  IMPORT_LIBRARY =
because we don't need an import library for the freebl DLL.

>+# do we need these?
>+#RES = $(OBJDIR)/freebl.res
>+#RESNAME = freebl.rc

Yes, we should uncomment those two lines and add a freebl.rc file
so we can get version info on Windows.	This reminds me that we
also need to add sccs and rcs version strings so we can use the
"what" or "ident" command to extract the version string on Unix.
See lib/nss/{nssver.c,nssinit.c} for an example.

>+ifdef NS_USE_GCC
>+EXTRA_SHARED_LIBS += \
>+	-L$(DIST)/lib \
>+	-L$(NSPR_LIB_DIR) \
>+	-lplc4 \
>+	-lplds4 \
>+	-lnspr4 \
>+	-lc

Remove "-L$(DIST)/lib \" and change "-lc" to "$(NULL)".
Here, $(DIST)/lib has been replaced by $(NSPR_LIB_DIR).
"-lc" is no longer necessary after Julien's recent change
to link $(SHARED_LIBRARY) with $(OS_LIBS).

>+else
>+
>+EXTRA_SHARED_LIBS += \
> 	-L$(DIST)/lib/ \
>+	-L$(NSPR_LIB_DIR) \
> 	-lplc4 \
> 	-lplds4 \
> 	-lnspr4 \
> 	-lc
>-#endif

Same as above, remove "-L$(DIST)/lib \" and change
"-lc" to "$(NULL)".

8. nss/lib/freebl/loader.c

If I understand it correctly, this file will be part of not only
the softokn3 shared library but also the ssl3 shared library.  Your
change assumes it is part of the softokn3 shared library.  So the
PR_GetLibraryFilePathname magic you do in bl_LoadLibrary won't work
for the ssl3 shared library.  Is this a problem?

For the definition of the getLibName function, I suggest that
you have a case for Solaris SPARC:
    defined(SOLARIS) && defined(__sparc)
and get rid of the Solaris x86 and x86_64 cases, which can use
the default case.

The curly braces around the strings in the initializers for
the const char arrays are not necessary:

>+const char fast_hybrid_shared_lib[] = { "libfreebl_64fpu_3.so"  };
>+const char slow_hybrid_shared_lib[] = { "libfreebl_64int_3.so" };
>+const char  non_hybrid_shared_lib[] = { "libfreebl_64fpu_3.so" };
>+
>+const char slow_hybrid_isa[] = { "sparcv9" };
>+const char fast_hybrid_isa[] = { "sparcv9+vis" };
>+
>+#else
>+
>+const char fast_hybrid_shared_lib[] = { "libfreebl_32fpu_3.so"  };
>+const char slow_hybrid_shared_lib[] = { "libfreebl_32int64_3.so" };
>+const char  non_hybrid_shared_lib[] = { "libfreebl_32int_3.so"  };
>+
>+const char slow_hybrid_isa[] = { "sparcv8plus" };
>+const char fast_hybrid_isa[] = { "sparcv8plus+vis" };
>+
>+#endif

And they should all be "static", too.

>+static const char *
>+getLibName(void)
> {
...
>     buflen = sysinfo(SI_ISALIST, buf, sizeof buf);
>+    if (buflen <= 0) 
>+	return ".";

You should return NULL instead of "." here.

>+    found_slow_hybrid = strstr(buf, slow_hybrid_isa);
>+    found_fast_hybrid = strstr(buf, fast_hybrid_isa);
>+    /* since the slow string is a subset of the fast string,
>+     * if the fast string is found, the slow string will definitely
>+     * be found, and will never appear to come after the fast string.
>+     * The slow string will either be before the fast string, or
>+     * at the same place as the fast string, not after it.
>+     */
>+    if (found_fast_hybrid && 
>+	(!found_slow_hybrid ||
>+	 (found_slow_hybrid - found_fast_hybrid) >= 0)) {
>+	return fast_hybrid_shared_lib;
>     }

The comment and test for "found_slow_hybrid" are useless
and should be removed.

Would you use slow_hybrid_shared_lib if both the fast
and slow strings are found and the slow string is after the
fast string?  I believe you still want fast_hybrid_shared_lib
even in that case.

>+#elif defined(HPUX) && !defined(NSS_USE_64)

Add "&& !defined(__ia64)" because this code is PA-RISC specific.

> /*
>  * On Solaris, if an application using libnss3.so is linked
>  * with the -R linker option, the -R search patch is only used
>  * to search for the direct dependencies of the application
>  * (such as libnss3.so) and is not used to search for the
>  * dependencies of libnss3.so.  So we build libnss3.so with
>  * the -R '$ORIGIN' linker option to instruct it to search for
>  * its dependencies (libfreebl_*.so) in the same directory
>  * where it resides.  This requires that libnss3.so, not
>  * libnspr4.so, be the shared library that calls dlopen().
>  * Therefore we have to duplicate the relevant code in the PR
>  * load library functions here.
>+ *
>+ * UPDATE : This workaround is no longer needed if we use
>+ * PR_GetLibraryFilePathname to get the path to softokn3, and then
>+ * do a PR_LoadLibrary with an absolute pathname for the freebl
>+ * shared library .
>  */

The old comment should simply be removed and
"UPDATE : This workaround is no longer needed if we" should
be shortened to "We".

A comment should describe the resulting code, not the change.

"softokn3" probably should be "softokn3 or ssl3".  See my first
comment about this file.

>+#include "stdio.h"

Change "stdio.h" to <stdio.h> because it is a system header.
Actually this header probably doesn't need to be included
because you use PR_fprintf, not fprintf.

The BLLibrary structure and the functions bl_FindSymbol, bl_UnloadLibrary
are no longer needed.  Just use PRLibrary and PR_FindFunctionSymbol,
PR_UnloadLibrary.  bl_LoadLibrary is still useful because it
calls PR_GetLibraryFilePathname to construct the full pathname.

>+    /* Get the pathname for the loaded libsoftokn, i.e. /usr/lib/libsoftokn.so.
>+     * PR_GetLibraryFilePathname works with either the base library name or a
>+     * function pointer, depending on the platform. So call it even if fn_addr
>+     * is NULL. We can't query a well-known symbol such as NSC_GetFunctionList,
>+     * because on some platforms we can't find symbols in loaded implicit 
>+     * dependencies such as libsoftokn.  
>+     * But we can just get the address of this function !
>+     */

"libsoftokn.so" should be "libsoftokn3.so".
Delete "So call it even if fn_addr is NULL."
Change "a well-known" to "an exported".

>+    softokenPath = PR_GetLibraryFilePathname(softoken, fn_addr);

You need to call PR_Free(softokenPath) when you are done with softokenPath.

>+           fullName = (char*) PORT_ZAlloc(strlen(name) + softoknPathSize + 2);
>+           if (fullName) {
>+               strncat(fullName, softokenPath, softoknPathSize);
>+               strcat(fullName, name); 
>+           }

You can replace PORT_ZAlloc by PORT_Alloc and use strncpy to copy
softoknPathSize bytes of the softokenPath string to the fullName buffer.

9. nss/lib/freebl/manifest.mn

>+ifdef FREEBL_CHILD_BUILD
>+  ifdef USE_ABI32_INT32
>+    LIBRARY_NAME = freebl_32int
>+  endif
>+  ifdef USE_ABI32_INT64
>+    LIBRARY_NAME = freebl_32int64
>+  endif
>+  ifdef USE_ABI32_FPU
>+    LIBRARY_NAME = freebl_32fpu
>+  endif
>+  ifdef USE_ABI64_INT
>+    LIBRARY_NAME = freebl_64int
>   endif
>+  ifdef USE_ABI64_FPU
>+    LIBRARY_NAME = freebl_64fpu
>+  endif
>+endif
>+ifndef LIBRARY_NAME
>+  LIBRARY_NAME = freebl
> endif
...
>+# same version as rest of NSS, but prefixed with _
>+LIBRARY_VERSION = _$(SOFTOKEN_LIBRARY_VERSION)

I suggest that you add "_" at the end of all the LIBRARY_NAME's
except the last one ("freebl"), and define LIBRARY_VERSION
to be simply 3.

Comment 28

[Julien Pierre]

Wan-Teh,

Re: comment #27, issue 8, about loader.c :
You understand correctly that the loader code will be included in both
libsoftokn3 and libssl3 . Both those libraries live in the same location.
libssl3 depends on libnss3, which in turn depends on libsoftokn3 .
Thus, when the loader code in libssl3 gets to executed, it is assured that
libsoftokn3 is already loaded . The PR_GetLibraryFilePathname may return the
pathname of either libssl3.so, if the platform implementation uses the function
pointer argument; or the pathname of libsoftokn3.so, if the platform
implementation uses the string argument . In both cases, the subsequent code
will work, because those libraries are presumed to be loaded from the same
location . The only case which could cause this to break is if NSPR had a single
platform implementation of PR_GetLibraryFilePathname that used *both* the
function pointer and the library filename. Because in the libssl loader case,
the pointer is to a function that's in libssl3, but the filename is that of
libsoftokn3, this may cause a problem. But only if NSPR were to use both
arguments on a single platform, which to my knowledge, it does not.
One fix for this would be to call PR_GetLibraryFilePathname a second time if the
first call failed, with the name of libssl3 instead of libsoftokn3 . But I
believe this isn't an actual problem with the code as written now .

Comment 29

[Wan-Teh Chang]

Comment on attachment 194403 [details] [diff] [review]
Add changes from freebl, coreconf, and shlibsign; address numerous review comments

In nss/lib/freebl/Makefile, this line for HP-UX:

> MKSHLIB += +k +vshlibunsats -u FREEBL_GetVector +e FREEBL_GetVector

should be removed in its entirety.

>+######################## ABI32_FPU stuff #########################
>+ifdef HAVE_ABI32_FPU
>+ABI32_FPU_DIR = $(OBJDIR)/$(OS_TARGET)_ABI32_FPU
>+ABI32_FPU_FILES = $(addprefix $(ABI32_FPU_DIR)/, $(FILES2LN))
>+ALL_TRASH += $(ABI32_FPU_DIR) 
>+
>+$(ABI32_FPU_DIR):
>+	-mkdir $(ABI32_FPU_DIR)
>+
>+$(ABI32_FPU_FILES) : $(ABI32_FPU_DIR)/% : % 
>+	$(LN_S) $(CDDIR)/$* $(ABI32_FPU_DIR)
>+
>+release_md libs:: $(ABI32_FPU_DIR) $(ABI32_FPU_FILES)
>+	cd $(ABI32_FPU_DIR) && $(MAKE) FREEBL_CHILD_BUILD=1 USE_ABI32_FPU=1 \
>+ FREEBL_PARENT=../.. CORE_DEPTH=../../$(CORE_DEPTH) $@
>+endif

I'm just using the ABI32_FPU case as an example.
These makefile rules are very similar for all the
freebl shared libraries.  We should define a few
variables to parameterize these rules.	We can do
this later.

In nss/lib/freebl/loader.c, you are relying on undocumented
behavior of the current PR_GetLibraryFilePathname implementation.
I suggest that bl_LoadLibrary only does the special handling
for Solaris (because it is only needed for a secure process on
Solaris), and instead of calling PR_GetLibraryFilePathname
you call dladdr to get the library file pathname. dladdr
doesn't need the library's name, so you can avoid the
"softokn3" or "ssl3" issue altogether.	It's very simple
to use dladdr.	You can copy the code from
PR_GetLibraryFilePathname.

Comment 30

[Saul Edwards]

Attachment: Address some of Wan-Teh's concerns (checked in on trunk)

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195088 [details] [diff] [review]
Address some of Wan-Teh's concerns (checked in on trunk)

r=nelson@bolyard.  This patch appears to address most of Wan-Teh's 
review comments.  Wan-Teh has agreed to let us address some of the 
remaining review comments after the upcoming alpha build.

Here is a list of remaining substantial work to be done, or issues to be
resolved, after we get 3.11 alpha 1 built.

1. Version info on Windows shared libs.
>>>+# do we need these?
>>>+#RES = $(OBJDIR)/freebl.res
>>>+#RESNAME = freebl.rc
>
> Yes, we should uncomment those two lines and add a freebl.rc file
> so we can get version info on Windows.

2. ident command strings Should be added to ldvector.c
> we also need to add sccs and rcs version strings so we can use the
> "what" or "ident" command to extract the version string on Unix.

3. The appearance of _3. in file names.  It is desirable to remove the
underscore in cases where the character before the underscore is not a 
numeral.

4. numerous unresolved issues in loader.c, especially those related to 
PR_GetLibraryFilePathname

Comment 32

[Saul Edwards]

Comment on attachment 195088 [details] [diff] [review]
Address some of Wan-Teh's concerns (checked in on trunk)

Checked into trunk:

Checking in coreconf/HP-UXB.11.mk;
/cvsroot/mozilla/security/coreconf/HP-UXB.11.mk,v  <--	HP-UXB.11.mk
new revision: 1.8; previous revision: 1.7
done
Checking in coreconf/SunOS5.mk;
/cvsroot/mozilla/security/coreconf/SunOS5.mk,v	<--  SunOS5.mk
new revision: 1.20; previous revision: 1.19
done
Checking in nss/cmd/shlibsign/Makefile;
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v	<--  Makefile
new revision: 1.13; previous revision: 1.12
done
Checking in nss/cmd/shlibsign/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/shlibsign/manifest.mn,v  <--	manifest.mn
new revision: 1.7; previous revision: 1.6
done
Checking in nss/lib/freebl/Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.64; previous revision: 1.63
done
Checking in nss/lib/freebl/arcfour.c;
/cvsroot/mozilla/security/nss/lib/freebl/arcfour.c,v  <--  arcfour.c
new revision: 1.17; previous revision: 1.16
done
Checking in nss/lib/freebl/blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.23; previous revision: 1.22
done
Checking in nss/lib/freebl/config.mk;
/cvsroot/mozilla/security/nss/lib/freebl/config.mk,v  <--  config.mk
new revision: 1.12; previous revision: 1.11
done
Checking in nss/lib/freebl/freebl.def;
/cvsroot/mozilla/security/nss/lib/freebl/freebl.def,v  <--  freebl.def
new revision: 1.2; previous revision: 1.1
done
Checking in nss/lib/freebl/ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.12; previous revision: 1.11
done
Checking in nss/lib/freebl/loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.22; previous revision: 1.21
done
Checking in nss/lib/freebl/loader.h;
/cvsroot/mozilla/security/nss/lib/freebl/loader.h,v  <--  loader.h
new revision: 1.16; previous revision: 1.15
done
Checking in nss/lib/freebl/manifest.mn;
/cvsroot/mozilla/security/nss/lib/freebl/manifest.mn,v	<--  manifest.mn
new revision: 1.40; previous revision: 1.39
done

Comment 33

[Wan-Teh Chang]

Attachment: Patch to fix BUILD_TREE builds

It turns out that the child builds must use absolute pathnames
as the values of their FREEBL_PARENT and COREDEPTH variables.
This is because $(OBJDIR) is not always one level below the
current working directory; it may be in a completely different
place if BUILD_TREE is specified.  See the assignments of OBJDIR
in mozilla/security/coreconf/location.mk.

So we need to pass FREEBL_PARENT=$(CDDIR) CORE_DEPTH=$(CDDIR)/$(CORE_DEPTH)
instead of FREEBL_PARENT=../.. CORE_DEPTH=../../$(CORE_DEPTH)
to the recursive child builds.

This problem broke the Mozilla build (see bug 307848).	The fact
that the bug submitter tries to build Mozilla with the NSS tip is
besides the point.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195778 [details] [diff] [review]
Patch to fix BUILD_TREE builds

Julien originally changed the FREEBL_PARENT and CORE_DEPTH to relative paths
because the absolute paths were not working with cygwin.  Something about
/cygdrive being passed to the cl compiler.  So, I'm asking Julien to SR this
patch in my place.

Comment 35

[Julien Pierre]

Comment on attachment 195778 [details] [diff] [review]
Patch to fix BUILD_TREE builds

Nelson,

Indeed, the reason for ../.. was the standalone NSS build using cygwin . This
patch breaks it. The log of the failure is below.

mkdir WINNT5.0_DBG.OBJ/WINNT_SINGLE_SHLIB
cp /cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/Makefile
WINNT5.0_DBG.OBJ
/WINNT_SINGLE_SHLIB
cp /cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/manifest.mn
WINNT5.0_DBG.
OBJ/WINNT_SINGLE_SHLIB
cp /cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/config.mk
WINNT5.0_DBG.OB
J/WINNT_SINGLE_SHLIB
cd WINNT5.0_DBG.OBJ/WINNT_SINGLE_SHLIB && gmake FREEBL_CHILD_BUILD=1 \
 FREEBL_PARENT=/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl
CORE_DEPTH=/c
ygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/../../.. libs
gmake[1]: Entering directory
`/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freeb
l/WINNT5.0_DBG.OBJ/WINNT_SINGLE_SHLIB'
cl -FoWINNT5.0_DBG.OBJ/ldvector.obj -c -Od -Z7 -MD -W3 -nologo -GT -DXP_PC
-DSHL
IB_SUFFIX=\"dll\" -DSHLIB_PREFIX=\"\" -DSHLIB_VERSION=\"_3\"
-DSOFTOKEN_SHLIB_VE
RSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -D_DEBUG -UNDEBUG
-DDEBUG_Administ
rator -DWIN32 -D_WINDOWS -D_X86_ -DWINNT -DMP_ASSEMBLY_MULTIPLY
-DMP_ASSEMBLY_SQ
UARE  -DMP_ASSEMBLY_DIV_2DX1D -DMP_USE_UINT_DIGIT -DMP_NO_MP_WORD
-DMP_API_COMPA
TIBLE
-I/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/../../../../dist/WIN
NT5.0_DBG.OBJ/include 
-I/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/../
../../../dist/public/nss
-I/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/.
./../../../dist/private/nss
-I/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freeb
l -I/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl/mpi
-I/cygdrive/c/NSS/ti
p/mozilla/security/nss/lib/freebl/ecl 
/cygdrive/c/NSS/tip/mozilla/security/nss/
lib/freebl/ldvector.c
Command line warning D4002 : ignoring unknown option
'/cygdrive/c/NSS/tip/mozill
a/security/nss/lib/freebl/ldvector.c'
Command line error D2003 : missing source filename
gmake[1]: *** [WINNT5.0_DBG.OBJ/ldvector.obj] Error 2
gmake[1]: Leaving directory
`/cygdrive/c/NSS/tip/mozilla/security/nss/lib/freebl
/WINNT5.0_DBG.OBJ/WINNT_SINGLE_SHLIB'
gmake: *** [libs] Error 2

Comment 36

[Wan-Teh Chang]

Attachment: Enhance the makefile rule for $(MAPFILE) (checked in)

1. Do not hardcode the prerequisite for the $(MAPFILE)
rule as $(LIBRARY_NAME).def.  Add the variable MAPFILE_SOURCE
to represent that file and allow it to be overridden.  (The
next patch for lib/freebl will override MAPFILE_SOURCE.)

I welcome suggestions for a better name than MAPFILE_SOURCE.

2. In the PROCESS_MAP_FILE macro, use the variable $<
rather than the hardcoded $(LIBRARY_NAME).def to
refer to the prerequisite of the $(MAPEFILE) rule.

Comment 37

[Wan-Teh Chang]

Attachment: Additional patch for lib/freebl from Wan-Teh

This patch implements some of my suggested changes for
Sun's lib/freebl patch.

I eliminated the need to set the FREEBL_PARENT variable
for the recursive child builds.  This fixed the BUILD_TREE
build problem as a byproduct.

The most important change in this patch is to do the recursive
child builds in the lib/freebl directory and just change the
build output directory (OBJDIR).  This change enabled me to
not have to create symlinks or copies of any file.  Because
I don't need to change directory to do the recursive child
builds, I don't need to set their FREEBL_PARENT or CORE_DEPTH
variables.

I removed an unnecessary -L$(DIST)/lib from lib/freebl/config.mk.

I also removed the _ from the name of the "freebl_3" shared library.
My straightforward approach requires me to move the _ to the end
of the multiple freebl shared libraries, which produces ugly mapfile
names like freebl_32fpu_.def as Nelson pointed out in private email.
If that's a problem, I can fix it.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195821 [details] [diff] [review]
Additional patch for lib/freebl from Wan-Teh

Wan-Teh, I haven't reviewed your patch yet, but the trailing underscores in
file names are at least as objectionable as the _3 was, IMO.

If you have more cycles to spend on the Makefile, and desire to make further
improvements, would you consider separating the parent and child makefiles (as
you previously suggested) and eliminating config.mk, factoring its contents
into the separate parent and child Makefiles?

Comment 39

[Neil Williams]

Attachment: freebl Makefile CFLAGS changes for SPARC build

These changes remove the -fast option from the compile flags for SPARC freebl
build. That option is a macro which expands to options appropriate for the
machine the compiler happens to be running on at build time. These changes make
the built libraries reproducible on different SPARC implentations.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195819 [details] [diff] [review]
Enhance the makefile rule for $(MAPFILE) (checked in)

Looks good to me.  But please wait until we've declared Alpha done to check it
in.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195821 [details] [diff] [review]
Additional patch for lib/freebl from Wan-Teh

I really like this patch's simplification of the Makefiles.  The only reason
for sr- here is the trailing underscores in file names.

Comment 42

[Wan-Teh Chang]

Attachment: Additional patch for lib/freebl from Wan-Teh, v2 (checked in)

I fixed the file names of the generated mapfiles.

I don't have time to separate the makefiles into parent
and child makefiles in NSS 3.11.  Sorry.  But the changes
I implemented in this patch are within reach.

Comment 43

[Wan-Teh Chang]

Comment on attachment 195831 [details] [diff] [review]
freebl Makefile CFLAGS changes for SPARC build

Neil: you should use variables to store the compiler flags
that you duplicated for the freebl shared libraries.

> 	ifdef USE_ABI64_INT
> 	    # this builds for Sparc v9a pure 64-bit architecture
>-	    SOL_CFLAGS += -fast -xO5 -xrestrict=%all -xchip=ultra
>-	    SOL_CFLAGS += -mt
>-	    ARCHFLAG = -xarch=v9a
>-	    SOLARIS_AS_FLAGS = -xarch=v9a -K PIC
>+	    SOL_CFLAGS += -D__MATHERR_ERRNO_DONTCARE -fns -fsimple=2 -fsingle
>+	    SOL_CFLAGS += -xalias_level=basic -xbuiltin=%all
>+ 	    SOL_CFLAGS += -xcache=generic -xchip=generic -xdepend -xlibmil
>+ 	    SOL_CFLAGS += -xmemalign=8s -xO5 -xprefetch=auto,explicit
>+	    ARCHFLAG = -xarch=v9
>+	    SOLARIS_AS_FLAGS = -xarch=v9 -K PIC	endif
> 	endif

There is an extraneous "endif" after "-K PIC".

Comment 44

[Wan-Teh Chang]

Attachment: Wan-Teh's code cleanup patch for lib/freebl/loader.c (checked in)

1. Structure the ifdefs around the definitions of getLibName like
this:

  #if defined(SOLARIS) && defined(__sparc)
  ...
  #elif defined(HPUX) && !defined(NSS_USE_64) && !defined(__ia64)
  ...
  #else
  ...
  #endif

This removes a duplicate default case for Solaris x86 and x86_64.

2. Removed the comment "So call it even if fn_addr is NULL." because
fn_addr cannot be NULL.

3. Use the accurate size for the 'fullName' buffer.  The old size
is one byte too large.

4. Use memcpy and strcpy to fill up the 'fullName' buffer.

These four changes are mutually independent, so please indicate
which ones are okay if you don't like some of them.  I am most
interested in getting 1 and 2 in.

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195950 [details] [diff] [review]
Additional patch for lib/freebl from Wan-Teh, v2 (checked in)

r=nelson.  Saul has some additional review comments coming soon, I think.

Suggestion:  In manifest.mn, instead of adding 5 new lines that all read:
>+    LIBRARY_VERSION := _$(LIBRARY_VERSION)

you might make a change something like the following, 
right after the endif for FREEBL_CHILD_BUILD

>-ifndef LIBRARY_NAME
>-  LIBRARY_NAME = freebl
>+ifneq($(LIBRARY_NAME),freebl)
>+    LIBRARY_VERSION := _$(LIBRARY_VERSION)
> endif

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195831 [details] [diff] [review]
freebl Makefile CFLAGS changes for SPARC build

r-  
See Wan-Teh's comments in comment 43, especially the extra endif.
I think there is some value in being able to change the compiler flags
for one platform without worrying about unintended consequences on 
others, so having some command line options duplicated among the 
various Solaris Hybrid platforms doesn't seem completely unreasonable.
But I would like to see the lines that contain the common/duplicated
flags appear exactly identical in the platforms that use them, to 
make it visually easy to verify that they are actually identical.

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 195953 [details] [diff] [review]
Wan-Teh's code cleanup patch for lib/freebl/loader.c (checked in)

This patch seems fine to me, but please wait until we've built our alpha to
check it in.

Comment 48

[Saul Edwards]

Comment on attachment 195950 [details] [diff] [review]
Additional patch for lib/freebl from Wan-Teh, v2 (checked in)

This looks good.  I think the BUILD_TREE issue goes beyond just fixing the
freebl directory but I can't test your latest until the Mozilla cvs server
comes back up.

Comment 49

[Neil Williams]

Attachment: freebl Makefile CFLAGS changes for SPARC build

This patch incoporates suggestions from one of the compiler people as well as
suggestions from the reviewers (thanks Wan-Teh). Because there are only 2 cases
with substantial common flags I have not used a macro to define them. I
anticipate more changes to the individual cases in the next few weeks some of
which may change the common flags.

Comment 50

[Wan-Teh Chang]

Comment on attachment 196115 [details] [diff] [review]
freebl Makefile CFLAGS changes for SPARC build

Why do you use -xO4 in the "INT" builds and -xO5 in the "FPU" builds?
These individual flags could become a maintenance nightmare.  Is there
no compiler flag that will specify those for us?

I found the following two paragraphs from Sun Studio 10 cc man page
(http://developers.sun.com/prodtech/cc/documentation/ss10_docs/mr/man1/cc.1.htm
l)
that should be useful:

	  Modules that are compiled with -fast must also be
	  linked with -fast. For a complete list of compiler
	  options that must be specified at both compile time and
	  at link time, see the C User's Guide.

	  The -fast option is unsuitable for programs that are
	  intended to run on a different target than the compila-
	  tion machine. In such cases, follow -fast with the
	  appropriate -xtarget option. For example:

		% cc -fast -xtarget=ultra

The second paragraph above seems to suggest a solution for the
problem you are trying to solve.  The paragraph below further
clarifies the situation:

	  These options are turned on for -fast:

	  -fns
	  -fsimple=2
	  -fsingle
	  -ftrap=%none
	  -nofstore (x86)
	  -xalias_level=basic
	  -xbuiltin=%all
	  -xdepend
	  -xlibmil
	  -xlibmopt
	  -xmemalign=8s (SPARC)
	  -xO5
	  -xprefetch=auto,explicit (SPARC)
	  -xregs=frameptr (x86)
	  -xtarget=native

So I believe only the -xtarget=native flag in the expansion of
-fast is problematic, and can be addressed by a -xtarget=<foo>
flag after -fast.

Comment 51

[Neil Williams]

(In reply to comment #50)
> (From update of attachment 196115 [details] [diff] [review] [edit])
> Why do you use -xO4 in the "INT" builds and -xO5 in the "FPU" builds?
> These individual flags could become a maintenance nightmare.  Is there
> no compiler flag that will specify those for us?

It IS a little ugly. The INT builds are mainly for a new cpu that the compiler
doesn't know about. The compiler group suggested these values. They did indeed
improve performance

> I found the following two paragraphs from Sun Studio 10 cc man page
> 
 ~snip~
> 
> So I believe only the -xtarget=native flag in the expansion of
> -fast is problematic, and can be addressed by a -xtarget=<foo>
> flag after -fast.
> 

Thanks for pointing this out. This may work for the FPU builds. I'll try it out.

Comment 52

[Robert Relyea]

There's a small problem with this patch as applied to the trunk. It's not really
an issue for Alpha, but should addressed by beta.... It breaks mozilla builds.

There are certain features of our build system which allow us to point to source
or target binaries to various directories. Mozilla uses these features to drop
the binaries in a mozilla specific place (Mozilla does not intertwine their
binaries and source directories so they can build from source in readonly file
systems). Anyway mozilla breaks with the following error if built using the tip
of NSS:

cd freebl; /usr/bin/make -j1 libs
make[5]: Entering directory `/cygdrive/c/builds/ff.tip/mozilla/security/nss/lib/
freebl'
nsinstall -m 664 c:/builds/ff.tip/mozilla/xulrunner-dbg/nss/freebl/freebl.lib c:
/builds/ff.tip/mozilla/xulrunner-dbg/dist/lib
cd c:/builds/ff.tip/mozilla/xulrunner-dbg/nss/freebl/WIN95_SINGLE_SHLIB && /usr/
bin/make -j1 FREEBL_CHILD_BUILD=1 \
 FREEBL_PARENT=../.. CORE_DEPTH=../../../../.. libs
make[6]: Entering directory `/cygdrive/c/builds/ff.tip/mozilla/xulrunner-dbg/nss
/freebl/WIN95_SINGLE_SHLIB'
Makefile:51: ../../../../../coreconf/config.mk: No such file or directory
Makefile:365: ../../../../../coreconf/rules.mk: No such file or directory
make[6]: *** No rule to make target `../../../../../coreconf/rules.mk'.  Stop.
make[6]: Leaving directory `/cygdrive/c/builds/ff.tip/mozilla/xulrunner-dbg/nss/
freebl/WIN95_SINGLE_SHLIB'
make[5]: *** [libs] Error 2

bob

Comment 53

[Robert Relyea]

oops sorry, I needed to read comment 33 from wtc. He already has a patch for
this problem... (Silly me to think I would be the only one to try to build
mozilla with the latest version of NSS;).

bob

Comment 54

[Wan-Teh Chang]

Comment on attachment 195819 [details] [diff] [review]
Enhance the makefile rule for $(MAPFILE) (checked in)

I've checked this patch in so we can fix the BUILD_TREE build
problem.  Bob, you only need to review changes to the last two
files (rules.mk and ruleset.mk), in particular the name of the
new variable SOURCE_MAPFILE, and look at one of the other
XXXOS.mk files (use $< to refer to the prerequisite of the
makefile rule).  This patch allows a library to have a "source"
mapfile whose name is not $(LIBRARY_NAME).def.	This allows
all the freebl shared libraries to use freebl.def as their
"source" mapfile.

Comment 55

[Wan-Teh Chang]

Attachment: Better way to set LIBRARY_VERSION for freebl shared libraries

This patch implements a variant of Nelson's suggested
change in comment 45.

I use the existence of '_' in the $(LIBRARY_NAME), rather
than comparing the $(LIBRARY_NAME) to "freebl", to decide
if we need to prefix LIBRARY_VERSION with '_'.

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196375 [details] [diff] [review]
Better way to set LIBRARY_VERSION for freebl shared libraries

This seems OK.

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Add version info to freebl shared libs

This patch builds the .res file from the .rc file and
includes the rcsid and sccsid strings

Comment 58

[Nelson Bolyard (seldom reads bugmail)]

Attachment: replace "fast" and "slow" with fpu and int in loader.c

This patch makes a change to variable names that Wan-Teh requested.
With the new names, it never appears that we're choosing the slow 
path instead of the fast one.

Comment 59

[Wan-Teh Chang]

Comment on attachment 196727 [details] [diff] [review]
Add version info to freebl shared libs

This patch is missing the new file freebl.rc, which
is only used on Windows.

>Index: ldvector.c
...
> #include "loader.h"
> #include "alghmac.h"
>+#include "nss.h"

This file doesn't need to include "nss.h" because you
didn't add a FREEBL_VersionCheck function that uses
NSS_VersionCheck (which is declared in "nss.h").

Comment 60

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196740 [details] [diff] [review]
replace "fast" and "slow" with fpu and int in loader.c

Wan-Teh please review this patch.

Comment 61

[Nelson Bolyard (seldom reads bugmail)]

Attachment: change envariables for nolocks feature. make fdx and nolocks mutually exclusive.

I'm attaching this patch to facilitate testing.  Not requesting review yet.

Comment 62

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Add version info to freebl shared libs (v2) (checked in)

This patch includes the missing .rc file and incorporates Wan-Teh's feedback.

Comment 63

[Wan-Teh Chang]

Comment on attachment 196752 [details] [diff] [review]
Add version info to freebl shared libs (v2) (checked in)

r=wtc.	In freebl.rc, we have

>+#define MY_FILEDESCRIPTION "NSS freebl Library"

It may look nicer to capitalize "freebl" or use a
more descriptive name like "NSS Crypto Library" or
"NSS Low-Level Crypto Library".

Comment 64

[Wan-Teh Chang]

Comment on attachment 196740 [details] [diff] [review]
replace "fast" and "slow" with fpu and int in loader.c

r=wtc.	You should restore the comment that described the
code below:

>+    found_int_hybrid = strstr(buf, int_hybrid_isa);
>+    found_fpu_hybrid = strstr(buf, fpu_hybrid_isa);
>+    if (found_fpu_hybrid && 
>+	(!found_int_hybrid ||
>+	 (found_int_hybrid - found_fpu_hybrid) >= 0)) {
>+	return fpu_hybrid_shared_lib;
>     }

Originally, that comment was confusing because of the old
variable names that used "fast" and "slow".

Does Solaris return the SI_ISALIST string in the decreasing
order of performance or some other measure of desirability?
That's what your code implies.	So a comment would help clarify
the purpose of the "(found_int_hybrid - found_fpu_hybrid) >= 0"
test.

It seems that you should check if you get a substring match
and do another strstr call to get the full string match.

Comment 65

[Julien Pierre]

Regarding the code in loader.c, a problem was identified during our QA. The
situation was that a product had symlinks to the old NSS 3.10 library names,
including libfreebl_hybrid3.so and libfreebl_pure32.so .

When dropping in NSS 3.11 and not adding new symlinks to the products, the new
loader failed to find libfreebl on Linux, but succeeded on Solaris.

This is because PR_GetLibraryFilePathname returned the name of the symlink to
libsoftokn3.so on Linux; but on Solaris it returned the original filename.

We are contacting the product team and telling them to stop making symlinks to
our private libraries. However, we would also make the upgrade seamless for our
customers and try to fix things on our side. The fix involves trying to get the
original filename if the pathname is a symlink. This fix can be made either in
loader.c or in NSPR . I suggest we do the later, so that we have a consistent
implementation of PR_GetLibraryFilePathname accross platforms. 

Comment 66

[Wan-Teh Chang]

When libsoftokn3.so is a symlink, it's not obvious whether
we should use the symlink or the symlink's target to locate
related files.

Comment 67

[Julien Pierre]

Re: comment #66,

I believe that no product has ever packaged libsoftokn and libfreebl in
different locations, so using the target of the link should always be correct.
We should state to our NSS users that this is a requirement, but I think
everybody already meets it.

There is another issue in loader which was discussed via email, and IMO should
have been taking place here. It is about the correctness of the usage of
PR_GetLibraryFilePathname, and the documentation requirement for both the
function pointer and pathname argument to match one library - a requirement
which is not currently inforced in any NSPR implementation.

Last week I offered three different approaches to abide by the NSPR
documentation. I'll just copy them here.

1) Since bl_loadLibrary doesn't currently know if it's the instance from
libsoftokn3.so or libssl3.so, it can just make two calls to
PR_GetLibraryFilePathname, one with the name of libsoftokn3 and another with the
name of libssl3 . One of the two calls will pass a combination of a function
pointer in library X and the name of library A, where X=A half the time and X=B
half the time. As far as I know, PR_GetLibraryFilePathname isn't defined to
crash if X != A , but rather merely to return NULL, so this isn't a fatal error.
The worst possible consequence is that PR_GetLibraryFilePathname will return
NULL, ie. the pathname won't be found. Wan-Teh, please correct me if this is wrong.

If the first call to PR_GetLibraryfilePathname returns NULL, then loader can
make a second call to it, with the same pointer in library X, and the name of
library B . The probability that one of the two PR_GetLibraryFilePathname
follows the proper NSPR semantics is 100%, since we know by design that X is
either A or B .

Even if the second PR_GetLibraryFilePathname call fails (for reason other than
invalid arguments - for example the unimplemented case on BeOS or program
changing directory on AIX that you mentioned last week), it still isn't a fatal
error, as we fallback to using PR_LoadLibrary with an unqualified filename .

As long as we document what both of these PR_GetLibraryFilePathname calls do, I
think it is fine.
In the current implementation, only the first call is really needed - I'm only
proposing we add the second call in order to make sure we are a model NSPR user
of PR_GetLibraryFilePathname at least once for each bl_LoadLibrary call .

2) we can compile two copies of loader.c, one for libsoftokn and one for libssl
. Each one will know the name of the shared library it's being linked to, and
there will only be one call to PR_GetLibraryFilePathname.
This might be accomplished with an #include "loader.c" and macros in each
Makefile ...

3) we can add a bl_Initialize function and have it take an argument that sets
the name of the calling library into a static variable .

I think 1) is simplest to implement and test .

There is some additional information about these possible fixes.

1) We still have a few tools that are statically linked. They include rsaperf
and bltest, which are not shipped and used only for QA; but also signtool, which
is shipped.

In these tools, loader.c will be linked to the executable program, rather than a
library. PR_GetLibraryFilePathname won't work. Is there a standardized way to
find the executable name  ? I don't think argv[0] is usable here in loader,
since it may be part of a library.

However, as long as they are internal QA tools, they can rely on LD_LIBRARY_PATH
being set. That leaves signtool, which would need to be fixed to find the path
to libfreebl correctly. I don't think any of the 3 possible fixes above works
for it. I think the only proper fix is to make it stop using static libraries.
This is the subject of bug 263948 .

Comment 68

[Robert Relyea]

Comment on attachment 195819 [details] [diff] [review]
Enhance the makefile rule for $(MAPFILE) (checked in)

I'm sure this is already checked in.
It looks fine to me.

Comment 69

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196740 [details] [diff] [review]
replace "fast" and "slow" with fpu and int in loader.c

Checked in the patch that replaced "fast" and "slow" with "fpu" and "int"
respectively.  Also added a comment explaining ISA lists.

Checking in loader.c;  new revision: 1.25; previous revision: 1.24

Comment 70

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196751 [details] [diff] [review]
change envariables for nolocks feature.  make fdx and nolocks mutually exclusive.

Oops, attached this to the wrong bug!

Comment 71

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196752 [details] [diff] [review]
Add version info to freebl shared libs (v2) (checked in)

Checking in freebl.rc;	  initial revision: 1.1
Checking in freeblver.c;  initial revision: 1.1
Checking in config.mk;	  new revision: 1.14; previous revision: 1.13
Checking in ldvector.c;   new revision: 1.13; previous revision: 1.12
Checking in manifest.mn;  new revision: 1.43; previous revision: 1.42

Comment 72

[Wan-Teh Chang]

Our build tree is a good example of a case where we
want to use the symlink to locate related files.
The .so's in mozilla/dist/xxx.OBJ/lib are symlinks
that point to targets in various directories.  Because
the dladdr function on Solaris resolves symlinks, we
can't remove the seemingly unnecessary second
PR_LoadLibraryWithFlags call in loader.c.

NSS should deal with this inconsistency of
PR_GetLibraryFilePathname between platforms.  It's too
late to change PR_GetLibraryFilePathname now.

Comment 73

[Julien Pierre]

re: comment #72,

You made the case yourself in our meeting a few weeks ago that
PR_GetLibraryFilePathname may have odd behavior in some programs that change
directories on AIX (and other circumstances I don't remember exactly). Given
this, I don't think we should remove the second PR_LoadLibraryWithFlags call -
the one with the unqualified path. As you point out, that also fixes our build
tree case, which I believe is the only case that anyone tries to run binaries
for libsoftokn, libssl and libfreebl that aren't all stored in the same directory.

Re: changing PR_GetLibraryFilePathname : the behavior of this function simply
hasn't been defined when it comes to symlinks, and the fact there is currently a
difference in platform behavior shows that existing applications should be
prepared to deal with either case. Clarifying the behavior of
PR_GetLibraryFilePathname to choose one of the two cases shouldn't break
compatibility, since existing apps couldn't have been depending on one case or
the other before.

Comment 74

[Neil Williams]

Attachment: freebl Makefile CFLAGS changes for SPARC builds

Is this what you had in mind, Wan-Teh?

Comment 75

[Wan-Teh Chang]

Comment on attachment 196965 [details] [diff] [review]
freebl Makefile CFLAGS changes for SPARC builds

Neil, yes, the comments you added are exactly what I
had in mind.  Thanks.  Please ask a Sun NSS developer
to review this patch for its correctness.

Comment 76

[Neil Williams]

Comment on attachment 196965 [details] [diff] [review]
freebl Makefile CFLAGS changes for SPARC builds

I didn't explicitly ask for a review of this patch because only comments had
changed since the previous review was granted. Nelson would you do the honors?

Comment 77

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196115 [details] [diff] [review]
freebl Makefile CFLAGS changes for SPARC build

cancelling review request for obsoleted patch.	(I surely wish that bugzilla
would do that automatically).

Comment 78

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196965 [details] [diff] [review]
freebl Makefile CFLAGS changes for SPARC builds

Saul and I have double-checked this.  r=nelson.  
Neil, Go ahead and check it in.

Comment 79

[Neil Williams]

Committed https://bugzilla.mozilla.org/attachment.cgi?id=196965 as revision 1.66

Comment 80

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Replace IS_64 with new symbol that says "use 64 bit registers in 32-bit build"

This patch creates a new defined symbol NSS_USE_64BIT_REGS which means
"This is a 32-bit ABI build running on a 64-bit CPU so 64-bit integer 
arithmetic and 64-bit loads and stores are fast."   This patch uses it in 
places where the trunk is erroneously using IS_64 to mean "this CPU has 
64-bit registers", in arcfour and in sha code (including the PRNG).

Note that this patch does NOT define that symbol for sparc family CPUs.
We need to redo the experiments (last done 4 years ago) to determine 
whether modern sparc family CPUs benefit from this or not.  I have put
the necessary symbols into the Makefile, but commented out, to facilitate
this testing.	

As written, the only net effect of this patch should be as follows:
a) on sparc 64-bit builds, sha1 and the PRNG will now use 64-bit variables.
b) on HPUX 32-bit "hybrid" builds, arcfour will now use 64-bit variables.

Comment 81

[Julien Pierre]

We are missing a rule to automatically regenerate mp_comba_amd64_sun.s . The C
source that only compiles with gcc is bound to change over time. We can add
these rules to the mpi Makefile, and model them after the rules to create
mpv_sparcv8.s and mpv_sparcv9.s .

Comment 82

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #81)
> We are missing a rule to automatically regenerate mp_comba_amd64_sun.s . 

That's because that file is not produced automatically.  It is hand edited
output from the compiler, hand edited to remove a bunch of extraneous stuff
(mostly debug info which the gcc compiler seems to produce when making a .s 
output file, whether you've specified -g or not), and to fix/eliminate
expressions that involve differences between addresses in different segments.
It's too involved to attempt to automate, IMO (except by changing gcc).

Comment 83

[Julien Pierre]

In that case, we need to have detailed instructions about how to regenerate the
file manually, so that the process can be replicated.

Comment 84

[Nelson Bolyard (seldom reads bugmail)]

Do we need to have detailed instructions about how to write all the code in NSS?
How about for the floating point code?  

Comment 85

[Julien Pierre]

There should be some rules or documentation about any .s file generated from the
C compiler output. There are already rules to generate usable floating point .s
files !

I think the problem with the new .s file I mentioned is even more serious due to
the size of the C source and the .s file. If a bug fix ever needs to be done to
the C source - and the larger the C file, the higher the likelihood of that -,
we should be able to easily rebuild the NSS libraries with the fix, without
having to do a lot of research and guesswork on how to build a new .s file. If
the process is overly complicated, then we should at least record what was done
the first time, so it can be done again. We might also reconsider the use of
such large .s files if their use is not sustainable in the long run.

Comment 86

[Saul Edwards]

*** Bug 305151 has been marked as a duplicate of this bug. ***

Comment 87

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197136 [details] [diff] [review]
Replace IS_64 with new symbol that says "use 64 bit registers in 32-bit build"

Saul, you've evaluated this patch.  Please give your evaluation comments here. 
If you think it's ready to go, please ask someone for SR.

Comment 88

[Saul Edwards]

Comment on attachment 197136 [details] [diff] [review]
Replace IS_64 with new symbol that says "use 64 bit registers in 32-bit build"

I tested the performance of this patch on ultrasparc 3 (v440) and a target
architecture for the abi32_int64 build.  I used blapitest for the crypto code
affected by the NSS_USE_64BIT_REGS, which is RC4 and SHA1.  The results on the
v440 showed no changes on SHA1, but a good 5-10% improvement in RC4 for the
"hybrid" build.  With the mystery architecture, building with
NSS_USE_64BIT_REGS was always a wash.

I recommend that we uncomment the DEFINE in the abi32_fpu build but leave the
abi32_int64 build as is.  Otherwise r+.

Comment 89

[Wan-Teh Chang]

Comment on attachment 197136 [details] [diff] [review]
Replace IS_64 with new symbol that says "use 64 bit registers in 32-bit build"

Saul, Nelson:

I'm sorry that my review comments are verbose.	But I
must point out your misunderstanding of the meaning of
the USE_64 build variable, which implies that the
NSS_USE_64 macro is not defined and used correctly.

If a system supports both 32-bit and 64-bit builds,
by default we do 32-bit builds and we set USE_64=1
to do 64-bit builds.

However, if a system supports 64-bit builds only,
NSPR and NSS/coreconf do not require setting USE_64=1.
An example of such a system is OSF1 on Alpha.  I
believe Linux on Itanium (ia64) is the same, too.

So, this definition of NSS_USE_64 in lib/freebl/Makefile
is incorrect:

 ifdef USE_64
	DEFINES += -DNSS_USE_64
 endif

because we fail to define NSS_USE_64 on 64-bit builds
only systems such as OSF1 on Alpha.  This is your first
misunderstanding.  My recommendation is to remove the
above definition of NSS_USE_64 and use the NSPR macro
IS_64 instead.	NSPR's IS_64 macro means what you intend
NSS_USE_64 to mean, and is defined where necessary if
you include any NSPR header file.

The second misunderstanding is that a 64-bit build uses
the LP64 (long and pointer are 64-bit) data model.  This
is almost always true, with only one known exception:
64-bit Windows.  In 64-bit Windows, long is still 32-bit.
Only a pointer is 64-bit.

In arcfour.c, you have:

>-#if (defined(IS_64) && !defined(__sparc)) || defined(NSS_USE_64)
>+#if defined(NSS_USE_64BIT_REGS) || defined(NSS_USE_64)
> /* 64-bit wordsize */
> #ifdef IS_LITTLE_ENDIAN
> #define ARCFOUR_NEXT_WORD() \
> 	{ streamWord = 0; ARCFOUR_NEXT4BYTES_L(0); ARCFOUR_NEXT4BYTES_L(32); }
> #else

Here, NSS_USE_64 would be true for 64-bit Windows, but
64-bit Windows's WORD would be unsigned long, which is
32-bit.  So, in the above code, you should replace
defined(NSS_USE_64) by (PR_BYTES_PER_LONG == 8).  Note
that the following use of NSS_USE_64 in sha_fast.h is fine
because PRUint64 is unsigned long long on 64-bit Windows
(again, I suggest using NSPR's IS_64 macro instead):

>-#if defined(IS_64) && !defined(__sparc) 
>+#if defined(NSS_USE_64BIT_REGS) || defined(NSS_USE_64)
> typedef PRUint64 SHA_HW_t;
> #define SHA1_USING_64_BIT 1
> #else
> typedef PRUint32 SHA_HW_t;
> #endif

Finally, you should add a comment to lib/freebl/Makefile to
explain why you commented out #DEFINES += -DNSS_USE_64BIT_REGS
for Solaris on SPARC.

Comment 90

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, thanks for your useful feedback.  One question: you mentioned LP64,
but I think we eliminated our dependencies on it.  That is, in a previous
patch we added references to LP64, but I believe the current (and recently
commmitted) patches do not.  So, what reference to LP64 triggered that comment?

Comment 91

[Wan-Teh Chang]

Comment on attachment 197136 [details] [diff] [review]
Replace IS_64 with new symbol that says "use 64 bit registers in 32-bit build"

Nelson, your changes to arcfour.c are what triggered my
mention of LP64:

>-#if defined(IS_64) && !defined(__sparc) && !defined(NSS_USE_64) 
>+#if defined(NSS_USE_64BIT_REGS)
> typedef unsigned long long WORD;
> #else
> typedef unsigned long WORD;
> #endif
> #define WORDSIZE sizeof(WORD)

and

>-#if (defined(IS_64) && !defined(__sparc)) || defined(NSS_USE_64)
>+#if defined(NSS_USE_64BIT_REGS) || defined(NSS_USE_64)
> /* 64-bit wordsize */
> #ifdef IS_LITTLE_ENDIAN
> #define ARCFOUR_NEXT_WORD() \
> 	{ streamWord = 0; ARCFOUR_NEXT4BYTES_L(0); ARCFOUR_NEXT4BYTES_L(32); }
> #else

WORD is unsigned long for non-NSS_USE_64_BIT_REGS cases,
including the NSS_USE_64 case.	But 64-bit Windows (for
which NSS_USE_64 would be defined) has a 32-bit long,
so it is not always true that defined(NSS_USE_64) implies
64-bit wordsize.

Comment 92

[Wan-Teh Chang]

Comment on attachment 197136 [details] [diff] [review]
Replace IS_64 with new symbol that says "use 64 bit registers in 32-bit build"

You can check in this patch.  The problems I pointed out
can be remedied as follows:

1. Always set USE_64=1 when doing 64-bit builds, even when
it's not required.  The only problem is that OBJDIR_NAME
will have an extraneous _64 tag for the 64-bit only platforms
such as OSF1 and Linux/ia64.

2. In arcfour.c, 

>+#if defined(NSS_USE_64BIT_REGS) || defined(NSS_USE_64)
> /* 64-bit wordsize */
> #ifdef IS_LITTLE_ENDIAN
> #define ARCFOUR_NEXT_WORD() \
> 	{ streamWord = 0; ARCFOUR_NEXT4BYTES_L(0); ARCFOUR_NEXT4BYTES_L(32); }
> #else

change defined(NSS_USE_64) to
(PR_BYTES_PER_LONG == 8) or, if you don't want to use NSPR macros,
(defined(NSS_USE_64) && !defined(_WIN64)) when we port NSS to
64-bit Windows.

Comment 93

[Julien Pierre]

Re: the symlink issue discussed in comments 66-67 and 72-73,

Christophe determined through testing with LD_LIBRARY_PATH / SHLIBPATH / LIBPATH
pointing to /tmp containing symlinks only to the public libraries, but not to
the private libs, that certutil only loads successfully on Solaris. This is
because PR_GetLibraryFilePathname finds the path of the original libsoftokn3 .
On the other Unix platforms - Linux, HP-UX (both 32/64) and AIX, certutil fails
to load, presumably because the path of the symlink to libsoftokn3 is found .

Christophe would like us to have a fix for this problem checked in for NSS 3.11
beta 2 by tomorrow midnight for integration with products on wednesday.

I think the behavior on Solaris is the correct one, and we need to fix the case
of all the other Unices. I have proposed that we make PR_GetLibraryFilePathname
always find the original library rather than the symlink. This change will not
break any existing portable application, due to the existing difference in
behavior accross platform.

If we aren't willing to do that, then we should have a function to resolve a
pathname to an original (if the pathname is not of a link, it would just return
the original), eg. char* PR_GetOriginalPathname(const char*). Given that its
implementation will be non-portable code, I think this function belongs in NSPR.

Comment 94

[Julien Pierre]

Attachment: patch to fix issue in programs that use symlinks

This is a quick and dirty attempt to fix this problem. I just compiled it and
tested it on Linux successfully. The PR_ function really belongs in NSPR rather
than loader.c / libfreebl.a, and conditional compilation is needed since
readlink() only exists on Unix .

Comment 95

[Wan-Teh Chang]

Comment on attachment 199893 [details] [diff] [review]
patch to fix issue in programs that use symlinks

You may need to call readlink in a loop to handle the
case that the symlink points to another symlink.
(I don't know how dladdr on Solaris resolves the symlink,
so I'm not sure if the loop is necessary.)

You don't need to zero the entire returned buffer.  You just
need to store the null byte at the end of the pathname, whose
index is the return value of readlink (if positive).  See
the sample code at
http://www.opengroup.org/onlinepubs/009695399/functions/readlink.html.

Comment 96

[Wan-Teh Chang]

Not all NSPR-based applications run on Solaris and
some other Unix/Linux platform.  For example, it's
reasonable for an NSPR-based client application to
only support Windows and Mac OS X.

The behaviors of Solaris and the other Unix/Linux
platforms both make sense in some situations.  The
non-Solaris behavior returns more information because
with that you can emulate the Solaris behavior, but
not the other way around.  In any case, since NSPR
usually has to implement the lowest common denominator,
we have no choice but to standardize on the Solaris behavior.
We can add a new function PR_GetOriginalLibraryFilePathname
in which the ambiguity regarding symlinks is eliminated.
For NSS 3.11 we need to resolve the symlink in NSS.

Comment 97

[Julien Pierre]

Attachment: new patch for symlinks issue

Wan-Teh,

On Solaris, dladdr appears to always resolve to the original pathname, but the
additional function I wrote is harmless. I agree it's OK to put it in to freebl
for 3.11, but the function should be moved to NSPR for 4.7 / NSS 3.12 .

This new patch addresses the issues of conditional compilation, loop, and
removes the memset. I tested it successfully on Linux and Solaris .

Comment 98

[Wan-Teh Chang]

Julien: sorry my comment about dladdr wasn't clear.
I meant that I am not sure if Solaris dladdr follows only
one symlink, or it follows all the symlinks (if
the symlink points to another symlink, which
points to yet another symlink).  We need to know
what Solaris dladdr does so we can emulate its
behavior with the _pr_GetOriginalPathname function on
other platforms.

Comment 99

[Julien Pierre]

Wan-Teh,

I understood your comment, and I tested with 4 levels of symlinks. dladdr
resolves them all at once.

Comment 100

[Wan-Teh Chang]

Comment on attachment 200026 [details] [diff] [review]
new patch for symlinks issue

This patch has an off-by-one error (strlen(link) should
be strlen(link)+1).  Other than that, it is correct.  I
will attach a new patch to make my suggested changes clear.

Comment 101

[Wan-Teh Chang]

Attachment: patch for symlinks issue, v3

I made some code formatting, comment, and variable/function
name changes.

The two essential changes are:
1. Fix the off-by-one error in the buffer length 'len'.
2. Do not call bl_GetOriginalPathname with a NULL argument
to avoid losing the error code set by PR_GetLibraryFilePathname.
(bl_GetOriginalPathname would set the error code to
PR_INVALID_ARGUMENT_ERROR.)

Comment 102

[Julien Pierre]

Thanks for correcting the off-by-one error. Saul pointed it out to me last
night, but I was waiting for your feedback to produce a new patch.
There is actually another issue remaining - a possible infinite loop in the
symlink resolution, if somebody created links that point to each other
indirectly - ie. A -> B and B -> A. I created such a loop manually in the shell,
and then used ls -H to resolve it, and got the error :
Number of symbolic links encountered during path name traversal exceeds MAXSYMLINKS
I think we should use the same approach (limit traversal iterations) to prevent
this problem.

Comment 103

[Wan-Teh Chang]

Julien:

You are right.  Unix has an ELOOP error code for this
condition.  Here is an excerpt from the open man page
http://www.opengroup.org/onlinepubs/009695399/functions/open.html:

[ELOOP]
    More than {SYMLOOP_MAX} symbolic links were encountered
    during resolution of the path argument.

{SYMLOOP_MAX} seems to be a value that you need to look up
using sysconf.  If there is no suitable system macro defined,
we can just use our own maximum of 8 or 16.

Comment 104

[Wan-Teh Chang]

On Linux and HP-UX, I found that the macro MAXSYMLINKS
is defined in <sys/param.h> with a value of 20.  So we
can try using that macro or use that value.

Comment 105

[Julien Pierre]

Wan-Teh,

16 or 20 seems like a reasonable maximum.
I think ELOOP might be returned from readlink() only if the link it is trying to
resolve directly points to itself (I'm not sure how that's possible to create).
But the loop in GetOriginalPathname needs to take additional action to prevent
further recursivity.
This won't be a common case. In order for the loader code to be executed, the
libssl3.so or libsoftokn3.so that contains it has to be already successfully
loaded in memory, ie. there can't be any symlink loop. The problem could only
occur if this symlink loop was created after the program loaded the
libssl3/libsoftokn3 library implicitly, but before it called
GetOriginalPathname, which in most programs is probably a fairly short window
(but could happen in the browser case, since it delays NSS initialization).

Comment 106

[Julien Pierre]

Attachment: limit iterations to 20

Comment 107

[Julien Pierre]

I checked in the patch for the symlink resolution.

Checking in loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.26; previous revision: 1.25
done

What work is left to do on this bug ?

Comment 108

[Julien Pierre]

Per our meeting today, marking fixed. I don't think there are any remaining issues with the new freebl shared libs, but if we find some, new bugs can be opened for them.