From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 97F513858D28 for ; Tue, 20 Feb 2024 17:55:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 97F513858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 97F513858D28 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708451749; cv=none; b=fHalcaZh0FZTMR7Hsgx7aLm3hy3EWmNVCKZxpN6Ynhxa6vR8faHckwkw3XD5vearCCzge9RnVWAItys7PJ84tcIkFalULt001zHeVzDkZAWoYyo1EK5wGU9RV60Te28xhTWNEuuL3Pa3qiJrlSZ3jJktwlrSFwj3Qf7HHNVjzLs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708451749; c=relaxed/simple; bh=OdLxAo89NRneRRotrKtNYG5o2csx9x2xtM0PvK42nSM=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=f4nOdyFOuqUY4uIw29OI3+p2jcb/5l3t6vawdNZOrdqrOJpRfZtKlpdFjOSwqKLSeOseDv1ZB7m2MrzNS/X1bnXVPJ3ABCQHClsUlfEBpmKAVypyt2wKuovo8qQydkryZA1wLZgZ5iH3znUuhE0PMGIqq15bmJFy9azfUnzVpCU= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708451747; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to; bh=5D31wUccX36nw6PkOLuIqPbUXUHFw0Q1ez6spLmDa4s=; b=Ye+h52BVA3ZsrRQgK6xRkNw7+XmNwwazULG1GJ371uZMLLOG8kW4SCl/EVITdPqLlmCnEO F48lwEUeszOSDE33ZokEYQApnrUN/W4w7E71EdZnIssd+Wa3BYCWhpyKYqihDPEyAiMXj5 TJNuPCqBX/OosybUJ4+qOFlJNyJmSFM= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-331-G3kREC0-NFiNVi5a5jyo7g-1; Tue, 20 Feb 2024 12:55:44 -0500 X-MC-Unique: G3kREC0-NFiNVi5a5jyo7g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CE5453C11C68; Tue, 20 Feb 2024 17:55:43 +0000 (UTC) Received: from greed.delorie.com (unknown [10.22.8.101]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B6EED112A4; Tue, 20 Feb 2024 17:55:43 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.15.2/8.15.2) with ESMTP id 41KHthuB1420654; Tue, 20 Feb 2024 12:55:43 -0500 From: DJ Delorie To: Mathieu Desnoyers Cc: mjeanson@efficios.com, libc-alpha@sourceware.org Subject: Re: [PATCH v8 7/8] aarch64: Add rseq_load32_load32_relaxed In-Reply-To: Date: Tue, 20 Feb 2024 12:55:43 -0500 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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: Mathieu Desnoyers writes: > 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. Given that I'm looking at it as "someone not familiar with the RSEQ API[*]", perhaps a one line comment that says "compare VAR and EXPECT and ensure they're equal, else abort to LABEL" would have made me think "Oh, that makes sense". Otherwise I have to read the inline asm, and my familiarity with x86 asm would make me confused. > 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'm not pushing for an ABI change at this point. *Any* undocumented inline assembler macro is going to be confusing to someone not familiar with it, and needs a human-understandable comment or documentation for it. >>> +#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, Yes. A short comment that says "This is a slow dumb memcpy, but it can be used in an rseq abortable code sequence." would be sufficient. That's what I meant by my WHY? comment - there should be something that explains why it exists and/or why you'd use it. And for a bit of humorous snark, I'll point out that this mail thread is already longer than the new documentation would need to be ;-) [*] And someone old enough to realize that comments aren't just for your peers, but also for your future self, to remind you what the heck you were thinking when you wrote that ;-)