485527 - Rename the _X86_ macro in lib/freebl

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

The attached patch implements the cleanup Julien suggested
in bug 431958 comment 32.

1. _X86_ is renamed NSS_X86.  NSS_X86 will still confuse people
who consider x86 and x86-64 different, but at least we won't
be overloading the _X86_ macro used by Windows system headers.

2. In sha512.c, we are using

    (defined(_X86_) && !defined(__x86_64__))

to test for 32-bit x86 CPUs.  This test fails to cover the
predefined macros used by Sun Studio (__x86_64) and Visual
C++ (_M_AMD64 or the newer _M_X64).  I replaced this test
with a test for predefined macros for 32-bit x86 CPUs used
by GCC, Sun Studio, and Visual C++:

    defined(__i386__) || defined(__i386) || defined(_M_IX86)

3. In mpi/mpcpucache.c, I made the following changes:
- Removed __X86__, which is unfortunately a typo (note the
  double underscore prefix and suffix).
- I didn't use NSS_X86 in this file because the NSS_ prefix
  looks out of place in mpi.  Instead, I follow the current
  code and test the predefined macros used by GCC, Sun Studio,
  and Visual C++.  For CPU foo, these macros look like
  __foo__, __foo, and _M_FOO, respecticaly.

Comment 1

[Julien Pierre]

Comment on attachment 369676 [details] [diff] [review]
Proposed patch

Wan-Teh,

This looks fine.

Good catch about sha512.
I ran some tests and unfortunately it did not improve the sha-512 performance with studio 12. The performance remained the same. I guess the assembly optimizations are really needed.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

There's a lot of testing to be done to be sure that this patch is correct
and has no unintended effects on any platform.  Unless someone is 
volunteering to do all that before Monday, I propose we target this for 
NSS 3.12.4.

Comment 3

[Julien Pierre]

Nelson,

Almost everything in the patch is very straightforward replacement of _X86_ with NSS_X86, except . That can be verified just by looking at the diff, no other work is needed.

The changes in sha512.c and mpcpucache.c are not as straightforward, but I believe they are correct too.

I agree that these changes can go in 3.12.4, but I don't think this requires a lot of testing.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, as you've noted, the old symbol _X86_ could be set by the compiler
or by the makefile.  The new symbol will never be set by the compiler.
Without significant review and/or testing, I cannot be sure that the new
symbol is defined in all the platforms where the old one was defined.

Comment 5

[Wan-Teh Chang]

The only reason I proposed target milestone 3.12.3 is that
this patch modifies lib/freebl, so it needs to be applied
before the FIPS algorithm testing starts.

After this patch is applied, there is no "_X86_" in
lib/freebl and its subdirectories, so it's easy to verify
the completeness of this patch.

I can prepare a straightforward version of this patch that
does nothing but macro renaming, if you prefer.  (I won't
modify mpcpucache.c in that case because its __X86__ macro
is a typo.)

In any case, I have learned what _X86_ means in lib/freebl,
so I can live with the current code.  This cleanup is for
future NSS maintainers.

Comment 6

[Julien Pierre]

Nelson,

In all my testing earlier this week, I didn't find any compiler that defined _X86_ . The only case where that symbol was defined or used by anything other than coreconf or the freebl Makefile was the Windows 32 bit system header files.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Julien, yes, you're right, it's system header files, not the compiler.
But the argument is the same.  The system header files will never define
NSS_X86, SO, if there is a platform (CPU, OS, compiler and ABI combination)
on which _X86_ is defined by the system header files and NOT by the Makefile,
then the logic that is conditional on that symbol will not be defined if 
the symbol is renamed.  This fact merely means that we must be sure that 
NSS_X86 is defined in all cases where _X86_ is also defined.  Determining 
if that is the case, or not, requires significant additional review and/or
testing, as you recently did for another patch for other reasons.

Comment 8

[Julien Pierre]

Nelson,

I tested all our our officially supported x86 platforms except Mac as part of my previous work. Only Windows was an issue with the system header files defining the symbols.

If some other unofficial x86 platform defined _X86_ before, and doesn't now have NSS_X86 defined, then yes, the behavior and performance would change. I don't think we can be worried about unofficial platforms at this point, and they wouldn't be broken, just possibly slower (or faster).

