From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 923751F45E for ; Tue, 11 Feb 2020 20:29:30 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:to:cc:references:from:subject:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=ewVoYTPhYuKf9ecq sZLFSkGVoNR7spkHSV3uWW1Le6Qp7XaplCKpfi6sEv2MrTDxRxJC4mClx9eXCEIT LW5n1DaqKeDVKhVHUA+CwUc6BwIt/1WcPeyxBJ8gbbfLpFJARkmIg6y3W5aeSmK+ j1faq71pS4IFl3TtVlUdNsGHCS0= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:to:cc:references:from:subject:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=6Mlx7+zSbcuvFYKL/f423l xozT8=; b=tQC63uh8F4nTgf1MuRpL+DNAw9hVBZvJFuUyrG0B/MhfHk2Iz08YVK Bfk9avw3pqxLFtwKmh5G+gYHe+YSwBCgnNFX+mCKFbHWsNlrTXb9WkGdtn2p0f4+ bMF0S+Bex7ipbCOQErnOm4a7vTHe2qsFsc7GsRxkO6hgmLVwnlnec= Received: (qmail 75014 invoked by alias); 11 Feb 2020 20:29:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 74993 invoked by uid 89); 11 Feb 2020 20:29:27 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: mail-qt1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:cc:references:from:autocrypt:subject:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=gP1JQ0cWeM95bGi6CMN8rdbrkVGB6QkONN4L475t27I=; b=JVD+2zJ+1QmlbIb0r3xcprIZ0sgCweEZ/718rIiiUK96X86O7cVOv3yq+GlgnVGi09 q7wfoQqM/2CwDkOwAyD/9n46U9Y6M87saPIWi3gxjIr5xEHraKf3Vf6TDxDGPN/uuxk6 QTMNJF+pagSV7rRJ+RjMm8Z2bVKrMsCaOPzGDqZ02hTOjwhiuXrTHGZHQJ5Q/pbCjOb4 bjGpthn2XaIlrCjy9kUXBEyzwMVG1LiWVkcVQJSiXr6DEWTMAJhgiv53xKGzyZo7INPq EeEIiYczkehVvszpSdZSCnplH2SkD1sbPoyTOszBY48HO+JaJtCLghj9AuHufpUjcIVd Pd8g== To: Florian Weimer Cc: libc-alpha@sourceware.org References: <20200210192038.23588-1-adhemerval.zanella@linaro.org> <20200210192038.23588-3-adhemerval.zanella@linaro.org> <878sl9pe78.fsf@oldenburg2.str.redhat.com> <10c5636d-6ea7-e95c-3a3c-67be298472fd@linaro.org> <874kvxnczu.fsf@mid.deneb.enyo.de> From: Adhemerval Zanella Subject: Re: [PATCH 03/15] sparc: Use Linux kABI for syscall return Message-ID: Date: Tue, 11 Feb 2020 17:29:20 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <874kvxnczu.fsf@mid.deneb.enyo.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 11/02/2020 16:24, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 11/02/2020 08:15, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> + if (__glibc_unlikely (__g1 != 0)) \ >>> >>> This change is inconsistent with the other updates, which use __g1 == >>> -1. Is this deliberate? >>> >>> Thanks, >>> Florian >>> >> >> In fact __SYSCALL_STRING already sets the 'o0' to a negative value if >> the 'xcc' condition is set (indicating that the syscall has failed). >> The 'g1' check is superfluous, it will be always true since 'g1' will >> be either 0 or 1. >> >> And both the set and check of 'g1' result is also superfluous, since 'o0' >> will already hold all the required information. >> >> Below is an updated patch, checked on sparc64-linux-gnu and >> sparcv9-linux-gnu. > > I see, nice additional cleanup. > >> It changes the sparc internal_syscall* macros to return a negative >> value instead the 'g1' register value on 'err' macro argument. > ^ of ^ in the? > > Ack. >> The __SYSCALL_STRING macro is also changed to no set the 'g1' > ^ not >> value, since 'o1' already holds all the required information >> to check if syscall has failed. >> >> The macro INTERNAL_SYSCALL_DECL is no longer required, and the >> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS. The > ^ macro? ^ kABIs Ack. > (or drop the “the“ on the preceding line) > >> redefinition of INTERNAL_VSYSCALL_CALL is also no longer >> required. >> >> Checked on sparc64-linux-gnu and sparcv9-linux-gnu. It fixes >> the sporadic issues on sparc32 where clock_nanosleep does not >> act as cancellation entrypoint. > > I double-checked this against the kernel sources, and entry.S has > this: > > ret_sys_call: > ld [%curptr + TI_FLAGS], %l6 > cmp %o0, -ERESTART_RESTARTBLOCK > ld [%sp + STACKFRAME_SZ + PT_PSR], %g3 > set PSR_C, %g2 > bgeu 1f > > But ERESTART_RESTARTBLOCK is not 4095, so glibc with this change will > now treat certain internal kernel error codes as errors, while they > were previously reported as success. This looks like a kernel bug, in > that ERESTART_RESTARTBLOCK was not updated when more error codes were > added. On the other hand, these error codes should never leak into > userspace. My understanding is such errors should not be visible by the application, as indicated by include/linux/errno.h comment. And it seems to be the case for sparc, at least on: arch/sparc/kernel/signal_64.c 477 static void do_signal(struct pt_regs *regs, unsigned long orig_i0) [...] 514 restart_syscall = 0; 515 if (pt_regs_is_syscall(regs) && 516 (regs->tstate & (TSTATE_XCARRY | TSTATE_ICARRY))) { 517 restart_syscall = 1; 518 orig_i0 = regs->u_regs[UREG_G6]; 519 } 520 521 if (has_handler) { 522 if (restart_syscall) 523 syscall_restart(orig_i0, regs, &ksig.ka.sa); 524 signal_setup_done(setup_rt_frame(&ksig, regs), &ksig, 0); 525 } else { 526 if (restart_syscall) { 527 switch (regs->u_regs[UREG_I0]) { 528 case ERESTARTNOHAND: 529 case ERESTARTSYS: 530 case ERESTARTNOINTR: 531 /* replay the system call when we are done */ 532 regs->u_regs[UREG_I0] = orig_i0; 533 regs->tpc -= 4; 534 regs->tnpc -= 4; 535 pt_regs_clear_syscall(regs); 536 /* fall through */ 537 case ERESTART_RESTARTBLOCK: 538 regs->u_regs[UREG_G1] = __NR_restart_syscall; 539 regs->tpc -= 4; 540 regs->tnpc -= 4; 541 pt_regs_clear_syscall(regs); 542 } 543 } 544 If signal has a handler, syscall_restart will either set EINTR or previous 'o0' value. Otherwise if syscall should be restarted, either it will be trying again (line 538) or previous error code would be set. > > To me, your patch looks good. >