[Bug nptl/16882] New: Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[Bug nptl/16882] New: Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[a5b at mail dot ru]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

            Bug ID: 16882
           Summary: Incorrect implementation of pthread_spin_lock for
                    sparc, no ldstub after spinning
           Product: glibc
           Version: 2.16
            Status: NEW
          Severity: normal
          Priority: P2
         Component: nptl
          Assignee: unassigned at sourceware dot org
          Reporter: a5b at mail dot ru
                CC: drepper.fsp at gmail dot com

I think there is sparc-specific bug introduced by commit 
'Avoid "anonymous" code in pthread_spin_lock' from 3 May 2012
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=e2dbf201abdfa13fc4035a1a8888ecec91bef44c

The bug exists since 2.16 version of glibc.

New version of pthread_spin_lock for sparc64 and sparcv8+ doesn't do "ldstub"
to lock the spinlock, if there were any spins on spinlock, locked by another
thread.

Original implementation of pthread_spin_lock: 

 Try to lock spinlock and return
 if spinlock was locked, go to 2: and spin, waiting spinlock to unlock
 then go to 1: to try lock it


    ("1: ldstub [%0], %%g2\n"
     "   orcc   %%g2, 0x0, %%g0\n"
     "   bne,a  2f\n"
     "   ldub   [%0], %%g2\n"
     ".subsection 2\n"
     "2: orcc   %%g2, 0x0, %%g0\n"
     "   bne,a  2b\n"
     "   ldub   [%0], %%g2\n"
     "   b,a    1b\n"
     ".previous"
     : /* no outputs */
     : "r" (lock)
     : "g2", "memory", "cc");

Code after patch:

 Try to lock spinlock and return
 if spinlock was locked, go to 2: and spin, waiting spinlock to unlock
 then return  

ENTRY(pthread_spin_lock)
       ldstub          [%o0], %g1
       brnz,pn         %g1, 2f
        membar         #StoreLoad | #StoreStore
1:     retl
        mov            0, %o0
2:     ldub            [%o0], %g1
       brnz,pt         %g1, 2b
        membar         #LoadLoad
       ba,a,pt         %xcc, 1b
END(pthread_spin_lock)


With newer code, pthread_spin_lock may return without actually locking the
spinlock. This is incorrect according to The Open Group Base Specifications
Issue 6
http://pubs.opengroup.org/onlinepubs/009695299/functions/pthread_spin_lock.html

> The pthread_spin_lock() function shall lock the spin lock referenced by lock.

I have short test:

$ cat pthread_spin_test.c
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

void* thread(void *arg)
{
    pthread_spinlock_t *lock = (pthread_spinlock_t*) arg;
    printf("Thread started, lock locked by main, value is %08x\n",
*(int*)lock);
    pthread_spin_lock(lock);
    printf("Lock now locked by thread, value is %08x\n", *(int*)lock);
    pthread_spin_unlock(lock);
    printf("Lock now unlocked by thread, value is %08x\n", *(int*)lock);
    return NULL;
}

int main()
{
    pthread_t thr1;
    pthread_spinlock_t lock;
    pthread_spin_init(&lock, PTHREAD_PROCESS_PRIVATE);
    printf("Lock created, value is %08x after init\n", *(int*)&lock);
    pthread_spin_lock(&lock);
    printf("Lock locked by main, value is %08x\n", *(int*)&lock);
    pthread_create(&thr1, NULL, thread, (void*)&lock);
    sleep(1);
    pthread_spin_unlock(&lock);
    pthread_join(thr1, NULL);
    return 0;
}

With glibc-2.7 it always outputs 0xFF as value of locked spinlock:

Lock created, value is 00000000 after init
Lock locked by main, value is ff000000
Thread started, lock locked by main, value is ff000000
Lock now locked by thread, value is ff000000
Lock now unlocked by thread, value is 00000000


And with glibc-2.16, spinlock locked by thread has incorrect value 0
(unlocked):

Lock created, value is 00000000 after init
Lock locked by main, value is ff000000
Thread started, lock locked by main, value is ff000000
Lock now locked by thread, value is 00000000       <<<<
Lock now unlocked by thread, value is 00000000


