From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6278 invoked by alias); 14 Jan 2013 22:58:54 -0000 Received: (qmail 6268 invoked by uid 22791); 14 Jan 2013 22:58:53 -0000 X-SWARE-Spam-Status: No, hits=-3.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_DM X-Spam-Check-By: sourceware.org Received: from mail-pb0-f52.google.com (HELO mail-pb0-f52.google.com) (209.85.160.52) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 Jan 2013 22:58:48 +0000 Received: by mail-pb0-f52.google.com with SMTP id ro2so2435084pbb.39 for ; Mon, 14 Jan 2013 14:58:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding:x-gm-message-state; bh=EfLXqliiynUBRYBmhTFXwcCAL6EpaNZDebbmt82/ZJk=; b=QM3C5Jc845IFUNhr2JOuuSutuOZwc/Ac87RmmLWPrrm2mpB6hrLHWk/dtj/sP3TShn RWXcLBcvnSgpRdlqP2qJe/epX2xvA3IYIJuwcwsl39A/icFExNfytC5DFLeXpcLcpjpa JmwbAYWqfd8IOUNIRaR3gSkwZCOY9NKssgtjMXKYFnSVCHgvolq+DSASVMOHXB62G0Kc GOQg7vmWUpxBwYL+guiuqpn00muvfjjH3WNqvzhEDN19+yx6nyW6bT7ZuCDBp57RzXaN 5zUK91LHBQ80DnumOyCnZZuOJeVnqfUAYPD5A0sP5RMXfecfkgYShiSlam3XTEY7p0Jh CagA== X-Received: by 10.68.248.70 with SMTP id yk6mr258107693pbc.160.1358204327692; Mon, 14 Jan 2013 14:58:47 -0800 (PST) Received: from [192.168.2.24] (bas3-ottawa23-1128747037.dsl.bell.ca. [67.71.80.29]) by mx.google.com with ESMTPS id vo6sm8932934pbc.8.2013.01.14.14.58.45 (version=TLSv1 cipher=RC4-SHA bits=128/128); Mon, 14 Jan 2013 14:58:46 -0800 (PST) Message-ID: <50F48DA4.4000407@systemhalted.org> Date: Mon, 14 Jan 2013 22:58:00 -0000 From: Carlos O'Donell User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: "Joseph S. Myers" CC: Jeff Law , Mike Frysinger , libc-alpha , libc-ports@sourceware.org, Marcus Shawcroft Subject: Re: [PATCH] Memory fencing problem in pthread cancellation References: <50F46969.1000305@redhat.com> <50F48281.3070207@systemhalted.org> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Gm-Message-State: ALoCoQlIYr1j+Sc8gri7j++Xj0Xssv1cvqtbWwIu7SUFqlUhuQCdd3mkiUNVBmLBi3NBjFzjmPMZ X-IsSubscribed: yes Mailing-List: contact libc-ports-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: libc-ports-owner@sourceware.org X-SW-Source: 2013-01/txt/msg00019.txt.bz2 On 01/14/2013 05:21 PM, Joseph S. Myers wrote: > On Mon, 14 Jan 2013, Carlos O'Donell wrote: > >> diff --git a/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c b/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c >> index 58ca9ac..d7ac847 100644 >> --- a/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c >> +++ b/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c >> @@ -104,6 +104,7 @@ asm ( >> " mov r6, r0\n" >> " cmp r3, #0\n" >> " beq 4f\n" >> + atomic_asm_full_barrier() >> "5: mov r0, r6\n" >> " ldmfd sp!, {r4, r5, r6, lr}\n" >> " " CFI_ADJUST_CFA_OFFSET (-16) "\n" >> @@ -123,7 +124,7 @@ asm ( >> #else >> "1: .word _GLOBAL_OFFSET_TABLE_ - 3b - 8\n" >> #endif >> -"2: .word libgcc_s_resume(GOTOFF)\n" >> +"2: .word libgcc_s_handle(GOTOFF)\n" >> " .size _Unwind_Resume, .-_Unwind_Resume\n" >> ); > > That last change doesn't seem right. Note the subsequent "bx r3" if r3 > isn't zero. If you want to test libgcc_s_handle for being zero, you then > need, separately (after the barrier?) to load the libgcc_s_resume value. You are correct, we need another entry for libgcc_s_resume, and we need to load that after the barrier, otherwise we won't call libgcc_s_resume. Something like this: diff --git a/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c b/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c index 58ca9ac..1e03b13 100644 --- a/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c +++ b/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-forcedunwind.c @@ -104,7 +104,12 @@ asm ( " mov r6, r0\n" " cmp r3, #0\n" " beq 4f\n" -"5: mov r0, r6\n" + atomic_asm_full_barrier() +"5: ldr r4, 6f\n" +" ldr r5, 7f\n" +"8: add r4, pc, r4\n" +" ldr r3, [r4, r5]\n" +" mov r0, r6\n" " ldmfd sp!, {r4, r5, r6, lr}\n" " " CFI_ADJUST_CFA_OFFSET (-16) "\n" " " CFI_RESTORE (r4) "\n" @@ -123,7 +128,13 @@ asm ( #else "1: .word _GLOBAL_OFFSET_TABLE_ - 3b - 8\n" #endif -"2: .word libgcc_s_resume(GOTOFF)\n" +"2: .word libgcc_s_handle(GOTOFF)\n" +#ifdef __thumb2__ +"6: .word _GLOBAL_OFFSET_TABLE_ - 8b - 4\n" +#else +"6: .word _GLOBAL_OFFSET_TABLE_ - 8b - 8\n" +#endif +"7: .word libgcc_s_resume(GOTOFF)\n" " .size _Unwind_Resume, .-_Unwind_Resume\n" ); --- I was hoping that someone with an ARM cross-build environment could test this and tell us how it goes? :-) > (Whatever you do in unwind-forcedunwind.c, presumably much the same change > should go in unwind-resume.c as well.) Yes, all the same changes are required in: ./sysdeps/gnu/unwind-resume.c ./ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-resume.c I'd say something like this? diff --git a/sysdeps/gnu/unwind-resume.c b/sysdeps/gnu/unwind-resume.c index df845cd..fecfc94 100644 --- a/sysdeps/gnu/unwind-resume.c +++ b/sysdeps/gnu/unwind-resume.c @@ -21,6 +21,7 @@ #include #include +static void *libgcc_s_handle; static void (*libgcc_s_resume) (struct _Unwind_Exception *exc); static _Unwind_Reason_Code (*libgcc_s_personality) (int, _Unwind_Action, _Unwind_Exception_Class, struct _Unwind_Exception *, @@ -41,13 +42,25 @@ init (void) libgcc_s_resume = resume; libgcc_s_personality = personality; + atomic_write_barrier (); + /* At the point at which any thread writes the handle + to libgcc_s_handle, the initialization is complete. + The writing of libgcc_s_handle is atomic. All other + threads reading libgcc_s_handle do so atomically. Any + thread that does not execute this function must issue + a read barrier to ensure that all of the above has + actually completed and that the values of the + function pointers are correct. */ + libgcc_s_handle = handle; } void _Unwind_Resume (struct _Unwind_Exception *exc) { - if (__builtin_expect (libgcc_s_resume == NULL, 0)) + if (__builtin_expect (libgcc_s_handle == NULL, 0)) init (); + else + atomic_read_barrier (); libgcc_s_resume (exc); } @@ -57,8 +70,10 @@ __gcc_personality_v0 (int version, _Unwind_Action actions, struct _Unwind_Exception *ue_header, struct _Unwind_Context *context) { - if (__builtin_expect (libgcc_s_personality == NULL, 0)) + if (__builtin_expect (libgcc_s_handle == NULL, 0)) init (); + else + atomic_read_barrier (); return libgcc_s_personality (version, actions, exception_class, ue_header, context); } --- diff --git a/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-resume.c b/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-resume.c index 0a3ad95..a67190d 100644 --- a/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-resume.c +++ b/ports/sysdeps/unix/sysv/linux/arm/nptl/unwind-resume.c @@ -20,6 +20,7 @@ #include #include +static void *libgcc_s_handle; static void (*libgcc_s_resume) (struct _Unwind_Exception *exc); static _Unwind_Reason_Code (*libgcc_s_personality) (_Unwind_State, struct _Unwind_Exception *, struct _Unwind_Context *); @@ -41,6 +42,16 @@ init (void) libgcc_s_resume = resume; libgcc_s_personality = personality; + atomic_write_barrier (); + /* At the point at which any thread writes the handle + to libgcc_s_handle, the initialization is complete. + The writing of libgcc_s_handle is atomic. All other + threads reading libgcc_s_handle do so atomically. Any + thread that does not execute this function must issue + a read barrier to ensure that all of the above has + actually completed and that the values of the + function pointers are correct. */ + libgcc_s_handle = handle; } /* It's vitally important that _Unwind_Resume not have a stack frame; the @@ -67,7 +78,12 @@ asm ( " mov r6, r0\n" " cmp r3, #0\n" " beq 4f\n" -"5: mov r0, r6\n" + atomic_asm_full_barrier() +"5: ldr r4, 6f\n" +" ldr r5, 7f\n" +"8: add r4, pc, r4\n" +" ldr r3, [r4, r5]\n" +" mov r0, r6\n" " ldmfd sp!, {r4, r5, r6, lr}\n" " " CFI_ADJUST_CFA_OFFSET (-16) "\n" " " CFI_RESTORE (r4) "\n" @@ -76,7 +92,7 @@ asm ( " " CFI_RESTORE (lr) "\n" " bx r3\n" " " CFI_RESTORE_STATE "\n" -"4: bl init\n" +"4: bl pthread_cancel_init\n" " ldr r3, [r4, r5]\n" " b 5b\n" " " CFI_ENDPROC "\n" @@ -86,7 +102,13 @@ asm ( #else "1: .word _GLOBAL_OFFSET_TABLE_ - 3b - 8\n" #endif -"2: .word libgcc_s_resume(GOTOFF)\n" +"2: .word libgcc_s_handle(GOTOFF)\n" +#ifdef __thumb2__ +"6: .word _GLOBAL_OFFSET_TABLE_ - 8b - 4\n" +#else +"6: .word _GLOBAL_OFFSET_TABLE_ - 8b - 8\n" +#endif +"7: .word libgcc_s_resume(GOTOFF)\n" " .size _Unwind_Resume, .-_Unwind_Resume\n" ); @@ -95,7 +117,9 @@ __gcc_personality_v0 (_Unwind_State state, struct _Unwind_Exception *ue_header, struct _Unwind_Context *context) { - if (__builtin_expect (libgcc_s_personality == NULL, 0)) + if (__builtin_expect (libgcc_s_handle == NULL, 0)) init (); + else + atomic_read_barrier (); return libgcc_s_personality (state, ue_header, context); } --- It looks like the assembly in unwind-resume.c and unwind-forcedunwind.c are identical and should be shared? Cheers, Carlos.