1256665 - PR_AtomicSet: NSPR should know about presence of __sync_lock_test_and_set with gcc 4.1.2 32-bit.

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[Kai Engert [:KaiE:]]

Bug 1181814 introduced code in libfreebl3.so which makes use of PR_ATOMIC_SET, but without linking to NSPR.

The link step fails, with undefined reference to `PR_AtomicSet'

pratom.h in nspr attempts to decide if __sync_lock_test_and_set is available and can be used to implement PR_ATOMIC_SET.

Because on gcc 4.1.2 the symbol __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 isn't defined, the NSPR code decides that __sync_lock_test_and_set is absent.

However, the gcc manual lists the macro as available:
https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html


I suggest that we change NSPR to make use of the __sync_lock_test_and_set macro, if the compiler version is at least gcc 4.1.2


For reference purposes, the check for the macros was introduced in
https://hg.mozilla.org/projects/nspr/rev/ef8fe5bd09d2 as part of bug 415563.


This is a blocker bug.
We are temporarily accepting the build bustage on the RHEL 5.x platform.

We should get this issue addressed asap, to avoid having to back out 1181814.

Comment 1

[Kai Engert [:KaiE:]]

In bug 415563, Wan-Teh had decided to use the following to test availability:

  (defined(__i386__) && defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4))

We know that i386 doesn't support the code. However, i486 and later do support it.
Wan-Teh couldn't decide whether to test for i486 or i686.

I propose we enhance the test, so the builtin atomic operations will be used, if we're using a hardware architecture that's newer than i386:

    ((defined(__i386__) && defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) ||
    defined(__i486__) || defined(__i586__) || defined(__i686__)

Comment 2

[Kai Engert [:KaiE:]]

Attachment: 1256665-v1.patch

Comment 3

[Wan-Teh Chang]

Comment on attachment 8730745 [details] [diff] [review]
1256665-v1.patch

Review of attachment 8730745 [details] [diff] [review]:
-----------------------------------------------------------------

Kai: thanks for the patch. I think it is OK to drop support
for 80386 processors, which will result in simpler code.

::: pr/include/pratom.h
@@ +102,5 @@
>             (defined(__ppc__) || defined(__i386__) || defined(__x86_64__))) || \
>         (defined(__linux__) && \
>             ((defined(__i386__) && \
>             defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4)) || \
> +           defined(__i486__) || defined(__i586__) || defined(__i686__) || \

Kai: it is better to just drop support for Intel 80386 processors.
You can revert the second change of this checkin:

https://hg.mozilla.org/projects/nspr/rev/ef8fe5bd09d2

I suggest we list those three processors in this order:
__i386__ __x86_64__ __ia64__

Also, I think you should add a PR_AtomicSet() stub to lib/freebl/stubs.*
in NSS to support the default implementation of PR_ATOMIC_SET.

Note: based on these links, it seems that __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
was added in gcc 4.3, which matches my commit message in
https://hg.mozilla.org/projects/nspr/rev/ef8fe5bd09d2 that this macro is
"supported by GCC 4.3 or later":

https://gcc.gnu.org/onlinedocs/gcc-4.1.2/cpp/Common-Predefined-Macros.html
https://gcc.gnu.org/onlinedocs/gcc-4.2.4/cpp/Common-Predefined-Macros.html
https://gcc.gnu.org/onlinedocs/gcc-4.3.6/cpp/Common-Predefined-Macros.html

What is the gcc version on the RHEL 5.x platform? Why does it not support
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4?

Comment 4

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #3)
> What is the gcc version on the RHEL 5.x platform? Why does it not support
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4?

4.1.2, which is older then 4.3

Comment 5

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #3)
> 
> Also, I think you should add a PR_AtomicSet() stub to lib/freebl/stubs.*
> in NSS to support the default implementation of PR_ATOMIC_SET.

I don't understand the purpose of your suggestion. Could you please explain?

It seems the stubs.* functionality is primarily used to enable compilation to work, but most of the stub functions are hardcoded to abort() if called.

Comment 6

[Wan-Teh Chang]

Kai, I looked at the original problem in NSS's lib/freebl directory.

PR_ATOMIC_SET is only used in one file: lib/freebl/lowhash_vector.c.
That file is currently only used on Linux. So you can also just use
__sync_lock_test_and_set directly. That may be the best solution.

The GCC documentation says:

==========
type __sync_lock_test_and_set (type *ptr, type value, ...)

This builtin, as described by Intel, is not a traditional test-and-set operation, but rather an atomic exchange operation. It writes value into *ptr, and returns the previous contents of *ptr.

Many targets have only minimal support for such locks, and do not support a full exchange operation. In this case, a target may support reduced functionality here by which the only valid value to store is the immediate constant 1. The exact value actually stored in *ptr is implementation defined.
==========

Note the warning about reduced functionality in the second paragraph.
That is why I added the test for __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4:

114 /*
115  * Because the GCC manual warns that some processors may support
116  * reduced functionality of __sync_lock_test_and_set, we test for the
117  * processors that we believe support a full atomic exchange operation.
118  */

lib/freebl/lowhash_vector.c calls PR_ATOMIC_SET with the second argument=1.
That works with the reduced functionality version of __sync_lock_test_and_set.

Also, note the sleep(1) call in lib/freebl/lowhash_vector.c sleeps for one
second. Whoever wrote that code may not realize that. There are better
options, such as sched_yield(), nanosleep(), and usleep().

Comment 7

[Kai Engert [:KaiE:]]

Attachment: 1256665-v2.patch

For the record, I believe this implements Wan-Teh's earlier suggestion.
It doesn't work, pkcs11.c and sftkdb.c fail to link with 
  undefined reference to `__sync_add_and_fetch_4'