I only tested actual x86 platforms. If some non-x86 platform defined _X86_, then there would be a behavior change too, but I think it would be a desirable one.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Julien,  IIRC, your recent tests focused on the builds where the _X86_ 
symbol is NOT defined by system header files.  If that recollection is 
correct, then one cannot infer from those results whether our Makefiles
also set _X86_ in all builds wherein the symbol is also defined by 
system header files.  

However, If you are saying that, based on your own recent testing, you 
are sure that _X86_ is defined today by our Makefiles in all builds 
where it is also defined in system/compiler header files, that's good.

Are you saying that?

Comment 10

[Julien Pierre]

Nelson,

When I worked on bug 431958, I tested x64 platforms that both had and did not have _X86_ defined, in order to determine whether it was best to define it or not. The final determination was that it was preferable overall.

I didn't document this, but when doing that testing, I had code like :

#ifdef _X86_
#error x86 !
#else
#error no x86 !
#endif

in des.c .

This allowed me to find out exactly when the macro was defined and not defined, and on which platforms.

I also used the following test program to find out what symbols the various compilers defined, both in 32 and 64 bits :

int main()
{
#if defined(_X86_)
#error _X86_
#elif defined(__i386__)
#error __i386__
#elif defined(__x86_64__)
#error __x86_64__
#else
#error nothing
#endif
        return 0;
}

Thus, I can confidently tell you that changing _X86_ to NSS_X86 isn't going to affect platform other than Windows, among the ones I tested, which were Solaris 10 (x64 machine) both 32 bits and 64 bits, RHEL 3 (x64 kernel) both 32 bits and 64 bits, Windows Server 2003 64 bits - both 32 bits and 64 bits.

I didn't test Mac. I am probably the only non-Mac user left, so hopefully somebody else can say if _X86_ was defined on Mac before and if NSS_X86 is defined with the patch.

Comment 11

[Julien Pierre]

Some issues were overlooked during the review for bug 431958, namely that _X86_ affects NSPR header files too, not just NSS. I only searched NSS header files when I generated the patch.

The Win64 runtime is currently hosed as a result, so I'm marking this P1 blocker for 3.12.3 . See more details in bug 485664.

Moving away from using _X86_ in freebl will resolve those problems.

Comment 12

[Julien Pierre]

The trunk is crashing in RC4 on Win64 on my quad core AMD Phenom at home. This was one of the algorithms that I tested on my Opteron at the office when I checked in the fix for bug 431958. That doesn't make much sense, unless the code works on some CPUs and not others. It's crashing in the assembly code, probably due to the NSPR mismatched macros.

Comment 13

[Wan-Teh Chang]

Julien, you can apply my NSPR patch in bug 485664 and see if
that fixes the crash in RC4.  I'd need to inspect a lot of code
to determine if IS_64 being undefined will actually break any
lib/freebl code, so it's easier to determine that by testing.

IS_64 is tested by lib/freebl/arcfour.c in two places, but the
two ifdef tests are different:

#if (defined(IS_64))
typedef PRUint64 WORD;
#else
typedef PRUint32 WORD;
#endif
#define WORDSIZE sizeof(WORD)

...

#if (defined(IS_64) && !defined(__sparc)) || 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
#define ARCFOUR_NEXT_WORD() \
        { streamWord = 0; ARCFOUR_NEXT4BYTES_B(32); ARCFOUR_NEXT4BYTES_B(0); }
#endif
#else
/* 32-bit wordsize */
#ifdef IS_LITTLE_ENDIAN
#define ARCFOUR_NEXT_WORD() \
        { streamWord = 0; ARCFOUR_NEXT4BYTES_L(0); }
#else
#define ARCFOUR_NEXT_WORD() \
        { streamWord = 0; ARCFOUR_NEXT4BYTES_B(0); }
#endif
#endif

So, the fact that these two ifdef tests are different could be
a problem of its own.

Comment 14

[Julien Pierre]

Wan-Teh,

Re: comment 13,

I tested your NSPR patch in bug 485664, and it does fix the crash in RC4 on Win64.
However, since this crash was a regression introduced in an NSS patch, I think it needs to be fixed in NSS. The fix shouldn't depend on a particular version of NSPR. It's possible others will be building NSS 3.12.3 against NSPR 4.7.3 or earlier.
I'm not saying the NSPR patch shouldn't also go in, just that we have to do something to fix freebl first. I like your solution in this bug to move away from using _X86_ in freebl.

