From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpout.efficios.com (smtpout.efficios.com [IPv6:2607:5300:203:b2ee::31e5]) by sourceware.org (Postfix) with ESMTPS id 727A83858D20 for ; Tue, 20 Feb 2024 15:08:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 727A83858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=efficios.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=efficios.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 727A83858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:5300:203:b2ee::31e5 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708441691; cv=none; b=j4haq9kj2y/y8OfjxBa0+tB4pnu4AAaBj959PCpEeRjGpZH7VnY2Qhz9Z4Mf0KEOnd+cWoOfLzBvKAYXwiEWBoOD7VwACAY1k37QIIE6Nn5AEqR3WAx4UYbY92x4f6x/2PANeR4G81NlldQvXS9CzokTVq5LqmPUCbKDVbyj/d8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708441691; c=relaxed/simple; bh=7Hqln8e6D0UbMDx78LA2sS+PyNXjYoBn33vdrQ3HMDA=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=UTzh7zjm9xkKdWnZVxnlLdL+2SOqC0NX/rBy4PbwbA80dbauFTG8BjuZpi9yCBiYDkcN23y37oMAK2o+SlK8j5wxm1GmJ26KZuxhcFL25h7E6kes5UFU4r6M82V5gUKZS9hMX3ESMNph+5VVOa5q5ih+MtLAptdtsToyNwZ7tbU= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=efficios.com; s=smtpout1; t=1708441678; bh=7Hqln8e6D0UbMDx78LA2sS+PyNXjYoBn33vdrQ3HMDA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=bKvG3h+KelxiSdUrczL296SLVnL+Ixw/ODSEka3K+ECvCH5Ji43xclTa4Gy/eRVyj 4OYn2Og90x8EcsIjdpq+5MNcCwqPW7s5KqVlKkIXqOIBzE7g/+NhVKQtgnei98kicL zVisE+SU8BMzRYgR0zZ46uNZthKcOgNRN7hdtOTDLi4z2BDUliVG1NzS37YsPTRNSe sCp5BAiNcnXmOoxeOdgNkKxzBAD2st6pRBnhEEo3CUzZsmELC3+YAK0m9CK2S47ZFC yO0kvIujyXuezmXUgAd7JilUYGhzp2yQgCCQSOYjd3i2vZMYIQMI5D6kbxUozl0uWP HWq7NoNkpzf7g== Received: from [172.16.0.134] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) by smtpout.efficios.com (Postfix) with ESMTPSA id 4TfN7f5565zbCn; Tue, 20 Feb 2024 10:07:58 -0500 (EST) Message-ID: Date: Tue, 20 Feb 2024 10:07:57 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 7/8] aarch64: Add rseq_load32_load32_relaxed Content-Language: en-US To: DJ Delorie , Michael Jeanson Cc: libc-alpha@sourceware.org References: From: Mathieu Desnoyers In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2024-02-16 22:53, DJ Delorie wrote: [...] > >> +#define RSEQ_ASM_OP_CMPEQ(var, expect, label) \ >> + " ldr " RSEQ_ASM_TMP_REG ", %[" __rseq_str(var) "]\n" \ >> + " sub " RSEQ_ASM_TMP_REG ", " RSEQ_ASM_TMP_REG \ >> + ", %[" __rseq_str(expect) "]\n" \ >> + " cbnz " RSEQ_ASM_TMP_REG ", " __rseq_str(label) "\n" > > This is why we need documentation; I would have guessed this was a CMPNE > operation, but it depends on how you define "label" This comes from the x86 implementation I originally did which has "RSEQ_ASM_CMP_CPU_ID()" and static inline functions such as "rseq_cmpeqv_storev()". The meaning of the "cmp" here is that the critical section does _not_ abort (does not branch) if the comparison matches. But I understand how the ASM helpers that were contributed for other architectures such as "RSEQ_ASM_OP_CMPEQ()", with the same semantic of "do not branch to abort if the comparison matches" can be misleading for someone used to reading assembler on pretty much any architecture, where the conditional branch is expected to be taken if the condition matches. So what I have here in librseq is backwards. Fortunately, librseq is still just a master branch (no releases yet), and the copy in the Linux kernel selftests is internal to that selftest, so there are no stable API expectations at this stage. So I don't think the semantic of e.g. "rseq_cmpeqv_storev()" is misleading: it proceeds to do the store if the comparison matches. However, the ASM macros would benefit from a logic flip. Even though the API is not stable, I would like to introduce this in a way that will allow users of the API to catch the change at compile-time. I propose the following remapping of the macros for added clarity: RSEQ_ASM_OP_CMPNE becomes RSEQ_ASM_OP_CBEQ (branch if equal) RSEQ_ASM_OP_CMPEQ becomes RSEQ_ASM_OP_CBNE (branch if not equal) RSEQ_ASM_CMP_CPU_ID becomes RSEQ_ASM_CBNE_CPU_ID (branch if cpu id is not equal) I can do this change across all architectures in librseq to keep things in sync. What do you think ? [...] > >> +#define RSEQ_ASM_OP_R_BAD_MEMCPY(dst, src, len) \ >> + " cbz %[" __rseq_str(len) "], 333f\n" \ >> + " mov " RSEQ_ASM_TMP_REG_2 ", %[" __rseq_str(len) "]\n" \ >> + "222: sub " RSEQ_ASM_TMP_REG_2 ", " RSEQ_ASM_TMP_REG_2 ", #1\n" \ >> + " ldrb " RSEQ_ASM_TMP_REG32 ", [%[" __rseq_str(src) "]" \ >> + ", " RSEQ_ASM_TMP_REG_2 "]\n" \ >> + " strb " RSEQ_ASM_TMP_REG32 ", [%[" __rseq_str(dst) "]" \ >> + ", " RSEQ_ASM_TMP_REG_2 "]\n" \ >> + " cbnz " RSEQ_ASM_TMP_REG_2 ", 222b\n" \ >> + "333:\n" > > Ok, but WHY? This is a memcpy from src to dst which can be aborted at any point during the copy. Do you recommend we add documentation about what it does, or that we remove it for now given that it is not used by the initial static inline ? Keeping all those helpers there simplifies the task of keeping librseq and glibc in sync. But I would also understand if you prefer that we only introduce what we use. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com