To fix the bug, I think it can be enough to move "1:" label before ldstub in
files
nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S
nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[culu.gyx at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

GUO Yixuan <culu.gyx at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |culu.gyx at gmail dot com

--- Comment #1 from GUO Yixuan <culu.gyx at gmail dot com> ---
Created attachment 7613
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7613&action=edit
0001-Fixed-pthread_spin_lock-on-sparc32-64-bug-16882.patch

This is a patch (based on a5b's description)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[culu.gyx at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #2 from GUO Yixuan <culu.gyx at gmail dot com> ---
I think a5b's idea is the correct fix. Patches and a new test are attached.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[culu.gyx at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #3 from GUO Yixuan <culu.gyx at gmail dot com> ---
Created attachment 7614
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7614&action=edit
0002-New-test-for-pthread_spin_lock-bug-16882.patch

A testcase (independent of particular implementation)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[culu.gyx at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #4 from GUO Yixuan <culu.gyx at gmail dot com> ---
I've tested it on sparc32 and sparc64, on the master branch. And this patch
should also work on release/2.x/master for 2.16 to 2.19.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[Ondřej Bílka]

On Fri, May 23, 2014 at 02:26:35PM +0000, culu.gyx at gmail dot com wrote:
> https://sourceware.org/bugzilla/show_bug.cgi?id=16882
> 
> --- Comment #4 from GUO Yixuan <culu.gyx at gmail dot com> ---
> I've tested it on sparc32 and sparc64, on the master branch. And this patch
> should also work on release/2.x/master for 2.16 to 2.19.
> 
Could you send this patch to libc-alpha@sourceware.org so it could be
reviewed?

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[neleai at seznam dot cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #5 from Ondrej Bilka <neleai at seznam dot cz> ---
On Fri, May 23, 2014 at 02:26:35PM +0000, culu.gyx at gmail dot com wrote:
> https://sourceware.org/bugzilla/show_bug.cgi?id=16882
> 
> --- Comment #4 from GUO Yixuan <culu.gyx at gmail dot com> ---
> I've tested it on sparc32 and sparc64, on the master branch. And this patch
> should also work on release/2.x/master for 2.16 to 2.19.
> 
Could you send this patch to libc-alpha@sourceware.org so it could be
reviewed?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[culu.gyx at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #6 from GUO Yixuan <culu.gyx at gmail dot com> ---
(In reply to Ondrej Bilka from comment #5)
> On Fri, May 23, 2014 at 02:26:35PM +0000, culu.gyx at gmail dot com wrote:
> > https://sourceware.org/bugzilla/show_bug.cgi?id=16882
> > 
> > --- Comment #4 from GUO Yixuan <culu.gyx at gmail dot com> ---
> > I've tested it on sparc32 and sparc64, on the master branch. And this patch
> > should also work on release/2.x/master for 2.16 to 2.19.
> > 
> Could you send this patch to libc-alpha@sourceware.org so it could be
> reviewed?

All right, I've just sent the patches.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[culu.gyx at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

GUO Yixuan <culu.gyx at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |sparc-linux-gnu
               Host|                            |sparc-linux-gnu
              Build|                            |sparc-linux-gnu

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[davem at davemloft dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

David S. Miller <davem at davemloft dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |davem at davemloft dot net
           Assignee|unassigned at sourceware dot org   |davem at davemloft dot net

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #7 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  196939fb0ede8b588019f7ff0779dffbfc35787d (commit)
       via  b42eca7d39d7d263815a4457c318eb57ee2686a4 (commit)
      from  f9d2d03254a58d92635a311a42253eeed5a40a47 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=196939fb0ede8b588019f7ff0779dffbfc35787d

commit 196939fb0ede8b588019f7ff0779dffbfc35787d
Author: Guo Yixuan <culu.gyx@gmail.com>
Date:   Thu May 29 21:53:16 2014 -0700

    New test for pthread_spin_lock (bug 16882)

        * nptl/tst-spin4.c: New test.
        * nptl/Makefile (tests): Add tst-spin4.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=b42eca7d39d7d263815a4457c318eb57ee2686a4

commit b42eca7d39d7d263815a4457c318eb57ee2686a4
Author: Guo Yixuan <culu.gyx@gmail.com>
Date:   Thu May 29 21:43:15 2014 -0700

    Fixed pthread_spin_lock on sparc32/64 (bug 16882)

        [BZ #16882]
        * nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S
        (pthread_spin_lock): Branch out of spin loop to proper location.
        * nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S
        (pthread_spin_lock): Likewise.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                      |   11 +++
 nptl/Makefile                                  |    2 +-
 nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S |    4 +-
 nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S |    4 +-
 nptl/tst-spin4.c                               |  109 ++++++++++++++++++++++++
 5 files changed, 125 insertions(+), 5 deletions(-)
 create mode 100644 nptl/tst-spin4.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.16/master has been updated
       via  02b1a8f86665320e961c2d12882ca0ba6112540a (commit)
      from  c3e44c48727747659e313f09bc5557b6c1007e49 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=02b1a8f86665320e961c2d12882ca0ba6112540a

commit 02b1a8f86665320e961c2d12882ca0ba6112540a
Author: Guo Yixuan <culu.gyx@gmail.com>
Date:   Tue Jun 3 16:19:11 2014 -0700

    Fixed pthread_spin_lock on sparc32/64 (bug 16882)

        [BZ #16882]
        * nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S
        (pthread_spin_lock): Branch out of spin loop to proper location.
        * nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S
        (pthread_spin_lock): Likewise.

        * nptl/tst-spin4.c: New test.
        * nptl/Makefile (tests): Add tst-spin4.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                      |   11 +++
 NEWS                                           |    2 +-
 nptl/Makefile                                  |    2 +-
 nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S |    4 +-
 nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S |    4 +-
 nptl/tst-spin4.c                               |  109 ++++++++++++++++++++++++
 6 files changed, 126 insertions(+), 6 deletions(-)
 create mode 100644 nptl/tst-spin4.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #9 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.17/master has been updated
       via  05573613e0b4f99f2718f62957c77d5842df3592 (commit)
      from  8c19f86e356cd28180eb199f07670de5f32f82b9 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=05573613e0b4f99f2718f62957c77d5842df3592

commit 05573613e0b4f99f2718f62957c77d5842df3592
Author: Guo Yixuan <culu.gyx@gmail.com>
Date:   Tue Jun 3 16:19:11 2014 -0700

    Fixed pthread_spin_lock on sparc32/64 (bug 16882)

        [BZ #16882]
        * nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S
        (pthread_spin_lock): Branch out of spin loop to proper location.
        * nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S
        (pthread_spin_lock): Likewise.

        * nptl/tst-spin4.c: New test.
        * nptl/Makefile (tests): Add tst-spin4.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                      |   11 +++
 NEWS                                           |    2 +-
 nptl/Makefile                                  |    2 +-
 nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S |    4 +-
 nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S |    4 +-
 nptl/tst-spin4.c                               |  109 ++++++++++++++++++++++++
 6 files changed, 126 insertions(+), 6 deletions(-)
 create mode 100644 nptl/tst-spin4.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #10 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.18/master has been updated
       via  d4deb63367247eaddcbe10b872d39ff70659eaf4 (commit)
      from  f2b605ed8882a922c403386b2af7e7a5eecbf39f (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=d4deb63367247eaddcbe10b872d39ff70659eaf4

commit d4deb63367247eaddcbe10b872d39ff70659eaf4
Author: Guo Yixuan <culu.gyx@gmail.com>
Date:   Tue Jun 3 16:19:11 2014 -0700

    Fixed pthread_spin_lock on sparc32/64 (bug 16882)

        [BZ #16882]
        * nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S
        (pthread_spin_lock): Branch out of spin loop to proper location.
        * nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S
        (pthread_spin_lock): Likewise.

        * nptl/tst-spin4.c: New test.
        * nptl/Makefile (tests): Add tst-spin4.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                      |   11 +++
 NEWS                                           |    2 +-
 nptl/Makefile                                  |    2 +-
 nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S |    4 +-
 nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S |    4 +-
 nptl/tst-spin4.c                               |  109 ++++++++++++++++++++++++
 6 files changed, 126 insertions(+), 6 deletions(-)
 create mode 100644 nptl/tst-spin4.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

--- Comment #11 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.19/master has been updated
       via  344e61df0200af758e794b9843ffb37bd89e5259 (commit)
      from  690bb11f6080800be584d5db7b0b0ff61253c461 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=344e61df0200af758e794b9843ffb37bd89e5259

commit 344e61df0200af758e794b9843ffb37bd89e5259
Author: Guo Yixuan <culu.gyx@gmail.com>
Date:   Tue Jun 3 16:19:11 2014 -0700

    Fixed pthread_spin_lock on sparc32/64 (bug 16882)

        [BZ #16882]
        * nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S
        (pthread_spin_lock): Branch out of spin loop to proper location.
        * nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S
        (pthread_spin_lock): Likewise.

        * nptl/tst-spin4.c: New test.
        * nptl/Makefile (tests): Add tst-spin4.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                      |   11 +++
 NEWS                                           |    2 +-
 nptl/Makefile                                  |    2 +-
 nptl/sysdeps/sparc/sparc32/pthread_spin_lock.S |    4 +-
 nptl/sysdeps/sparc/sparc64/pthread_spin_lock.S |    4 +-
 nptl/tst-spin4.c                               |  109 ++++++++++++++++++++++++
 6 files changed, 126 insertions(+), 6 deletions(-)
 create mode 100644 nptl/tst-spin4.c

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16882] Incorrect implementation of pthread_spin_lock for sparc, no ldstub after spinning

[davem at davemloft dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=16882

David S. Miller <davem at davemloft dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from David S. Miller <davem at davemloft dot net> ---
Fixed in mainline and all relevant branches.

-- 
You are receiving this mail because:
You are on the CC list for the bug.