The #ifdef discrepancy you found is a good catch. It happened as a result of bug 227049 . Nelson initially took over the changes to arcfour a year ago. When I work on them to complete it last week, I only worked from Nelson's patch, but I didn't look at much of the rest of the source file. That's how this other #ifdef was missed. It should probably have been changed too, though it isn't exactly the same ifdef as the one in attachment 368174 [details] [diff] [review] . I don't believe that patch caused any regression .

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369676 [details] [diff] [review]
Proposed patch

I am not comfortable giving this patch an r+ on the eve of tagging 
NSS 3.12.3 RTM, but I cannot say I know of a definite bug that deserves r-.  

This patch clearly changes many occurrences of _X86_ to NSS_X86.  e.g.

>-#if defined (_X86_)
>+#if defined (NSS_X86)

But it also 
a) deletes a definition of _X86_ without replacing it in at least one place:

> 	# Solaris x86
>-	DEFINES += -D_X86_
> 	DEFINES += -DMP_USE_UINT_DIGIT

b) Makes changes to other symbols, whose correlation to _X86_ is 
not clear (to me).  I cannot determine, by review alone, that these 
substitutions are correct.  Examples:

>-#if (defined(_X86_) && !defined(__x86_64__)) || defined(SHA_NO_LONG_LONG)
>+#if defined(__i386__) || defined(__i386) || defined(_M_IX86) || \
>+    defined(SHA_NO_LONG_LONG)


>-#if defined(_X86_) || defined(__x86_64__) || defined(__x86_64) 
>+#if defined(NSS_X86)
> static __inline__ PRUint32 swap4b(PRUint32 value)

>-#if defined(i386) || defined(__i386) || defined(__X86__) || defined (_M_IX86) || defined(__x86_64__) || defined(__x86_64) || defined(_M_AMD64)
>+#if defined(__i386__) || defined(__i386) || defined (_M_IX86) || defined(__x86_64__) || defined(__x86_64) || defined(_M_AMD64)
> /* X86 processors have special instructions that tell us about the cache */

I think we need very thorough testing to be confident that this change
is correct and has no other undesired side effects.  I am skeptical about
our ability to do that testing under the high pressure of the moment.
After all, we thought that such testing had been done before the change 
for bug 431958 was committed, when we were not under pressure quite as 
high, and that testing was mistaken.  

We now have two options: 
a) back out the checkin for bug 431958, or 
b) try this patch, and then try to do better testing than was done before.

We don't have days to fix this.  
We need a very well defined process by which to decide that 
a) we've achieved victory, or 
b) we need to roll back.  
The time for unbridled enthusiasm for going forward with more changes 
with potentially sweeping consequences is over.  It's RTM time.

Comment 16

[Julien Pierre]

Nelson,

Re: comment 15,

a) This removal is OK. On Solaris x86, CPU_ARCH is set to the value "x86" .
So, the following code in the patch will take care of setting NSS_X86 :

+# Define NSS_X86 for x86 CPUs, both 32 and 64 bits (x86-64).
 ifeq (,$(filter-out x386 x86 x86_64,$(CPU_ARCH)))
-	DEFINES += -D_X86_
+	DEFINES += -DNSS_X86
 endif

Was there any other removal that worried you in the patch ?

b) let's take a look at this test :

-#if (defined(_X86_) && !defined(__x86_64__)) || defined(SHA_NO_LONG_LONG)
+#if defined(__i386__) || defined(__i386) || defined(_M_IX86) || \
+    defined(SHA_NO_LONG_LONG)

An english translation of the old test meant "if (32-bit X86 platform) or SHA_NO_LONG_LONG is set".

The presence of !defined(__x86_64__) in the original test was necessary for Linux x64 only - the only x64 platform where _X86_ was set. Windows x64 and Solaris x64 did not have it set.

With the new patch, NSS_X86 is now set on both 32-bit and 64-bit Intel CPUs. So, it would be set for 64-bit CPUs and was not suitable for this test. Wan-Teh decided to do that by testing 3 compiler-specific macros. Here is another way which may be closer to the original and doesn't depend on compiler macros :

-#if (defined(_X86_) && !defined(__x86_64__)) || defined(SHA_NO_LONG_LONG)
+#if (defined(NSS_X86) && !defined(NSS_USE_64)) || defined(SHA_NO_LONG_LONG)

This will do what we need to on all 3 x64 platforms, in a compiler-independent way, and may be easier to review.