I don't yet understand why.

Comment 8

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #7)
> Created attachment 8730889 [details] [diff] [review]
> 1256665-v2.patch
> 
> For the record, I believe this implements Wan-Teh's earlier suggestion.
> It doesn't work, pkcs11.c and sftkdb.c fail to link with 
>   undefined reference to `__sync_add_and_fetch_4'
> 
> I don't yet understand why.

Now that I wrote it... It's probably because I only used -march=i486 for the lib/freebl directory, but not for lib/softokn.

If we use the approach that Wan-Teh suggested earlier, then we must build all of NSS with -march=i486.

With the patch that I suggested initially, building only lib/freebl with -march=i486 is sufficient.

Comment 9

[Wan-Teh Chang]

Comment on attachment 8730745 [details] [diff] [review]
1256665-v1.patch

Review of attachment 8730745 [details] [diff] [review]:
-----------------------------------------------------------------

I meant to mark this patch feedback-, not feedback+.

Comment 10

[Kai Engert [:KaiE:]]

I believe it is necessary to build with -march=i486, because only then the compiler will make the atomic functions available, is that correct?

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 8730889 [details] [diff] [review]
1256665-v2.patch

When using this patch, and compiling all C files of NSS with -march=i486, then I get past the failure of undefined references in the lib/softokn files.

However, I still get them at a different step: 
When linking bltest, I get undef refs to __sync_sub_and_fetch_4 and __sync_add_and_fetch_4.

So, Wan-Teh earlier suggestion would apparently have larger consequences, while my initial patch worked in a simpler way.


I'll try to look at Wan-Teh's second suggestion next.

Comment 12

[Kai Engert [:KaiE:]]

> So, Wan-Teh earlier suggestion would apparently have larger consequences,
> while my initial patch worked in a simpler way.

Which, I realize, might be to compile NSPR itself with -march=i486, too.

Comment 13

[Kai Engert [:KaiE:]]

Wan-Teh, since you have suggested to fix the problem at the NSS level, I believe you're suggesting that this bug is WONTFIX.