Another thing I may propose to improve Wan-Teh's patch is that we name the new symbol NSS_X86_X64, since it is really set on both types of CPU architectures, not just 32-bit x86 . That may be a mouthful, but it might also make code review and browsing easier. That is purely a cosmetic suggestion, though. I still give r+ to the original patch.

I am still looking at your other issues.

Comment 17

[Julien Pierre]

Re, comment 15,

You can verify by code inspection that the following tests in sha_fast.h are OK :

>-#if defined(_X86_) || defined(__x86_64__) || defined(__x86_64) 
>+#if defined(NSS_X86)
> static __inline__ PRUint32 swap4b(PRUint32 value)

You know from the freebl Makefile already that NSS_X86 is set for all 32-bit and 64-bit Intel CPUs, so this test should be very obvious. It would be even more obvious if we rename NSS_X86 to NSS_X86_X64 .

Lastly, the change in mpcpucache.c is a no-op . The __X86__ symbol (with double-underscores) was never set. We can drop this from the patch.

I will generate an alternate patch that is easier to review.

Comment 18

[Julien Pierre]

Attachment: Alternate patch

This patch does 3 things compared to Wan-Teh's patch :
1) rename NSS_X86 to NSS_X86_X64
2) change the test in sha512.c to not use compiler built-in macros to detect 32-bit CPUs
3) drop the change in mpcpucache.c, which is not necessary

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

> You know from the freebl Makefile already that NSS_X86 is set for all 
> 32-bit and 64-bit Intel CPUs, 

No, actually, I don't know that.  I would like to be sure it is true, but
I am not. 

There are only two CPU architectures here, 32-bit x86 and 64-bit x86, but 
we have many more than two symbols that represent them.  That's crazy.
Then, in many cases, we're testing NSS/NSPR's "CPU_ARCH" instead of testing
the compiler-defined symbols.  It's a MESS.  

We need ONE symbol that means 32-bit X86 on all compilers and all OSes.
We need ONE symbol that means 64-bit X96 on all compilers and all OSes.
It would be nice to have ONE symbol that means 32-bit or 64-bit X86 on all
compilers and all OSes.  

I'm really not interested in increasing the number of symbols I have to 
keep in my head to include i386, __i386, __i386__, _M_IX86, __x86_64__, 
__x86_64 and any others that add NO VALUE WHATSOEVER to NSS.

Comment 20

[Julien Pierre]

Nelson,

Actually, you know that because you reviewed the patch in bug 431958 last week.
I tested all x86 and x64 platforms.

The only thing I failed to catch was that _X86_ conflicted with NPSR, which relied on it. That's the only thing needs to be fixed now to resolve the RC4 crash on Win64. If you can verify by code review that _X86_ is simply renamed to NSS_X86, that eliminates the issue with NSPr, and resolves the RC4 crash on Win64.

Regarding your 3 requests

1) we have the following symbol defined in NSS called NSS_USE_64 . I believe it is set for all 64-bit platforms (except DEC, which we don't care about). 
2) Conversely, when this symbol is not set, we are always 32 bits, since we dropped WIN16.
3) we already have one symbol that means 32-bit x86 or 64-bit x64 . It's defined in freebl and it's called _X86_ . The only thing we are doing here is renaming it to avoid an NSPR conflict on Win64.

Comment 21

[Wan-Teh Chang]

Comment on attachment 370335 [details] [diff] [review]
Alternate patch

>+# Define NSS_X86_X64 for x86 CPUs, both 32 and 64 bits (x86-64).

Nit: you may want to update this comment since you renamed the macro.
How about "Define NSS_X86_X64 for x86 and x64 CPUs." ?

I can't spend too much time on NSS this week, so I'm reassigning
this bug to you.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Julien, the presence or absence of NSS_USE_64 says nothing about x86.
Please don't use the testing done last week as a supporting argument for 
any changes, since it was a failure.

Comment 23

[Julien Pierre]

Nelson,

1) #if defined(NSS_X86_X64) && (!defined(NSS_USE_64)) tells you that you are on a 32-bit x86 architecture

2) #if defined(NSS_X86_X64) && (!defined(NSS_USE_64)) tells you that you are on a 64-bit x64 architecture

Regarding the failure last week, only a single platform is broken, Win64, and both patches attached to this bug fix it.

Comment 24

[Wan-Teh Chang]

Comment on attachment 370335 [details] [diff] [review]
Alternate patch

>-#if (defined(_X86_) && !defined(__x86_64__)) || defined(SHA_NO_LONG_LONG)
>+#if (defined(NSS_X86_X64) && !defined(NSS_USE_64)) || defined(SHA_NO_LONG_LONG)

Julien, let's change
    !defined(NSS_USE_64)
back to
    !defined(__x86_64__)

so that this minor issue won't block the more important
_X86_ renaming changes.

Comment 25

[Julien Pierre]

Wan-Teh,

You are right. This change is not 100% equivalent to what was there before . What was there before may not have been ideal, but it's not the time to fix that part.

I think doing a simple search/replace of the _X86_ symbol in the freebl directory will fix the Win64 regression, to the exclusion of any other code change. I am in the process of testing that.

Comment 26

[Julien Pierre]

Attachment: Alternative patch 2, even simpler to review

Nelson,

Please consider the following facts that should help you with your review :
1) the _X86_ symbol is never used outside the freebl directory in NSS
2) the _X86_ symbol is only used by some NSPR header files and sources that apply to Windows only, not for any other platform
3) the _X86_ symbol is only used by OS header files on Windows, not for any other platform
4) in light of items 3 through 5, renaming the symbol _X86_ in freebl to a private symbol cannot have any effect on any platform other than Windows, and therefore if all the patch does is rename that symbol in freebl, only Windows platforms need to be retested with the patch
5) freebl on the Win64 platform is known to be broken, because the _X86_ symbol is defined in freebl and conflicts with NSPR
6) freebl on the Win32 platform, and all other platforms, is known to be working
7) the only thing this new patch does is rename that symbol and change a comment
8) I have tested this patch on Win32 with cipher.sh, both debug and optimized, and can confirm that it doesn't break it . I did a full clobber build with the current trunk. all.sh is also running against both and will take a few hours to complete.
9) I have tested this patch on Win64 with cipher.sh, both debug and optimized, and can confirm that it fixes the regression in RC4 . I did a full clobber build with the current trunk. all.sh is also running against both and will take a few hours to complete.

Comment 27

[Julien Pierre]

In comment 26, the number 4 item should have read "in light of items 1 through 3", of course.

Comment 28

[Julien Pierre]

All my tests completed on both Win32 and Win64 for both debug and optimized. The only failures that I observed were some that I had also ran into before - those I reported in bug 485040, and some PR_Connect and PR_Poll I/O timeout errors in SSL tests, mostly in the stress tests, which have always happened on my Vista box and are completely unrelated, which I haven't had time to investigate, but aren't regressions.

Comment 29

[Wan-Teh Chang]

Comment on attachment 370356 [details] [diff] [review]
Alternative patch 2, even simpler to review

r=wtc.  You forgot to remove the following code from win_rand.c:

37	#if defined(_WIN32) && defined(NSS_USE_64)
38	#undef _X86_
39	#endif

That code was added as part of your patch for bug 431958 comment 44,
so it should be removed when you rename _X86_.

Comment 30

[Julien Pierre]

Wan-Teh,

That code should be removed indeed, but its presence doesn't hurt anything. I was trying to have a patch as simple as possible.

Comment 31

[Wan-Teh Chang]

Comment on attachment 370356 [details] [diff] [review]
Alternative patch 2, even simpler to review

That code was added only because the macro's
name was _X86_.  If the macro is renamed, that
code should be removed.  That code was only
added a week ago.  Do not leave that code
dangling there.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Of the patches presently attached to this bug, I like the first one best, 
but ideally I would like to see a patch that does all the following:

1. Define one symbol in ALL c compilations for code that uses the instruction 
set of x86-family CPUs (either 32-bit or 64-bit), regardless of the OS, or 
brand of compiler or assembler.  This was my original intent behind _x86_.
NSS_X86 will be fine for this purpose. 

2. Define one symbol in ALL c compilations for code that should use 64-bit 
registers in a (some, any) 64-bit ABI for ANY CPU that has 64-bit registers
and a 64-bit ABI.  Ensure that this symbol is defined in builds for CPUs, compilers and/or OSes that do not support 32-bit ABIs.  Note that NSS_USE_64 
presently has the property that it is defined in C compilations if and only 
if it was defined in the build environment.  The symbol that I envision for 
this requirement will not require that any symbol be defined in the build 
environment for systems that do not support 32-bit ABIs. That symbol could 
be NSS_USE_64, IFF we're willing to redefine NSS_USE_64 to meet this new requirement.  For now, let me call this "NSS_64_BIT_ISA".

3. (optionally) define two new symbols, named (say) NSS_X86_32 and NSS_X86_64
which are never both defined in the same build.  One means effectively:
   defined(NSS_X86) && !defined(NSS_64_BIT_ISA)
and the other means, effectively:
   defined(NSS_X86) && defined(NSS_64_BIT_ISA)

4. Eliminates all references in all .c .h .s .S and .asm files (in freebl, 
at least) to any and all other symbols for x86 family CPUs, including 
(but not limited to):
    __i386
    __i386__
    _M_IX86
    __x86_64
    __x86_64__
    _M_AMD64
substituting tests for the symbols I described above instead.

The problem with that plethora of compiler-defined symbols (named in item 4 
above) is that they are NOT common to all the build systems (compilers)
that build for a common underlying Instruction Set Architecture (ISA).
The main point is to stop using those uncommon symbols, and start using a 
SMALL set of portable symbols that we define, which are common to all 
platforms of a given ISA.  That was the original motivation for the present
use of _X86_.  Let's not lose sight of that goal.

I don't like the symbol name NSS_X86_X64  because its name suggests to me
that it is ONLY for 64-bit, even though (I suspect) it was intended to 
represent both 32-bit and 64-bit x86-family CPUs.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

When i wrote "ALL c compilations", I meant to qualify it with "in lib/freebl".

Comment 34

[Julien Pierre]

Nelson,

re: comment 32,

1. attachment 369423 [details] [diff] [review] in bug 431958 made _X86_ the symbol that is defined on all x86 and x64 platforms. There is no indication that it didn't succeed in doing that.

The only reason that patch was backed out is because defining _X86_ conflicted with NSPR header files on Win64. We should check in that patch again, before we apply one of the incremental patches attached in this bug to fix that name conflict.

I think NSS_X86 is confusing for a symbol that is supposed to be defined also on x64 platforms. That's why I chose another symbol, NSS_X86_X64, in attachment 370335 [details] [diff] [review] and attachment 370356 [details] [diff] [review] .

You said in item 4 that you don't like NSS_X86_X64 because that symbol suggests that it's only for 64-bit. How about renaming it to
NSS_X86_OR_X64 ? Would that be clear enough for everybody ?

2. Regarding USE_64 and its equivalent NSS_USE_64 : is there any way to build 64-bit today on any supported platform without defining USE_64 in the build environment ? I believe USE_64 is now requirement on all our supported platforms.

As far as I know, the only platform that is known to support only a 64-bit ABI, and that will build without defining USE_64 (though
I have no way of verifying this), is DEC OSF/1. I think we could fix the coreconf/OSF*.mk files to always define USE_64 . For any new 64-bit-only platforms in the future, they would have USE_64 automatically defined as well in coreconf.

This would clarify that USE_64 / NSS_USE_64 always mean a 64-bit target (regardless of instruction set).

Does that change sound reasonable ?

3. I think once 1 and 2 are solved, then we can easily create these 2 new symbols. I personally prefer NSS_X86 to mean 32-bits, and
NSS_X64 to mean 64-bits, because it's concise. But if it's not clear enough that NSS_X86 means 32-bit, then NSS_X86_32 is fine with
me.

4. That's a good goal, but I don't think that work is required to fix the immediate and only known problem we have seen today which
was on Win64, and which can be fixed by simply renaming the _X86_ symbol to something else.

Making these additional changes may require extra testing on all platforms to get it right, while the changes for 1/2/3 should be easily reviewable. I propose to do changes 1/2/3 first starting from Wan-Teh's patch, and 4 later as a separate patch, as I'm not sure if that part will be done in the next week. Let me know if you agree with that plan.

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Julien, 
Once we resolve the string themselves, I'm fine with your plan.

Where else is "x64" used to mean "x86_64"? 
I don't recall ever seeing it used for that meaning before.
It just doesn't convey that meaning to me.  

Maybe we should go back to XP_PC to mean x86-family-32-and-64.  1/2 :)

Comment 36

[Julien Pierre]

Nelson,

Sun uses x64 everywhere .

Eg: http://www.sun.com/x64/index.jsp

So does Microsoft ("Microsoft Windows XP Professional x64 Edition")
http://www.microsoft.com/windowsxp/64bit/default.mspx

Same with their Windows server.
http://www.microsoft.com/servers/64bit/overview.mspx

I like x64 because it's a lot shorter than "x86 64", both to write and to say, and it's unambiguous. It is also vendor-neutral, unlike AMD64, EM64T, and many other acronyms.

I think XP_PC is a little too vague to use in this case.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

ok, so NSS_X86_OR_X64 would be OK.

Comment 38

[Julien Pierre]

OK. What about 32 bits ? I think NSS_X86 is sufficient for that - NSS_X86_32 looks a bit redundant, but we do need the symbol to be clear to everyone. Obviously there will be comments about each symbol in freebl/Makefile if there is ever any doubt. I just think it's nice to have 2 opposite symbols of the same length - NSS_X86 and NSS_X64 . And NSS_X86_OR_X64 for the union - not NSS_X86_32_OR_X64 .

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

> NSS_X86 and NSS_X64 And NSS_X86_OR_X64 for the union

OK

Comment 40

[Julien Pierre]

Attachment: Implement items 1 and 3 from comment 34

This patch includes only the files in freebl, so that it can be compared against Wan-Teh's fix. The patch was made against the trunk, not against the same revisions that Wan-Teh did.

So, a couple of differences will show up due to the backout of attachment 369423 [details] [diff] [review]. But since that patch was very small, it should still be easy to review the patch.

I will attach the patch for coreconf OSF1 separately since it affects a different file.

Comment 41

[Julien Pierre]

Comment on attachment 371606 [details] [diff] [review]
Implement items 1 and 3 from comment 34

hmm, this doesn't work with interdiff because my patch didn't include win_rand.c as the change was no longer necessary. I will attach another patch that should work. sigh.

Comment 42

[Julien Pierre]

Attachment: patch that should work with interdiff

Just ignore the change in win_rand.c, which has already been committed.

Comment 43

[Julien Pierre]

Comment on attachment 371607 [details] [diff] [review]
patch that should work with interdiff

Interdiff is useless.

Comment 44

[Julien Pierre]

Comment on attachment 371606 [details] [diff] [review]
Implement items 1 and 3 from comment 34

Just review this trunk patch on its own merits, without the help of interdiff.

Comment 45

[Julien Pierre]

Attachment: coreconf patch for DEC alpha

Untested. But it should work fine. I made sure the bit tag was unchanged so this patch should only affect freebl code.

Comment 46

[Wan-Teh Chang]

Comment on attachment 371606 [details] [diff] [review]
Implement items 1 and 3 from comment 34

>+# Define NSS_X86_OR_X64 for x86 CPUs, both 32 and 64 bits (x86-64).

This comment comes from the following comment in my patch (attachment 369676 [details] [diff] [review]):

    Define NSS_X86 for x86 CPUs, both 32 and 64 bits (x86-64).

I wrote that comment to explain that we broadened "x86" to also cover
x86-64.  In your new proposal, "x86" has the usual meaning, so you
shouldn't simply replace NSS_X86 by NSS_X86_OR_X64 without updating
the rest of the comment.

I suggest that you simply say:

    NSS_X86 means the target is a 32-bit x86 CPU architecture
    NSS_X64 means the target is a 64-bit x64 CPU architecture
    NSS_X86_OR_X64 means the target is either x86 or x64

In some files (for example, desblapi.c, md5.c, and mpi/mpcpucache.c),
existing comments mention "x86", which should now be changed to "x86 or x64".
You may also want to remove "Intel" from those comments.  Again, I didn't
change those comments in my original patch because that patch assumed that
the term "x86" was broadened to also cover x86-64.

In sha512.c:

>-#if (defined(_X86_) && !defined(__x86_64__)) || defined(SHA_NO_LONG_LONG)
>+#if defined(__i386__) || defined(__i386) || defined(_M_IX86) || \
>+    defined(SHA_NO_LONG_LONG)

This should now be

    #if defined(NSS_X86) || defined(SHA_NO_LONG_LONG)

In mpi/mpcpucache.c:

>-#if defined(i386) || defined(__i386) || defined(__X86__) || defined (_M_IX86) || defined(__x86_64__) || defined(__x86_64) || defined(_M_AMD64)
>+#if defined(i386) || defined(__i386) || defined (_M_IX86) || defined(__x86_64__) || defined(__x86_64) || defined(_M_AMD64)

I suggest that you change
    defined(i386)
to
    defined(__i386__)
because predefined macros that don't start with an underscore are deprecated.
If you are not sure, feel free to ignore this suggestion.

Note that I didn't change this test to test our own macro because the files
in the mpi directory could in theory be built stand-alone with their own
makefiles.  I didn't want to spend the time modifying the stand-alone mpi
makefiles, so I just let mpi/mpcpucache.c continue to test predefined macros.

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 371608 [details] [diff] [review]
coreconf patch for DEC alpha

sure, why not, for DEC. :)

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 371606 [details] [diff] [review]
Implement items 1 and 3 from comment 34

I think we have an agreed plan now of how to resolve this.  Since this patch doesn't embody that plan, I'm giving this one r-.
I'm expecting to receive a new patch review request very soon.

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369676 [details] [diff] [review]
Proposed patch

SR- for this patch.  Please see comment 48.

Comment 50

[Julien Pierre]

Nelson,

Re: comment 48, can you explain in what way attachment 371606 [details] [diff] [review] didn't embody the plan ?

As far as I can tell, comment 46 from Wan-Teh is mostly about issues that fall under your item 4 from comment 32, which I said in comment 34 would be implemented later in a separate patch. I thought you agreed to that plan.

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 371606 [details] [diff] [review]
Implement items 1 and 3 from comment 34

This is a mixed/conditional review, partly r+ and partly r-.

This patch changes 9 source files.
I'm giving r- to the change to one of them,
and unconditional r+ to the changes to 7 of them,
and r+ to one after a change is made.

r- for the change to mpcpucache.c.  This change solves no problem,
and is pointless.  

unconditional r+ for the patches to all the other files except sha512.c, 
where we see

>-#if (defined(_X86_) && !defined(__x86_64__)) || defined(SHA_NO_LONG_LONG)
>+#if defined(__i386__) || defined(__i386) || defined(_M_IX86) || \
>+    defined(SHA_NO_LONG_LONG)

Make that be:

>+#if defined(NSS_X86) || defined(SHA_NO_LONG_LONG)

then with that change, r+ for the patch to that file, too.

Comment 52

[Wan-Teh Chang]

Comment on attachment 371606 [details] [diff] [review]
Implement items 1 and 3 from comment 34

The change to mpi/mpcpucache.c is not pointless.  It removes a typo
(__X86__).  The corret macro _X86_ is subsumed by the predefined
macro _M_IX86.

Comment 53

[Julien Pierre]

Attachment: Patch as checked in

I verified that cipher.sh runs properly with this patch on solaris x86, x64, linux x86, x64, and windows x86 and x64. I didn't run any benchmark.

I didn't check in the change to mpcpucache.c, but I think it would be good to get rid of the __X86__ typo, and that change really belongs as part of this bug, unlike the earlier request to get rid of the compiler macros, which can be treated as a separate problem and which probably should be a bug of its own.

Comment 54

[Julien Pierre]

cvs commit log :

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.105; previous revision: 1.104
done
Checking in alg2268.c;
/cvsroot/mozilla/security/nss/lib/freebl/alg2268.c,v  <--  alg2268.c
new revision: 1.9; previous revision: 1.8
done
Checking in des.c;
/cvsroot/mozilla/security/nss/lib/freebl/des.c,v  <--  des.c
new revision: 1.7; previous revision: 1.6
done
Checking in desblapi.c;
/cvsroot/mozilla/security/nss/lib/freebl/desblapi.c,v  <--  desblapi.c
new revision: 1.8; previous revision: 1.7
done
Checking in md5.c;
/cvsroot/mozilla/security/nss/lib/freebl/md5.c,v  <--  md5.c
new revision: 1.14; previous revision: 1.13
done
Checking in rijndael.c;
/cvsroot/mozilla/security/nss/lib/freebl/rijndael.c,v  <--  rijndael.c
new revision: 1.25; previous revision: 1.24
done
Checking in sha512.c;
/cvsroot/mozilla/security/nss/lib/freebl/sha512.c,v  <--  sha512.c
new revision: 1.14; previous revision: 1.13
done
Checking in sha_fast.h;
/cvsroot/mozilla/security/nss/lib/freebl/sha_fast.h,v  <--  sha_fast.h
new revision: 1.8; previous revision: 1.7

Comment 55

[Julien Pierre]

Also :

Checking in OSF1.mk;
/cvsroot/mozilla/security/coreconf/OSF1.mk,v  <--  OSF1.mk
new revision: 1.9; previous revision: 1.8
done

Comment 56

[Julien Pierre]

I am closing this as fixed. I opened bug 492898 to track part 4 of Nelson's requests in comment 32, which I chose to defer.